threads / patch / 24179

patch, 4 partsMakefile: Don't pass $(ALL_CFLAGS) to the linker

Subject: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

## tl;dr

12 messages between Jun 23, 2010 and Jul 1, 2010. Diffs are folded; open one to read it.

replies: 11people: 5as markdown or json

Ramsay Jones· Jun 23, 2010, 19:48 UTC · lore

The msvc debug build (make MSVC=1 DEBUG=1) issues a warning on every invocation of the linker:

    LINK : warning LNK4044: unrecognized option '/Zi'; ignored

In order to suppress the warning, we refrain from passing the $(ALL_CFLAGS) macro to the linker.

Note that, should it be necessary in the future, an option intended for both the (front-end) compiler and the linker can be included in both CFLAGS and LDFLAGS.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
 Makefile |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
Show changes to Makefile +7 −7
diff --git a/Makefile b/Makefile
index b979fb7..2730199 100644
--- a/Makefile
+++ b/Makefile
@@ -1515,7 +1515,7 @@ git.s git.o: EXTRA_CPPFLAGS = -DGIT_VERSION='"$(GIT_VERSION)"' \
 	'-DGIT_HTML_PATH="$(htmldir_SQ)"'
 
 git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
+	$(QUIET_LINK)$(CC) -o $@ git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
 builtin/help.o: common-cmds.h
@@ -1810,17 +1810,17 @@ http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
 endif
 
 git-%$X: %.o $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
+	$(QUIET_LINK)$(CC) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
 git-imap-send$X: imap-send.o $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+	$(QUIET_LINK)$(CC) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(OPENSSL_LINK) $(OPENSSL_LIBSSL)
 
 git-http-fetch$X: revision.o http.o http-walker.o http-fetch.o $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+	$(QUIET_LINK)$(CC) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL)
 git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+	$(QUIET_LINK)$(CC) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
 $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
@@ -1830,7 +1830,7 @@ $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
 	cp $< $@
 
 $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
+	$(QUIET_LINK)$(CC) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
 $(LIB_FILE): $(LIB_OBJS)
@@ -1934,7 +1934,7 @@ test-parse-options$X: parse-options.o
 .PRECIOUS: $(TEST_OBJS)
 
 test-%$X: test-%.o $(GITLIBS)
-	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
+	$(QUIET_LINK)$(CC) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
 check-sha1:: test-sha1$X
 	./test-sha1.sh

-- 
1.7.1
Jonathan Nieder· Jun 23, 2010, 21:08 UTC · re: Ramsay Jones · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

Hi Ramsay,
Ramsay Jones wrote:
Show 8 quoted lines
>     LINK : warning LNK4044: unrecognized option '/Zi'; ignored
> 
> In order to suppress the warning, we refrain from passing the
> $(ALL_CFLAGS) macro to the linker.
> 
> Note that, should it be necessary in the future, an option
> intended for both the (front-end) compiler and the linker can
> be included in both CFLAGS and LDFLAGS.

I think traditionally CPPFLAGS is meant to be used for the purpose you are describing (see [1] for example).

I realize that the Makefile does not currently use the terms this way: making it consistent would require

 . s/BASIC_CFLAGS/BASIC_CPPFLAGS/, except that the
	BASIC_CFLAGS += -Kthread
   settings should probably stay as-is
 . Windows BASIC_CFLAGS would probably need to be split:
	BASIC_CFLAGS = -nologo
	BASIC_CPPFLAGS = -I. -I../zlib ... -DWIN32 ...
 . s/COMPAT_CFLAGS/COMPAT_CPPFLAGS

What do you think? Jonathan

[1] http://www.gnu.org/software/autoconf/manual/html_node/Preset-Output-Variables.html#index-CFLAGS-80
Ramsay Jones· Jun 26, 2010, 17:59 UTC · re: Jonathan Nieder · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

Jonathan Nieder wrote:
Show 13 quoted lines
> Ramsay Jones wrote:
> 
>>     LINK : warning LNK4044: unrecognized option '/Zi'; ignored
>>
>> In order to suppress the warning, we refrain from passing the
>> $(ALL_CFLAGS) macro to the linker.
>>
>> Note that, should it be necessary in the future, an option
>> intended for both the (front-end) compiler and the linker can
>> be included in both CFLAGS and LDFLAGS.
> 
> I think traditionally CPPFLAGS is meant to be used for the purpose
> you are describing (see [1] for example).
Really? I thought that the general scheme was something like:
    - LDFLAGS is for options which only affects the operation of
      the linker (e.g. -L).
    - CPPFLAGS is for options which only affects the operation of
      the C pre-processor (e.g. -I, -D, -U)
    - CFLAGS is for options which only affects the operation of
      the compiler proper.

