[trunk] RFS: translate built-in include paths for sysroot

ChangeLog
        * Makefile.in (gcc_gxx_include_dir_add_sysroot): New.
          (PREPROCESSOR_DEFINES): Define GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT.

        * cppdefault.c (cpp_include_defaults): replace hard coded "1" with
          GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT for "add_sysroot" field.

        * configure.ac (AC_SUBST): Add gcc_gxx_include_dir_add_sysroot to
          control whether sysroot should be prepended to gxx include dir.

        * configure: Regenerate.

Hi, this is a follow up for issue "http://codereview.appspot.com/4641076".

The rationale for the patch copied from previous thread:
=======================================
The setup:

Configuring a toolchain targeting x86-64 GNU Linux (Ubuntu Lucid), as a
cross-compiler.  Using a sysroot to provide the Lucid headers+libraries,
with the sysroot path being within the GCC install tree.  Want to use the
Lucid system libstdc++ and headers, which means that I'm not
building/installing libstdc++-v3.

So, configuring with:
  --with-sysroot="$SYSROOT"
  --disable-libstdc++-v3 \
  --with-gxx-include-dir="$SYSROOT/usr/include/c++/4.4" \
(among other options).

Hoping to support two usage models with this configuration, w.r.t. use of
the sysroot:

(1) somebody installs the sysroot in the normal location relative to the
GCC install, and relocates the whole bundle (sysroot+GCC).  This works
great AFAICT, GCC finds its includes (including the C++ includes) thanks
to the add_standard_paths iprefix handling.

(2) somebody installs the sysroot in a non-standard location, and uses
--sysroot to try to access it.  This works fine for the C headers, but
doesn't work.

For the C headers, add_standard_paths prepends the sysroot location to
the /usr/include path (since that's what's specified in cppdefault.c for
that path).  It doesn't do the same for the C++ include path, though
(again, as specified in cppdefault.c).

add_standard_paths doesn't attempt to relocate built-in include paths that
start with the compiled-in sysroot location (e.g., the g++ include dir, in
this case).  This isn't surprising really: normally you either prepend the
sysroot location or you don't (as specified by cppdefault.c); none of the
built-in paths normally *start* with the sysroot location and need to be
relocated.  However, in this odd-ball case of trying to use the C++ headers
from the sysroot, one of the paths *does* need to be relocated in this way.
===========================

Chris(cgd@) provided a patch for the issue, but Joseph had a different opinion, my patch here is just coded as suggested.

Tested before/after on a x86_64-unknown-linux-gnu system, with
--enable-languages=all, RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}'.
No changes in test results.

[shenhan, 2011-11-16 00:47:10]

2011-11-15   Han Shen  <shenhan@google.com>

	* gcc/Makefile.in:
	* gcc/configure:
	* gcc/cppdefault.c:

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index ae4f4da..0a05783 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -615,6 +615,7 @@ gcc_tooldir = @gcc_tooldir@
 build_tooldir = $(exec_prefix)/$(target_noncanonical)
 # Directory in which the compiler finds target-independent g++ includes.
 gcc_gxx_include_dir = @gcc_gxx_include_dir@
+gcc_gxx_include_dir_add_sysroot = @gcc_gxx_include_dir_add_sysroot@
 # Directory to search for site-specific includes.
 local_includedir = $(local_prefix)/include
 includedir = $(prefix)/include
@@ -3979,6 +3980,7 @@ PREPROCESSOR_DEFINES = \
   -DGCC_INCLUDE_DIR=\"$(libsubdir)/include\" \
   -DFIXED_INCLUDE_DIR=\"$(libsubdir)/include-fixed\" \
   -DGPLUSPLUS_INCLUDE_DIR=\"$(gcc_gxx_include_dir)\" \
+  -DGPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT=$(gcc_gxx_include_dir_add_sysroot) \
  
