ld_compile: force CFLAGS really necessary ?

ld_compile: force CFLAGS really necessary ?

[Mike Frysinger]

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

i fetched binutils-2.18 and ran it through some `make check` only to find ld 
oddly failling some tests on me ... after digging through it a bit, it seems 
it's due to mixing of CFLAGS and CXXFLAGS.

on my system, i throw -Wimplicit-function-declaration into my CFLAGS which 
does two things: (1) shows me implicit functions so i can send patches to fix 
code and (2) see when packages wrongly mix CFLAGS and CXXFLAGS.

in ld/testsuite/lib/ld-lib.exp, the ld_compile function will append $CFLAGS to 
all commands given it.  this means even tests like ld-cdtest which compiles 
C++ code and which will cause erroneous failures when CFLAGS contains things 
that are not valid in CXXFLAGS.  doing a quick grep against ld_compile, i see 
CFLAGS is explicitly specified in the call to ld_compile by over half the 
tests which use this function ... and on my system, simply removing the 
CFLAGS append inside of the ld_compile function doesnt cause any regressions.

so the question is, do we drop this auto CFLAGS append completely ?  or do we 
do a check against the suffix of $source and if it is set to .cc, auto append 
CXXFLAGS rather than CFLAGS ?
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: ld_compile: force CFLAGS really necessary ?

[Mike Frysinger]

[-- Attachment #1.1: Type: text/plain, Size: 1571 bytes --]

On Tuesday 28 August 2007, Mike Frysinger wrote:
> i fetched binutils-2.18 and ran it through some `make check` only to find
> ld oddly failling some tests on me ... after digging through it a bit, it
> seems it's due to mixing of CFLAGS and CXXFLAGS.
>
> on my system, i throw -Wimplicit-function-declaration into my CFLAGS which
> does two things: (1) shows me implicit functions so i can send patches to
> fix code and (2) see when packages wrongly mix CFLAGS and CXXFLAGS.
>
> in ld/testsuite/lib/ld-lib.exp, the ld_compile function will append $CFLAGS
> to all commands given it.  this means even tests like ld-cdtest which
> compiles C++ code and which will cause erroneous failures when CFLAGS
> contains things that are not valid in CXXFLAGS.  doing a quick grep against
> ld_compile, i see CFLAGS is explicitly specified in the call to ld_compile
> by over half the tests which use this function ... and on my system, simply
> removing the CFLAGS append inside of the ld_compile function doesnt cause
> any regressions.
>
> so the question is, do we drop this auto CFLAGS append completely ?  or do
> we do a check against the suffix of $source and if it is set to .cc, auto
> append CXXFLAGS rather than CFLAGS ?

maybe something like the attached patch

2007-08-28  Mike Frysinger  <vapier@gentoo.org>

	* lib/ld-lib.exp (default_ld_compile): Pull in global CXXFLAGS and
	add it to $flags when $source matches *.cc.
	(run_ld_link_exec_tests): Pull in global CXXFLAGS and execute CXX
	with CXXFLAGS when $lang matches c++.
	(run_cc_link_tests): Likewise.
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: binutils-ld-tests-CXX.patch --]
[-- Type: text/x-diff, Size: 2335 bytes --]

2007-08-28  Mike Frysinger  <vapier@gentoo.org>

	* lib/ld-lib.exp (default_ld_compile): Pull in global CXXFLAGS and
	add it to $flags when $source matches *.cc.
	(run_ld_link_exec_tests): Pull in global CXXFLAGS and execute CXX
	with CXXFLAGS when $lang matches c++.
	(run_cc_link_tests): Likewise.

