| Message ID | 20111116004705.A872461576@shenhan.mtv.corp.google.com |
|---|---|
| State | New |
| Headers | show |
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.".)
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
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?
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. */