Patchwork [trunk] RFS: translate built-in include paths for sysroot (issue5394041)

login
register
mail settings
Submitter Han Shen
Date Nov. 16, 2011, 12:47 a.m.
Message ID <20111116004705.A872461576@shenhan.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/125903/
State New
Headers show

Comments

Han Shen - Nov. 16, 2011, 12:47 a.m.
2011-11-15   Han Shen  <shenhan@google.com>

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


--
This patch is available for review at http://codereview.appspot.com/5394041
Joseph S. Myers - Nov. 18, 2011, 4:54 p.m.
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.".)
Han Shen - Nov. 19, 2011, 12:21 a.m.
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
Joseph S. Myers - Dec. 5, 2011, 9:33 p.m.
On Fri, 18 Nov 2011, Han Shen 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.

Please make sure to follow the style of existing ChangeLog entries.  For 
Makefile.in for example it might be:

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

I went to review the change as I had it queued to look at, but found that 
this message, which I had noted as the message to look at, did not 
actually contain a patch - and I cannot find any posting of the 
configure.ac changes in any of your messages to gcc-patches since at least 
the start of November.  So it's still not ready to review.  Please post a 
complete, self-contained patch submission, that includes all of:

* the patch itself, including configure.ac changes;
* the ChangeLog entries;
* self-contained rationale;
* URLs to previous gcc-patches discussion

in the one gcc-patches message.  If you have in fact already posted such a 
submission and I've failed to locate it, could you give the gcc-patches 
URL?

Patch

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.  */