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

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

[Ramsay Jones]

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(-)

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

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

[Jonathan Nieder]

Hi Ramsay,

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).

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

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

[Peter Harris]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

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

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

Peter Harris

[-- Attachment #2: msvc.patch --]
[-- Type: application/octet-stream, Size: 1010 bytes --]

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);

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

[Ramsay Jones]

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

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

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

[Ramsay Jones]

Jonathan Nieder wrote:
> 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 ...]

> 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

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

[Jonathan Nieder]

Ramsay Jones wrote:

> 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]
>> 
>> 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.

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

[Bernhard R. Link]

* Ramsay Jones <ramsay@ramsay1.demon.co.uk> [100626 20:02]:
> 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...

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

[Peter Harris]

On Fri, Jun 25, 2010 at 4:24 PM, Ramsay Jones wrote:
> 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

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

[Ramsay Jones]

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

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

[Ramsay Jones]

Peter Harris wrote:
> 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.

>> 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

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

[Andreas Schwab]

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."

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

[Peter Harris]

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