-DGPLUSPLUS_TOOL_INCLUDE_DIR=\"$(gcc_gxx_include_dir)/$(target_noncanonical)\" \
   -DGPLUSPLUS_BACKWARD_INCLUDE_DIR=\"$(gcc_gxx_include_dir)/backward\" \
   -DLOCAL_INCLUDE_DIR=\"$(local_includedir)\" \
diff --git a/gcc/configure b/gcc/configure
index 99334ce..364d8c2 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -638,6 +638,7 @@ host_xm_include_list
 host_xm_file_list
 host_exeext
 gcc_gxx_include_dir
+gcc_gxx_include_dir_add_sysroot
 gcc_config_arguments
 float_h_file
 extra_programs
@@ -3291,12 +3292,20 @@ gcc_gxx_include_dir=
 # Specify the g++ header file directory
 
 # Check whether --with-gxx-include-dir was given.
+gcc_gxx_include_dir_add_sysroot=0
 if test "${with_gxx_include_dir+set}" = set; then :
   withval=$with_gxx_include_dir; case "${withval}" in
 yes)	as_fn_error "bad value ${withval} given for g++ include directory"
"$LINENO" 5 ;;
 no)	;;
 *)	gcc_gxx_include_dir=$with_gxx_include_dir ;;
 esac
+  if test "${with_sysroot+set}" = set; then :
+    gcc_gxx_without_sysroot=`expr "${gcc_gxx_include_dir}" :
"${with_sysroot}"'\(.*\)'`
+    if test "${gcc_gxx_without_sysroot}"; then :
+      gcc_gxx_include_dir="${gcc_gxx_without_sysroot}"
+      gcc_gxx_include_dir_add_sysroot=1
+    fi
+  fi
 fi
 
 
