Patchwork re-expand CXX_FOR_TARGET after libstdc++-v3 is configured

login
register
mail settings
Submitter Alexandre Oliva
Date Nov. 13, 2010, 7:19 p.m.
Message ID <or8w0x116g.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/71070/
State New
Headers show

Comments

Alexandre Oliva - Nov. 13, 2010, 7:19 p.m.
I got a report from RTH and Aldy that libitm (in the
transactional-memory branch) would fail to build at first, but once you
retried, it would succeed.  What's new about libitm is that it expects
CXX to be a functional C++ compiler, with usual C++ headers in place.
Other libraries that require a C++ compiler don't seem to depend on the
C++ headers.

It turns out the problem is that CXX_FOR_TARGET is shell-evaluated and
passed down to sub-makes before libstdc++-v3 is configured, but it can
only get the right -I flags after libstdc++-v3 is configured.  Thus the
first build fails, but the subsequent one overrides the incorrect CXX
settings passed to libitm and it works out in the end.

This patch changes our handling of CXX_FOR_TARGET in two ways:

- before libstdc++-v3 is configured, we add an invalid flag
  (-funconfigured-libstdc++-v3) to CXX_FOR_TARGET, to make sure it isn't
  used (libstdc++-v3 and libjava use RAW_CXX_FOR_TARGET)

- we do *not* pass CXX_FOR_TARGET to a sub-make if its shell expansion
  contains -funconfigured-, so that the Makefile definition prevails and
  gets expanded at every use within the sub-make.  I considered a
  cheaper make test rather than a $(shell) expansion, but it doesn't
  have the desired effect: only the shell knows how to evaluate the
  backticks in CXX_FOR_TARGET, so we end up not ever passing down an
  expansion, because the macro definition contains the -funconfigured-*
  word within backticks.  I guess it would be possible to make it a make
  test, testing for the backticks too, but it's not as robust, and
  probably not worth it.


Here's the patch.  I'll install it soon if there aren't any objections.
Tested on x86_64-linux-gnu.
Ralf Wildenhues - Nov. 14, 2010, 2:42 p.m.
Hi Alexandre,

* Alexandre Oliva wrote on Sat, Nov 13, 2010 at 08:19:35PM CET:
> I got a report from RTH and Aldy that libitm (in the
> transactional-memory branch) would fail to build at first, but once you
> retried, it would succeed.  What's new about libitm is that it expects
> CXX to be a functional C++ compiler, with usual C++ headers in place.
> Other libraries that require a C++ compiler don't seem to depend on the
> C++ headers.
> 
> It turns out the problem is that CXX_FOR_TARGET is shell-evaluated and
> passed down to sub-makes before libstdc++-v3 is configured, but it can
> only get the right -I flags after libstdc++-v3 is configured.  Thus the
> first build fails, but the subsequent one overrides the incorrect CXX
> settings passed to libitm and it works out in the end.
> 
> This patch changes our handling of CXX_FOR_TARGET in two ways:
> 
> - before libstdc++-v3 is configured, we add an invalid flag
>   (-funconfigured-libstdc++-v3) to CXX_FOR_TARGET, to make sure it isn't
>   used (libstdc++-v3 and libjava use RAW_CXX_FOR_TARGET)

I'm not really sure I understand the problem completely, but to me it
sounds like
- there is a dependency problem at the toplevel (missing dependency of
  configure-target-libitm on all-target-libstdc++-v3 for example), and
- variables that, at the toplevel, evaluate differently upon a make
  rerun will cause config.cache conflicts in the lower levels.

I'm not claiming that your patch is wrong, but for proper operation,
AFAICS there should be a static stateless map from configure-target-* to
the value of variables passed to that configure script.

> - we do *not* pass CXX_FOR_TARGET to a sub-make if its shell expansion
>   contains -funconfigured-, so that the Makefile definition prevails and
>   gets expanded at every use within the sub-make.  I considered a
>   cheaper make test rather than a $(shell) expansion, but it doesn't
>   have the desired effect: only the shell knows how to evaluate the
>   backticks in CXX_FOR_TARGET, so we end up not ever passing down an
>   expansion, because the macro definition contains the -funconfigured-*
>   word within backticks.  I guess it would be possible to make it a make
>   test, testing for the backticks too, but it's not as robust, and
>   probably not worth it.

Wasn't toplevel supposed to be buildable with non-GNU make as well
(given that binutils and some other parts of src aim to be buildable
without GNU make)?  Again, it might well be that it still is even with
this $(shell ...) expansion as long as make doesn't actually have to do
the expansion if not building GCC, but it would be good to try it out
(several GNU/Linux distros ship some BSD make under the names pmake or
freebsd-make, if you need an easy way to test).

