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
<jos...@codesourcery.com>wrote:
> On Tue, 15 Nov 2011, Han Shen wrote:
>
> > 2011-11-15 Han Shen <mailto:shen...@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:jos...@codesourcery.com
>
--
http://codereview.appspot.com/5394041/