diff --git a/gcc/cppdefault.c b/gcc/cppdefault.c
index 099899a..e8341d5 100644
--- a/gcc/cppdefault.c
+++ b/gcc/cppdefault.c
@@ -44,15 +44,15 @@ const struct default_include cpp_include_defaults[]
 = {
 #ifdef GPLUSPLUS_INCLUDE_DIR
     /* Pick up GNU C++ generic include files.  */
-    { GPLUSPLUS_INCLUDE_DIR, "G++", 1, 1, 0, 0 },
+    { GPLUSPLUS_INCLUDE_DIR, "G++", 1, 1, GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT, 0
},
 #endif
 #ifdef GPLUSPLUS_TOOL_INCLUDE_DIR
     /* Pick up GNU C++ target-dependent include files.  */
-    { GPLUSPLUS_TOOL_INCLUDE_DIR, "G++", 1, 1, 0, 1 },
+    { GPLUSPLUS_TOOL_INCLUDE_DIR, "G++", 1, 1,
GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT, 1 },
 #endif
 #ifdef GPLUSPLUS_BACKWARD_INCLUDE_DIR
     /* Pick up GNU C++ backward and deprecated include files.  */
-    { GPLUSPLUS_BACKWARD_INCLUDE_DIR, "G++", 1, 1, 0, 0 },
+    { GPLUSPLUS_BACKWARD_INCLUDE_DIR, "G++", 1, 1,
GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT, 0 },
 #endif
 #ifdef GCC_INCLUDE_DIR
     /* This is the dir for gcc's private headers.  */

--
This patch is available for review at http://codereview.appspot.com/5394041

[shenhan, 2011-11-16 00:54:09]

On 2011/11/16 00:47:10, shenhan wrote:
> 2011-11-15   Han Shen  <mailto:shenhan@google.com>
> 
> 	* gcc/Makefile.in:
> 	* gcc/configure:
> 	* gcc/cppdefault.c:
> 
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index ae4f4da..0a05783 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -615,6 +615,7 @@ gcc_tooldir = @gcc_tooldir@
>  build_tooldir = $(exec_prefix)/$(target_noncanonical)
>  # Directory in which the compiler finds target-independent g++ includes.
>  gcc_gxx_include_dir = @gcc_gxx_include_dir@
> +gcc_gxx_include_dir_add_sysroot = @gcc_gxx_include_dir_add_sysroot@
>  # Directory to search for site-specific includes.
>  local_includedir = $(local_prefix)/include
>  includedir = $(prefix)/include
> @@ -3979,6 +3980,7 @@ PREPROCESSOR_DEFINES = \
>    -DGCC_INCLUDE_DIR=\"$(libsubdir)/include\" \
>    -DFIXED_INCLUDE_DIR=\"$(libsubdir)/include-fixed\" \
>    -DGPLUSPLUS_INCLUDE_DIR=\"$(gcc_gxx_include_dir)\" \
> +  -DGPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT=$(gcc_gxx_include_dir_add_sysroot) \
>   
> -DGPLUSPLUS_TOOL_INCLUDE_DIR=\"$(gcc_gxx_include_dir)/$(target_noncanonical)\"
\
>    -DGPLUSPLUS_BACKWARD_INCLUDE_DIR=\"$(gcc_gxx_include_dir)/backward\" \
>    -DLOCAL_INCLUDE_DIR=\"$(local_includedir)\" \
> diff --git a/gcc/configure b/gcc/configure
> index 99334ce..364d8c2 100755
> --- a/gcc/configure
> +++ b/gcc/configure
> @@ -638,6 +638,7 @@ host_xm_include_list
>  host_xm_file_list
>  host_exeext
>  gcc_gxx_include_dir
> +gcc_gxx_include_dir_add_sysroot
>  gcc_config_arguments
>  float_h_file
>  extra_programs
> @@ -3291,12 +3292,20 @@ gcc_gxx_include_dir=
>  # Specify the g++ header file directory
>  
>  # Check whether --with-gxx-include-dir was given.
> +gcc_gxx_include_dir_add_sysroot=0
>  if test "${with_gxx_include_dir+set}" = set; then :
>    withval=$with_gxx_include_dir; case "${withval}" in
>  yes)	as_fn_error "bad value ${withval} given for g++ include directory"
> "$LINENO" 5 ;;
>  no)	;;
>  *)	gcc_gxx_include_dir=$with_gxx_include_dir ;;
>  esac
> +  if test "${with_sysroot+set}" = set; then :
> +    gcc_gxx_without_sysroot=`expr "${gcc_gxx_include_dir}" :
> "${with_sysroot}"'\(.*\)'`
> +    if test "${gcc_gxx_without_sysroot}"; then :
> +      gcc_gxx_include_dir="${gcc_gxx_without_sysroot}"
> +      gcc_gxx_include_dir_add_sysroot=1
> +    fi
> +  fi
>  fi
>  
>  
> diff --git a/gcc/cppdefault.c b/gcc/cppdefault.c
> index 099899a..e8341d5 100644
> --- a/gcc/cppdefault.c
> +++ b/gcc/cppdefault.c
> @@ -44,15 +44,15 @@ const struct default_include cpp_include_defaults[]
>  = {
>  #ifdef GPLUSPLUS_INCLUDE_DIR
>      /* Pick up GNU C++ generic include files.  */
> -    { GPLUSPLUS_INCLUDE_DIR, "G++", 1, 1, 0, 0 },
> +    { GPLUSPLUS_INCLUDE_DIR, "G++", 1, 1, GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT,
0
> },
>  #endif
>  #ifdef GPLUSPLUS_TOOL_INCLUDE_DIR
>      /* Pick up GNU C++ target-dependent include files.  */
> -    { GPLUSPLUS_TOOL_INCLUDE_DIR, "G++", 1, 1, 0, 1 },
> +    { GPLUSPLUS_TOOL_INCLUDE_DIR, "G++", 1, 1,
> GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT, 1 },
>  #endif
>  #ifdef GPLUSPLUS_BACKWARD_INCLUDE_DIR
>      /* Pick up GNU C++ backward and deprecated include files.  */
> -    { GPLUSPLUS_BACKWARD_INCLUDE_DIR, "G++", 1, 1, 0, 0 },
> +    { GPLUSPLUS_BACKWARD_INCLUDE_DIR, "G++", 1, 1,
> GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT, 0 },
>  #endif
>  #ifdef GCC_INCLUDE_DIR
>      /* This is the dir for gcc's private headers.  */
> 
> --
> This patch is available for review at http://codereview.appspot.com/5394041

Hi, this is a follow up for issue "http://codereview.appspot.com/4641076".

The issue description from that issue is copied below:

=========================
The setup:

Configuring a toolchain targeting x86-64 GNU Linux (Ubuntu Lucid), as a
cross-compiler.  Using a sysroot to provide the Lucid headers+libraries,
with the sysroot path being within the GCC install tree.  Want to use the
Lucid system libstdc++ and headers, which means that I'm not
building/installing libstdc++-v3.

So, configuring with:
  --with-sysroot="$SYSROOT"
  --disable-libstdc++-v3 \
  --with-gxx-include-dir="$SYSROOT/usr/include/c++/4.4" \
(among other options).

Hoping to support two usage models with this configuration, w.r.t. use of
the sysroot:

(1) somebody installs the sysroot in the normal location relative to the
GCC install, and relocates the whole bundle (sysroot+GCC).  This works
great AFAICT, GCC finds its includes (including the C++ includes) thanks
to the add_standard_paths iprefix handling.

(2) somebody installs the sysroot in a non-standard location, and uses
--sysroot to try to access it.  This works fine for the C headers, but
doesn't work.

For the C headers, add_standard_paths prepends the sysroot location to
the /usr/include path (since that's what's specified in cppdefault.c for
that path).  It doesn't do the same for the C++ include path, though
(again, as specified in cppdefault.c).

add_standard_paths doesn't attempt to relocate built-in include paths that
start with the compiled-in sysroot location (e.g., the g++ include dir, in
this case).  This isn't surprising really: normally you either prepend the
sysroot location or you don't (as specified by cppdefault.c); none of the
built-in paths normally *start* with the sysroot location and need to be
relocated.  However, in this odd-ball case of trying to use the C++ headers
from the sysroot, one of the paths *does* need to be relocated in this way.

========================

Chris(cgd@) provided a patch for the issue, but Joseph had a different opinion,
my patch here is just coded as suggested.

[joseph_codesourcery.com, 2011-11-18 16:54:35]

On Tue, 15 Nov 2011, Han Shen wrote:

> 2011-11-15   Han Shen  <shenhan@google.com>
> 
> 	* gcc/Makefile.in:
> 	* gcc/configure:
> 	* gcc/cppdefault.c:

You need to include the full ChangeLog entries with your patch.  Note that 
paths in ChangeLogs are relative to the directory with the ChangeLog, so 
no gcc/ in this case.

Please also include the full rationale with your patch *and URLs for the 
previous discussion* (from June, it seems) so that reviewers don't need to 
track that down themselves.

> diff --git a/gcc/configure b/gcc/configure
> index 99334ce..364d8c2 100755
> --- a/gcc/configure
> +++ b/gcc/configure

It's not possible to review this patch as is because you've only included 
the changes to the generated file configure, not to its source file 
configure.ac.  Please make sure that your resubmission includes all the 
source file changes.  (There is no need to include the changes to the 
generated file configure at all in the submission; the ChangeLog entry can 
just mention it as "* configure: Regenerate.".)

-- 
Joseph S. Myers
joseph@codesourcery.com

[shenhan, 2011-11-19 00:10:20]

Hi, Joseph, thanks!

ChangeLog entries added to the issue description.

ChangeLog
        * Makefile.in (GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT): add a macro
        definition to compile command.
        * cppdefault.c (GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT): replace hard
        coded "add_sysroot" field with the control macro.
        * configure.ac (gcc_gxx_include_dir_add_sysroot): add a flag
        variable to control whether sysroot should be prepended to gxx
        include dir.
        * configure : Regenerate.

Yeah, I should include the URL for the previous discussion in the mail. (I
previously put it in the issue description together with the rationale.)

This is a follow up for issue - http://codereview.appspot.com/4641076.
The rationale for this patch is also copied here:
==========

The setup:

Configuring a toolchain targeting x86-64 GNU Linux (Ubuntu Lucid), as a
cross-compiler.  Using a sysroot to provide the Lucid headers+libraries,
with the sysroot path being within the GCC install tree.  Want to use the
Lucid system libstdc++ and headers, which means that I'm not
building/installing libstdc++-v3.

So, configuring with:
  --with-sysroot="$SYSROOT"
  --disable-libstdc++-v3 \
  --with-gxx-include-dir="$SYSROOT/usr/include/c++/4.4" \
(among other options).

Hoping to support two usage models with this configuration, w.r.t. use of
the sysroot:

(1) somebody installs the sysroot in the normal location relative to the
GCC install, and relocates the whole bundle (sysroot+GCC).  This works
great AFAICT, GCC finds its includes (including the C++ includes) thanks
to the add_standard_paths iprefix handling.

(2) somebody installs the sysroot in a non-standard location, and uses
--sysroot to try to access it.  This works fine for the C headers, but
doesn't work.

For the C headers, add_standard_paths prepends the sysroot location to
the /usr/include path (since that's what's specified in cppdefault.c for
that path).  It doesn't do the same for the C++ include path, though
(again, as specified in cppdefault.c).

add_standard_paths doesn't attempt to relocate built-in include paths that
start with the compiled-in sysroot location (e.g., the g++ include dir, in
this case).  This isn't surprising really: normally you either prepend the
sysroot location or you don't (as specified by cppdefault.c); none of the
built-in paths normally *start* with the sysroot location and need to be
relocated.  However, in this odd-ball case of trying to use the C++ headers
from the sysroot, one of the paths *does* need to be relocated in this way.
=========


Chris(cgd@) provided a patch for the issue, but there was a different opinion,
my patch here is just coded as suggested.

Tested before/after on a x86_64-unknown-linux-gnu system, with
--enable-languages=all, RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}'.
No changes in test results.