Thanks,
Ralf
Alexandre Oliva - Nov. 14, 2010, 6:43 p.m.
On Nov 14, 2010, Ralf Wildenhues <Ralf.Wildenhues@gmx.de> wrote:

> - there is a dependency problem at the toplevel (missing dependency of
>   configure-target-libitm on all-target-libstdc++-v3 for example), and

No, no, totally different problem.  The deps are just fine.  The problem
is that the rule for “all” calls a sub-make, passing it a CXX_FOR_TARGET
evaluated *before* libstdc++-v3 is configured, so the solution is to
delay the expansion of CXX_FOR_TARGET.

> - variables that, at the toplevel, evaluate differently upon a make
>   rerun will cause config.cache conflicts in the lower levels.

That was true before the patch, but it wasn't the issue.

> Wasn't toplevel supposed to be buildable with non-GNU make as well

Yep, except when building GCC.  That's why the $(shell) bit is limited
to an @if conditional that's only passed on to make when we're building
GCC.
Paolo Bonzini - Nov. 14, 2010, 7:06 p.m.
On 11/13/2010 08:19 PM, Alexandre Oliva wrote:
> The problem is that the rule for “all” calls a sub-make, passing it a
> CXX_FOR_TARGET evaluated *before* libstdc++-v3 is configured, so the
> solution is to delay the expansion of CXX_FOR_TARGET.

Maybe the problem is this:

# We leave this in just in case, but it is not needed anymore.
RECURSE_FLAGS_TO_PASS = $(BASE_FLAGS_TO_PASS)
MAKEOVERRIDES=

We could try removing both of these and see what happens...

Paolo
Alexandre Oliva - Nov. 17, 2010, 6:55 a.m.
On Nov 14, 2010, Paolo Bonzini <bonzini@gnu.org> wrote:

> On 11/13/2010 08:19 PM, Alexandre Oliva wrote:
>> The problem is that the rule for “all” calls a sub-make, passing it a
>> CXX_FOR_TARGET evaluated *before* libstdc++-v3 is configured, so the
>> solution is to delay the expansion of CXX_FOR_TARGET.

> Maybe the problem is this:

> # We leave this in just in case, but it is not needed anymore.
> RECURSE_FLAGS_TO_PASS = $(BASE_FLAGS_TO_PASS)
> MAKEOVERRIDES=

> We could try removing both of these and see what happens...

That won't be enough.  We explicitly pass down TARGET_FLAGS_TO_PASS
(including CXX_FOR_TARGET) when recursing for all-host all-target.  And
it's not like it would be right to refrain from overriding a number of
variables, CXX_FOR_TARGET included: we want to use different compilers
and options for different stages, rather than taking whatever overrider
the user might have passed for stage1 or so.
Paolo Bonzini - Nov. 17, 2010, 9:23 a.m.
On 11/17/2010 07:55 AM, Alexandre Oliva wrote:
> On Nov 14, 2010, Paolo Bonzini<bonzini@gnu.org>  wrote:
>
>> On 11/13/2010 08:19 PM, Alexandre Oliva wrote:
>>> The problem is that the rule for “all” calls a sub-make, passing it a
>>> CXX_FOR_TARGET evaluated *before* libstdc++-v3 is configured, so the
>>> solution is to delay the expansion of CXX_FOR_TARGET.
>
>> Maybe the problem is this:
>
>> # We leave this in just in case, but it is not needed anymore.
>> RECURSE_FLAGS_TO_PASS = $(BASE_FLAGS_TO_PASS)
>> MAKEOVERRIDES=
>
>> We could try removing both of these and see what happens...
>
> That won't be enough.  We explicitly pass down TARGET_FLAGS_TO_PASS
> (including CXX_FOR_TARGET) when recursing for all-host all-target.  And
> it's not like it would be right to refrain from overriding a number of
> variables, CXX_FOR_TARGET included: we want to use different compilers
> and options for different stages, rather than taking whatever overrider
> the user might have passed for stage1 or so.

I see.

Paolo

Patch

for gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* configure.ac (CXX_FOR_TARGET): Add -funconfigured-libstdc++-v3.
	* Makefile.def (CXX_FOR_TARGET): Removed from flags_to_pass.
	* Makefile.tpl (CXX_FOR_TARGET_FLAG_TO_PASS): New.
	(BASE_FLAGS_TO_PASS): Use it.
	* configure: Rebuilt.
	* Makefile.in: Rebuilt.
	