If an option affects multiple phases, then (one option) is to include it into each of the above macros to which it applies.

In practice, of course, I've yet to see a Makefile which faithfully implements the above scheme. ;-)

Also, the last time I was forced to use automake (yuck), I noticed that it passed CPPFLAGS to the linker; I consider this to be a bug in automake. :-P

[CFLAGS et. al. are also supposed to be user settable ...]
Show 17 quoted lines
> I realize that the Makefile does not currently use the terms this way:
> making it consistent would require
> 
>  . s/BASIC_CFLAGS/BASIC_CPPFLAGS/, except that the
> 
> 	BASIC_CFLAGS += -Kthread
> 
>    settings should probably stay as-is
> 
>  . Windows BASIC_CFLAGS would probably need to be split:
> 
> 	BASIC_CFLAGS = -nologo
> 	BASIC_CPPFLAGS = -I. -I../zlib ... -DWIN32 ...
> 
>  . s/COMPAT_CFLAGS/COMPAT_CPPFLAGS
> 
> What do you think?

I think I am missing something, since I don't see how this relates to my patch!

I'm sure the misunderstanding is mine; sorry to be so dense!

ATB, Ramsay Jones

Jonathan Nieder· Jun 26, 2010, 18:19 UTC · re: Ramsay Jones · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

Ramsay Jones wrote:
Show 11 quoted lines
> I thought that the general scheme was something like:
> 
>     - LDFLAGS is for options which only affects the operation of
>       the linker (e.g. -L).
>     - CPPFLAGS is for options which only affects the operation of
>       the C pre-processor (e.g. -I, -D, -U)
>     - CFLAGS is for options which only affects the operation of
>       the compiler proper.
>
> If an option affects multiple phases, then (one option) is to include
> it into each of the above macros to which it applies.

That does make sense. Perhaps “traditionally” was not the right word; I was just looking at common practice. I’ve just never seen -O put into LDFLAGS, for example, and I think most people would expect setting CFLAGS=-O0 to affect the linker, too.

> Jonathan Nieder wrote:
>> I realize that the Makefile does not currently use the terms this way:
>> making it consistent would require
>> 
>>  . s/BASIC_CFLAGS/BASIC_CPPFLAGS/, except that the
[etc]
Show 5 quoted lines
>> 
>> What do you think?
>
> I think I am missing something, since I don't see how this relates to
> my patch!
I suspect the problem comes from my unfamiliarity with MSVC.

By analogy with GCC, I assumed that any parameter is intended for the preprocessor, the linker, or the frontend/both. In other words, I was assuming flags for the compiler proper can be safely passed to the linker, and that /Zi must be for the preprocessor-using stages only. CFLAGS tends to contain front-end arguments used for all stages of translation.

What does /Zi do?  Searching online reveals:
 - it means “enable debugging information”
 - weakly implies /debug for the linker
 - overrides /Z7 and /Zd, and is overridden by them
 - is incompatible with /MP
 - is among the prerequisites for /dlp and for /Gm

Maybe clink.pl should use “cl.exe /link” instead of “link.exe” and this problem would go away.

Ramsay Jones· Jun 30, 2010, 19:40 UTC · re: Jonathan Nieder · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

Jonathan Nieder wrote:
> That does make sense.  Perhaps “traditionally” was not the right word;
> I was just looking at common practice.  I’ve just never seen -O put
> into LDFLAGS, for example, and I think most people would expect
> setting CFLAGS=-O0 to affect the linker, too.

[This is now a bit off-topic, see below, but what version of gcc are you using that passes -O to the linker (well, collect2 anyway)? Do you have to pass -flto? For example "gcc -v -g -O2 hello.o -o hello" seems to show (for both gcc 3.4.4 and gcc 4.4.0) that neither -g or -O2 is passed to collect2.

Hmmm, the output from gcc 4.4.0 adds a line like:
    COLLECT_GCC_OPTIONS='-v' '-g' '-O2' '-o' 'hello.exe' '-mtune=i386'
so, does this imply this is a env. var exported to collect2?
dunno ;-P ]
> I suspect the problem comes from my unfamiliarity with MSVC.
Heh, I'm not an expert user of msvc either!
> Maybe clink.pl should use “cl.exe /link” instead of “link.exe” and this
> problem would go away.