-Han


On Fri, Nov 18, 2011 at 8:54 AM, Joseph S. Myers <joseph@codesourcery.com>wrote:

> On Tue, 15 Nov 2011, Han Shen wrote:
>
> > 2011-11-15   Han Shen  <shenhan@google.com>
> >
> >       * gcc/Makefile.in:
> >       * gcc/configure:
> >       * gcc/cppdefault.c:
>
> You need to include the full ChangeLog entries with your patch.  Note that
> paths in ChangeLogs are relative to the directory with the ChangeLog, so
> no gcc/ in this case.
>
> Please also include the full rationale with your patch *and URLs for the
> previous discussion* (from June, it seems) so that reviewers don't need to
> track that down themselves.
>
> > diff --git a/gcc/configure b/gcc/configure
> > index 99334ce..364d8c2 100755
> > --- a/gcc/configure
> > +++ b/gcc/configure
>
> It's not possible to review this patch as is because you've only included
> the changes to the generated file configure, not to its source file
> configure.ac.  Please make sure that your resubmission includes all the
> source file changes.  (There is no need to include the changes to the
> generated file configure at all in the submission; the ChangeLog entry can
> just mention it as "* configure: Regenerate.".)
>
> --
> Joseph S. Myers
> joseph@codesourcery.com
>