Index: Makefile.def
===================================================================
--- Makefile.def.orig	2010-11-13 15:46:42.995359300 -0200
+++ Makefile.def	2010-11-13 15:46:48.553299990 -0200
@@ -280,7 +280,6 @@  flags_to_pass = { flag= AS_FOR_TARGET ; 
 flags_to_pass = { flag= CC_FOR_TARGET ; };
 flags_to_pass = { flag= CFLAGS_FOR_TARGET ; };
 flags_to_pass = { flag= CPPFLAGS_FOR_TARGET ; };
-flags_to_pass = { flag= CXX_FOR_TARGET ; };
 flags_to_pass = { flag= CXXFLAGS_FOR_TARGET ; };
 flags_to_pass = { flag= DLLTOOL_FOR_TARGET ; };
 flags_to_pass = { flag= FLAGS_FOR_TARGET ; };
Index: Makefile.in
===================================================================
--- Makefile.in.orig	2010-11-13 15:46:43.095358229 -0200
+++ Makefile.in	2010-11-13 15:46:48.981295850 -0200
@@ -632,6 +632,26 @@  HOST_LIB_PATH_libelf = \
 @endif libelf
 
 
+CXX_FOR_TARGET_FLAG_TO_PASS = \
+	"CXX_FOR_TARGET=$(CXX_FOR_TARGET)"
+@if target-libstdc++-v3
+# CXX_FOR_TARGET is tricky to get right for target libs that require a
+# functional C++ compiler.  When we recurse, if we expand
+# CXX_FOR_TARGET before configuring libstdc++-v3, we won't get
+# libstdc++ include flags from the script.  Instead, we get an
+# -funconfigured-* word, so that we'll get errors if this invalid C++
+# command line is used for anything, but also so that we can use the
+# word to decide whether or not to pass on this CXX_FOR_TARGET.  If we
+# don't pass it on, sub-make will use the default definition, that
+# re-expands it at the time of use, so we'll get it right when we need
+# it.  One potential exception is the expansion of CXX_FOR_TARGET
+# passed down as part of CXX within TARGET_FLAGS, but this wouldn't
+# really work, for C++ host programs can't depend on the current-stage
+# C++ target library.
+CXX_FOR_TARGET_FLAG_TO_PASS = \
+	$(shell if echo "$(CXX_FOR_TARGET)" | grep " -funconfigured-" > /dev/null; then :; else echo '"CXX_FOR_TARGET=$(CXX_FOR_TARGET)"'; fi)
+@endif target-libstdc++-v3
+
 # Flags to pass down to all sub-makes.
 BASE_FLAGS_TO_PASS = \
 	"DESTDIR=$(DESTDIR)" \
@@ -699,7 +719,6 @@  BASE_FLAGS_TO_PASS = \
 	"CC_FOR_TARGET=$(CC_FOR_TARGET)" \
 	"CFLAGS_FOR_TARGET=$(CFLAGS_FOR_TARGET)" \
 	"CPPFLAGS_FOR_TARGET=$(CPPFLAGS_FOR_TARGET)" \
-	"CXX_FOR_TARGET=$(CXX_FOR_TARGET)" \
 	"CXXFLAGS_FOR_TARGET=$(CXXFLAGS_FOR_TARGET)" \
 	"DLLTOOL_FOR_TARGET=$(DLLTOOL_FOR_TARGET)" \
 	"FLAGS_FOR_TARGET=$(FLAGS_FOR_TARGET)" \
@@ -737,6 +756,7 @@  BASE_FLAGS_TO_PASS = \
 	"STAGEfeedback_CFLAGS=$(STAGEfeedback_CFLAGS)" \
 	"STAGEfeedback_CXXFLAGS=$(STAGEfeedback_CXXFLAGS)" \
 	"STAGEfeedback_TFLAGS=$(STAGEfeedback_TFLAGS)" \
+	$(CXX_FOR_TARGET_FLAG_TO_PASS) \
 	"TFLAGS=$(TFLAGS)" \
 	"CONFIG_SHELL=$(SHELL)" \
 	"MAKEINFO=$(MAKEINFO) $(MAKEINFOFLAGS)" 
Index: Makefile.tpl
===================================================================
--- Makefile.tpl.orig	2010-11-13 15:46:43.514353755 -0200
+++ Makefile.tpl	2010-11-13 15:46:49.604288725 -0200
@@ -543,6 +543,26 @@  HOST_LIB_PATH_[+module+] = \
 @endif [+module+]
 [+ ENDIF lib_path +][+ ENDFOR host_modules +]
 
+CXX_FOR_TARGET_FLAG_TO_PASS = \
+	"CXX_FOR_TARGET=$(CXX_FOR_TARGET)"
+@if target-libstdc++-v3
+# CXX_FOR_TARGET is tricky to get right for target libs that require a
+# functional C++ compiler.  When we recurse, if we expand
+# CXX_FOR_TARGET before configuring libstdc++-v3, we won't get
+# libstdc++ include flags from the script.  Instead, we get an
+# -funconfigured-* word, so that we'll get errors if this invalid C++
+# command line is used for anything, but also so that we can use the
+# word to decide whether or not to pass on this CXX_FOR_TARGET.  If we
+# don't pass it on, sub-make will use the default definition, that
+# re-expands it at the time of use, so we'll get it right when we need
+# it.  One potential exception is the expansion of CXX_FOR_TARGET
+# passed down as part of CXX within TARGET_FLAGS, but this wouldn't
+# really work, for C++ host programs can't depend on the current-stage
+# C++ target library.
+CXX_FOR_TARGET_FLAG_TO_PASS = \
+	$(shell if echo "$(CXX_FOR_TARGET)" | grep " -funconfigured-" > /dev/null; then :; else echo '"CXX_FOR_TARGET=$(CXX_FOR_TARGET)"'; fi)
+@endif target-libstdc++-v3
+
 # Flags to pass down to all sub-makes.
 BASE_FLAGS_TO_PASS =[+ FOR flags_to_pass +][+ IF optional +] \
 	"`echo '[+flag+]=$([+flag+])' | sed -e s'/[^=][^=]*=$$/XFOO=/'`"[+ ELSE optional +] \
@@ -550,6 +570,7 @@  BASE_FLAGS_TO_PASS =[+ FOR flags_to_pass
 	"STAGE[+id+]_CFLAGS=$(STAGE[+id+]_CFLAGS)" \
 	"STAGE[+id+]_CXXFLAGS=$(STAGE[+id+]_CXXFLAGS)" \
 	"STAGE[+id+]_TFLAGS=$(STAGE[+id+]_TFLAGS)"[+ ENDFOR bootstrap-stage +] \
+	$(CXX_FOR_TARGET_FLAG_TO_PASS) \
 	"TFLAGS=$(TFLAGS)" \
 	"CONFIG_SHELL=$(SHELL)" \
 	"MAKEINFO=$(MAKEINFO) $(MAKEINFOFLAGS)" 
Index: configure
===================================================================
--- configure.orig	2010-11-13 15:46:43.387355189 -0200
+++ configure	2010-11-13 15:46:49.762287562 -0200
@@ -13119,7 +13119,7 @@  else
   esac
   if test $ok = yes; then
     # An in-tree tool is available and we can use it
-    CXX_FOR_TARGET='$$r/$(HOST_SUBDIR)/gcc/g++ -B$$r/$(HOST_SUBDIR)/gcc/ -nostdinc++ `test ! -f $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags || $(SHELL) $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags --build-includes` -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src/.libs'
+    CXX_FOR_TARGET='$$r/$(HOST_SUBDIR)/gcc/g++ -B$$r/$(HOST_SUBDIR)/gcc/ -nostdinc++ `if test -f $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags; then $(SHELL) $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags --build-includes; else echo -funconfigured-libstdc++-v3 ; fi` -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src/.libs'
     { $as_echo "$as_me:${as_lineno-$LINENO}: result: just compiled" >&5
 $as_echo "just compiled" >&6; }
   elif expr "x$CXX_FOR_TARGET" : "x/" > /dev/null; then
Index: configure.ac
===================================================================
--- configure.ac.orig	2010-11-13 15:46:43.244356651 -0200
+++ configure.ac	2010-11-13 15:46:49.937285702 -0200
@@ -3162,7 +3162,7 @@  GCC_TARGET_TOOL(ar, AR_FOR_TARGET, AR, [
 GCC_TARGET_TOOL(as, AS_FOR_TARGET, AS, [gas/as-new])
 GCC_TARGET_TOOL(cc, CC_FOR_TARGET, CC, [gcc/xgcc -B$$r/$(HOST_SUBDIR)/gcc/])
 GCC_TARGET_TOOL(c++, CXX_FOR_TARGET, CXX,
-		[gcc/g++ -B$$r/$(HOST_SUBDIR)/gcc/ -nostdinc++ `test ! -f $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags || $(SHELL) $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags --build-includes` -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src/.libs],
+		[gcc/g++ -B$$r/$(HOST_SUBDIR)/gcc/ -nostdinc++ `if test -f $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags; then $(SHELL) $$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags --build-includes; else echo -funconfigured-libstdc++-v3 ; fi` -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src/.libs],dnl see comments for CXX_FOR_TARGET_FLAG_TO_PASS
 		c++)
 GCC_TARGET_TOOL(c++ for libstdc++, RAW_CXX_FOR_TARGET, CXX,
 		[gcc/xgcc -shared-libgcc -B$$r/$(HOST_SUBDIR)/gcc -nostdinc++ -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src/.libs],