--- binutils-2.18/ld/testsuite/lib/ld-lib.exp
+++ binutils-2.18/ld/testsuite/lib/ld-lib.exp
@@ -206,6 +206,7 @@
 #
 proc default_ld_compile { cc source object } {
     global CFLAGS
+    global CXXFLAGS
     global srcdir
     global subdir
     global host_triplet
@@ -222,7 +223,11 @@
 
     catch "exec rm -f $object" exec_output
 
-    set flags "-I$srcdir/$subdir $CFLAGS"
+    if {[string match "*.cc" $source]} then {
+	set flags "-I$srcdir/$subdir $CXXFLAGS"
+    } else {
+	set flags "-I$srcdir/$subdir $CFLAGS"
+    }
 
     # If we are compiling with gcc, we want to add gcc_gas_flag to
     # flags.  Rather than determine this in some complex way, we guess
@@ -1287,6 +1287,7 @@
     global CC
     global CXX
     global CFLAGS
+    global CXXFLAGS
     global errcnt
     global exec_output
 
@@ -1321,7 +1322,11 @@
 	    # We ignore warnings since some compilers may generate
 	    # incorrect section attributes and the assembler will warn
 	    # them.
-	    ld_compile "$CC -c $CFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    if { [ string match "c++" $lang ] } {
+		ld_compile "$CXX -c $CXXFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    } else {
+		ld_compile "$CC -c $CFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    }
 
 	    # We have to use $CC to build PIE and shared library.
 	    if { [ string match "c" $lang ] } {
@@ -1413,6 +1418,7 @@
     global CC
     global CXX
     global CFLAGS
+    global CXXFLAGS
 
     foreach testitem $ldtests {
 	set testname [lindex $testitem 0]
@@ -1434,7 +1440,11 @@
 	    # We ignore warnings since some compilers may generate
 	    # incorrect section attributes and the assembler will warn
 	    # them.
-	    ld_compile "$CC -c $CFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    if { [ string match "c++" $lang ] } {
+		ld_compile "$CXX -c $CXXFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    } else {
+		ld_compile "$CC -c $CFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    }
 	}
 
 	# Clear error and warning counts.

Re: ld_compile: force CFLAGS really necessary ?

[Nick Clifton]

Hi Mike,

>> so the question is, do we drop this auto CFLAGS append completely ?  or do
>> we do a check against the suffix of $source and if it is set to .cc, auto
>> append CXXFLAGS rather than CFLAGS ?

I think the latter.

> maybe something like the attached patch
> 
> 2007-08-28  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* lib/ld-lib.exp (default_ld_compile): Pull in global CXXFLAGS and
> 	add it to $flags when $source matches *.cc.
> 	(run_ld_link_exec_tests): Pull in global CXXFLAGS and execute CXX
> 	with CXXFLAGS when $lang matches c++.
> 	(run_cc_link_tests): Likewise.

Nice, but I see one small niggle:

   * Not all C++ source files use the .cc file extension.  Earlier on in proc 
default_ld_compile there is code to determine if the compiler is gcc or g++, 
maybe you could tap into that ?  Of course you can then argue that we are not 
handling the case when the compiler is not gcc/g++.  I am not sure if this is a 
problem worth worrying about, but if it is we could always generate another 
patch to add an optional fourth parameter to default_ld_compile which specifies 
the language being compiled.

Cheers
   Nick

Re: ld_compile: force CFLAGS really necessary ?

[Mike Frysinger]

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

On Wednesday 29 August 2007, Nick Clifton wrote:
> > maybe something like the attached patch
> >
> > 2007-08-28  Mike Frysinger  <vapier@gentoo.org>
> >
> > 	* lib/ld-lib.exp (default_ld_compile): Pull in global CXXFLAGS and
> > 	add it to $flags when $source matches *.cc.
> > 	(run_ld_link_exec_tests): Pull in global CXXFLAGS and execute CXX
> > 	with CXXFLAGS when $lang matches c++.
> > 	(run_cc_link_tests): Likewise.
>
> Nice, but I see one small niggle:
>
>    * Not all C++ source files use the .cc file extension.  Earlier on in
> proc default_ld_compile there is code to determine if the compiler is gcc
> or g++, maybe you could tap into that ?

i think you mean latter on in default_ld_compile ?  are you referring to this 
code snippet ?
    set ccexe [string replace $ccexe 0 [string last "/" $ccexe] ""]
    if {[string match "*gcc*" $ccexe] || [string match "*++*" $ccexe]} then {
	set flags "$gcc_gas_flag $flags"
    }

> Of course you can then argue that 
> we are not handling the case when the compiler is not gcc/g++.  I am not
> sure if this is a problem worth worrying about, but if it is we could
> always generate another patch to add an optional fourth parameter to
> default_ld_compile which specifies the language being compiled.

i think i'm happy with the current stuff ...
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: ld_compile: force CFLAGS really necessary ?

[Nick Clifton]

Hi Mike,

>>    * Not all C++ source files use the .cc file extension.  Earlier on in
>> proc default_ld_compile there is code to determine if the compiler is gcc
>> or g++, maybe you could tap into that ?
> 
> i think you mean latter on in default_ld_compile ?

Yes.

 > are you referring to this code snippet ?
>     set ccexe [string replace $ccexe 0 [string last "/" $ccexe] ""]
>     if {[string match "*gcc*" $ccexe] || [string match "*++*" $ccexe]} then {
> 	set flags "$gcc_gas_flag $flags"
>     }

Yes.  I thought that you could split it up into separate "*gcc*" and "*++*" 
tests and add in the append of CFLAGS or CXXFLAGS to flags as appropriate.

Cheers
   Nick

Re: ld_compile: force CFLAGS really necessary ?

[Mike Frysinger]

[-- Attachment #1.1: Type: text/plain, Size: 789 bytes --]

On Wednesday 29 August 2007, Nick Clifton wrote:
>  > are you referring to this code snippet ?
> >
> >     set ccexe [string replace $ccexe 0 [string last "/" $ccexe] ""]
> >     if {[string match "*gcc*" $ccexe] || [string match "*++*" $ccexe]}
> > then { set flags "$gcc_gas_flag $flags"
> >     }
>
> Yes.  I thought that you could split it up into separate "*gcc*" and "*++*"
> tests and add in the append of CFLAGS or CXXFLAGS to flags as appropriate.

updated version attached

2007-09-01  Mike Frysinger  <vapier@gentoo.org>

	* lib/ld-lib.exp (default_ld_compile): Pull in global CXXFLAGS and
	add it to $flags when $ccexe matches *++*.
	(run_ld_link_exec_tests): Pull in global CXXFLAGS and execute CXX
	with CXXFLAGS when $lang matches c++.
	(run_cc_link_tests): Likewise.
-mike

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: binutils-ld-tests-CXX.patch --]
[-- Type: text/x-diff, Size: 2827 bytes --]

2007-09-01  Mike Frysinger  <vapier@gentoo.org>

	* lib/ld-lib.exp (default_ld_compile): Pull in global CXXFLAGS and
	add it to $flags when $ccexe matches *++*.
	(run_ld_link_exec_tests): Pull in global CXXFLAGS and execute CXX
	with CXXFLAGS when $lang matches c++.
	(run_cc_link_tests): Likewise.

--- ld/testsuite/lib/ld-lib.exp
+++ ld/testsuite/lib/ld-lib.exp
@@ -207,6 +207,7 @@ proc default_ld_simple_link { ld target 
 #
 proc default_ld_compile { cc source object } {
     global CFLAGS
+    global CXXFLAGS
     global srcdir
     global subdir
     global host_triplet
@@ -224,7 +225,7 @@ proc default_ld_compile { cc source obje
     remote_file build delete "$object"
     remote_file host delete "$object"
 
-    set flags "-I$srcdir/$subdir $CFLAGS"
+    set flags "-I$srcdir/$subdir"
 
     # If we are compiling with gcc, we want to add gcc_gas_flag to
     # flags.  Rather than determine this in some complex way, we guess
@@ -242,6 +243,12 @@ proc default_ld_compile { cc source obje
 	set flags "$gcc_gas_flag $flags"
     }
 
+    if {[string match "*++*" $ccexe]} {
+	set flags "$flags $CXXFLAGS"
+    } else {
+	set flags "$flags $CFLAGS"
+    }
+
     if [board_info [target_info name] exists multilib_flags] {
 	append flags " [board_info [target_info name] multilib_flags]"
     }
@@ -1285,6 +1292,7 @@ proc run_ld_link_exec_tests { targets_to
     global CC
     global CXX
     global CFLAGS
+    global CXXFLAGS
     global errcnt
     global exec_output
 
@@ -1319,7 +1327,11 @@ proc run_ld_link_exec_tests { targets_to
 	    # We ignore warnings since some compilers may generate
 	    # incorrect section attributes and the assembler will warn
 	    # them.
-	    ld_compile "$CC -c $CFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    if { [ string match "c++" $lang ] } {
+		ld_compile "$CXX -c $CXXFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    } else {
+		ld_compile "$CC -c $CFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    }
 
 	    # We have to use $CC to build PIE and shared library.
 	    if { [ string match "c" $lang ] } {
@@ -1411,6 +1423,7 @@ proc run_cc_link_tests { ldtests } {
     global CC
     global CXX
     global CFLAGS
+    global CXXFLAGS
 
     foreach testitem $ldtests {
 	set testname [lindex $testitem 0]
@@ -1432,7 +1445,11 @@ proc run_cc_link_tests { ldtests } {
 	    # We ignore warnings since some compilers may generate
 	    # incorrect section attributes and the assembler will warn
 	    # them.
-	    ld_compile "$CC -c $CFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    if { [ string match "c++" $lang ] } {
+		ld_compile "$CXX -c $CXXFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    } else {
+		ld_compile "$CC -c $CFLAGS $cflags" $srcdir/$subdir/$src_file $objfile
+	    }
 	}
 
 	# Clear error and warning counts.

Re: ld_compile: force CFLAGS really necessary ?

[Nick Clifton]

Hi Mike,

> 2007-09-01  Mike Frysinger  <vapier@gentoo.org>
> 
> 	* lib/ld-lib.exp (default_ld_compile): Pull in global CXXFLAGS and
> 	add it to $flags when $ccexe matches *++*.
> 	(run_ld_link_exec_tests): Pull in global CXXFLAGS and execute CXX
> 	with CXXFLAGS when $lang matches c++.
> 	(run_cc_link_tests): Likewise.

Approved - please apply.

Cheers
   Nick