-- 
 Han Shen |  Software Engineer |  shenhan@google.com |  +1-650-440-3330

[shenhan, 2011-11-28 22:48:22]

Hi, Joseph, get a chance to take a look? Thanks!
-Han

On 2011/11/19 00:10:20, shenhan wrote:
> Hi, Joseph, thanks!
> 
> ChangeLog entries added to the issue description.
> 
> ChangeLog
>         * Makefile.in (GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT): add a macro
>         definition to compile command.
>         * cppdefault.c (GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT): replace hard
>         coded "add_sysroot" field with the control macro.
>         * configure.ac (gcc_gxx_include_dir_add_sysroot): add a flag
>         variable to control whether sysroot should be prepended to gxx
>         include dir.
>         * configure : Regenerate.
> 
> Yeah, I should include the URL for the previous discussion in the mail. (I
> previously put it in the issue description together with the rationale.)
> 
> This is a follow up for issue - http://codereview.appspot.com/4641076.
> The rationale for this patch is also copied here:
> ==========
> 
> The setup:
> 
> Configuring a toolchain targeting x86-64 GNU Linux (Ubuntu Lucid), as a
> cross-compiler.  Using a sysroot to provide the Lucid headers+libraries,
> with the sysroot path being within the GCC install tree.  Want to use the
> Lucid system libstdc++ and headers, which means that I'm not
> building/installing libstdc++-v3.
> 
> So, configuring with:
>   --with-sysroot="$SYSROOT"
>   --disable-libstdc++-v3 \
>   --with-gxx-include-dir="$SYSROOT/usr/include/c++/4.4" \
> (among other options).
> 
> Hoping to support two usage models with this configuration, w.r.t. use of
> the sysroot:
> 
> (1) somebody installs the sysroot in the normal location relative to the
> GCC install, and relocates the whole bundle (sysroot+GCC).  This works
> great AFAICT, GCC finds its includes (including the C++ includes) thanks
> to the add_standard_paths iprefix handling.
> 
> (2) somebody installs the sysroot in a non-standard location, and uses
> --sysroot to try to access it.  This works fine for the C headers, but
> doesn't work.
> 
> For the C headers, add_standard_paths prepends the sysroot location to
> the /usr/include path (since that's what's specified in cppdefault.c for
> that path).  It doesn't do the same for the C++ include path, though
> (again, as specified in cppdefault.c).
> 
> add_standard_paths doesn't attempt to relocate built-in include paths that
> start with the compiled-in sysroot location (e.g., the g++ include dir, in
> this case).  This isn't surprising really: normally you either prepend the
> sysroot location or you don't (as specified by cppdefault.c); none of the
> built-in paths normally *start* with the sysroot location and need to be
> relocated.  However, in this odd-ball case of trying to use the C++ headers
> from the sysroot, one of the paths *does* need to be relocated in this way.
> =========
> 
> 
> Chris(cgd@) provided a patch for the issue, but there was a different opinion,
> my patch here is just coded as suggested.
> 
> Tested before/after on a x86_64-unknown-linux-gnu system, with
> --enable-languages=all, RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}'.
> No changes in test results.
> 
> -Han
> 
> 
> On Fri, Nov 18, 2011 at 8:54 AM, Joseph S. Myers
<joseph@codesourcery.com>wrote:
> 
> > On Tue, 15 Nov 2011, Han Shen wrote:
> >
> > > 2011-11-15   Han Shen  <mailto:shenhan@google.com>
> > >
> > >       * gcc/Makefile.in:
> > >       * gcc/configure:
> > >       * gcc/cppdefault.c:
> >
> > You need to include the full ChangeLog entries with your patch.  Note that
> > paths in ChangeLogs are relative to the directory with the ChangeLog, so
> > no gcc/ in this case.
> >
> > Please also include the full rationale with your patch *and URLs for the
> > previous discussion* (from June, it seems) so that reviewers don't need to
> > track that down themselves.
> >
> > > diff --git a/gcc/configure b/gcc/configure
> > > index 99334ce..364d8c2 100755
> > > --- a/gcc/configure
> > > +++ b/gcc/configure
> >
> > It's not possible to review this patch as is because you've only included
> > the changes to the generated file configure, not to its source file
> > configure.ac.  Please make sure that your resubmission includes all the
> > source file changes.  (There is no need to include the changes to the
> > generated file configure at all in the submission; the ChangeLog entry can
> > just mention it as "* configure: Regenerate.".)
> >
> > --
> > Joseph S. Myers
> > mailto:joseph@codesourcery.com
> >
> 
> 
> 
> --

[shenhan, 2011-12-05 18:12:36]

Ping?