I tried this *very* quickly and it didn't work. (It's quite possible that I could spend some time and get it to work, but ...)

The patch that Peter sent seems like the way to go, since it does not affect the regular (non-msvc) build - and it works! ;-)

So we can drop this patch. Thanks for your time.

ATB, Ramsay Jones

Andreas Schwab· Jun 30, 2010, 21:16 UTC · re: Ramsay Jones · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> Hmmm, the output from gcc 4.4.0 adds a line like:
>     COLLECT_GCC_OPTIONS='-v' '-g' '-O2' '-o' 'hello.exe' '-mtune=i386'
> so, does this imply this is a env. var exported to collect2?
This is needed for the case that the linker needs to rerun the compiler.
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Bernhard R. Link· Jun 27, 2010, 09:44 UTC · re: Ramsay Jones · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

* Ramsay Jones <ramsay@ramsay1.demon.co.uk> [100626 20:02]:
Show 8 quoted lines
> Really? I thought that the general scheme was something like:
>
>     - LDFLAGS is for options which only affects the operation of
>       the linker (e.g. -L).
>     - CPPFLAGS is for options which only affects the operation of
>       the C pre-processor (e.g. -I, -D, -U)
>     - CFLAGS is for options which only affects the operation of
>       the compiler proper.

I think the problem are options like -g or -m64 and the like which you also want to give to the linker on some systems, so it is quite common and I'd say recommended to have the compiling stage get CPPFLAGS and CFLAGS and the linking stage getting CFLAGS and LDFLAGS (and link using the compiler, otherwise you might miss libraries anyway). I do not know if automake started with this, or if it is older.

> Also, the last time I was forced to use automake (yuck), I noticed that
> it passed CPPFLAGS to the linker; I consider this to be a bug in
> automake. :-P

Huh? Never heared of that bug, it must be more than 15 years ago. Are you sure it was not CFLAGS it passed to the linker?

	Bernhard R. Link
No need to CC me, I'm subscribed...
Peter Harris· Jun 24, 2010, 15:30 UTC · re: Ramsay Jones · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

On Wed, Jun 23, 2010 at 3:48 PM, Ramsay Jones wrote:
Show 8 quoted lines
>
> The msvc debug build (make MSVC=1 DEBUG=1) issues a warning
> on every invocation of the linker:
>
>    LINK : warning LNK4044: unrecognized option '/Zi'; ignored
>
> In order to suppress the warning, we refrain from passing the
> $(ALL_CFLAGS) macro to the linker.

Alternatively, we could leave the makefile alone and fix it up in compat/vcbuild/scripts/clink.pl

(Apologies for the patch attached instead of inline; I'm not using a whitespace-clean email client at the moment.)

Peter Harris
Show changes to 2 files +7 −1

Makefile, compat/vcbuild/scripts/clink.pl

diff --git a/Makefile b/Makefile
index 7134509..dccee8e 100644
--- a/Makefile
+++ b/Makefile
@@ -1072,7 +1072,8 @@ ifndef DEBUG
 	BASIC_LDFLAGS += -LTCG
 	AR += -LTCG
 else
 	BASIC_CFLAGS += -Zi -MTd
+	BASIC_LDFLAGS = -DEBUG -MAP
 endif
 	X = .exe
 endif
diff --git a/compat/vcbuild/scripts/clink.pl b/compat/vcbuild/scripts/clink.pl
index 4374771..aec9980 100644
--- a/compat/vcbuild/scripts/clink.pl
+++ b/compat/vcbuild/scripts/clink.pl
@@ -12,10 +12,14 @@
 use strict;
 my @args = ();
 my @cflags = ();
+my @lflags = ();
 my $is_linking = 0;
+my %linkflag = ( '-DEBUG' => 1, '-MAP' => 1 );
 while (@ARGV) {
 	my $arg = shift @ARGV;
-	if ("$arg" =~ /^-[DIMGO]/) {
+	if ($linkflag{$arg}) {
+		push(@lflags, $arg);
+	} elsif ("$arg" =~ /^-[DIMGOZ]/) {
 		push(@cflags, $arg);
 	} elsif ("$arg" eq "-o") {
 		my $file_out = shift @ARGV;
@@ -44,6 +48,7 @@ while (@ARGV) {
 }
 if ($is_linking) {
 	unshift(@args, "link.exe");
+	push(@args, @lflags);
 } else {
 	unshift(@args, "cl.exe");
 	push(@args, @cflags);
Ramsay Jones· Jun 25, 2010, 20:24 UTC · re: Peter Harris · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

Peter Harris wrote:
Show 11 quoted lines
> On Wed, Jun 23, 2010 at 3:48 PM, Ramsay Jones wrote:
>> The msvc debug build (make MSVC=1 DEBUG=1) issues a warning
>> on every invocation of the linker:
>>
>>    LINK : warning LNK4044: unrecognized option '/Zi'; ignored
>>
>> In order to suppress the warning, we refrain from passing the
>> $(ALL_CFLAGS) macro to the linker.
> 
> Alternatively, we could leave the makefile alone and fix it up in
> compat/vcbuild/scripts/clink.pl

Hmm, OK ... but it would be much easier if we didn't pass inappropriate options in the first place. :-P

Also, are you correctly filtering *all* possible inappropriate options? For example, the very next patch on my branch (not sent to list) adds a -W3 option ... ;-)

ATB, Ramsay Jones

Peter Harris· Jun 27, 2010, 20:45 UTC · re: Ramsay Jones · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

On Fri, Jun 25, 2010 at 4:24 PM, Ramsay Jones wrote:
Show 15 quoted lines
> Peter Harris wrote:
>> On Wed, Jun 23, 2010 at 3:48 PM, Ramsay Jones wrote:
>>> The msvc debug build (make MSVC=1 DEBUG=1) issues a warning
>>> on every invocation of the linker:
>>>
>>>    LINK : warning LNK4044: unrecognized option '/Zi'; ignored
>>>
>>> In order to suppress the warning, we refrain from passing the
>>> $(ALL_CFLAGS) macro to the linker.
>>
>> Alternatively, we could leave the makefile alone and fix it up in
>> compat/vcbuild/scripts/clink.pl
>
> Hmm, OK ... but it would be much easier if we didn't pass inappropriate
> options in the first place. :-P
Well, yes and no.

The vast majority of git devs work in an environment where compiler flags and linker flags aren't completely disjoint, and it's not very polite of us to rework their makefile to fit our view of the world. Especially when we already have a wrapper that fits their world to ours.

So I suggest that improving clink.pl is a whole lot easier for a whole lot more people (compared to reworking the entire makefile being easier for far fewer people).

> Also, are you correctly filtering *all* possible inappropriate options?
> For example, the very next patch on my branch (not sent to list) adds
> a -W3 option ... ;-)
Heck no. This only filters the options that are used by the git makefile.
But adding a filter for -W* is a trivial one liner (one character-er, even).
Peter Harris
Ramsay Jones· Jun 30, 2010, 19:52 UTC · re: Peter Harris · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

Peter Harris wrote:
Show 5 quoted lines
> The vast majority of git devs work in an environment where compiler
> flags and linker flags aren't completely disjoint, and it's not very
> polite of us to rework their makefile to fit our view of the world.
> Especially when we already have a wrapper that fits their world to
> ours.
Yep, good point.
Show 6 quoted lines
>> Also, are you correctly filtering *all* possible inappropriate options?
>> For example, the very next patch on my branch (not sent to list) adds
>> a -W3 option ... ;-)
> 
> Heck no. This only filters the options that are used by the git makefile.
> But adding a filter for -W* is a trivial one liner (one character-er, even).

Your patch fixes my problem, and is *much* better than my patch, so could you send a formal patch to the list (with W added to the the regex as well as Z)?

Thanks.

ATB, Ramsay Jones

Peter Harris· Jul 1, 2010, 21:27 UTC · re: Ramsay Jones · lore

Re: [PATCH 4/4] Makefile: Don't pass $(ALL_CFLAGS) to the linker

On Wed, Jun 30, 2010 at 3:52 PM, Ramsay Jones wrote:
> Your patch fixes my problem, and is *much* better than my patch, so could
> you send a formal patch to the list (with W added to the the regex as well
> as Z)?
I'm currently on vacation. I'll try to remember to do that on Monday.
Peter Harris

← back to recent threads