| Message ID | 20120831122044.49785A0E72@xoom.chi.corp.google.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 31, 2012 at 7:20 AM, Simon Baldwin <simonb@google.com> wrote: > Add a configure option to disable system header canonicalizations. > > Libcpp may canonicalize system header paths with lrealpath() for diagnostics, > dependency output, and similar. If gcc is held in a symlink farm the > canonicalized paths may be meaningless to users, and will also conflict with > build frameworks that (for example) disallow absolute paths to header files. > > Tested with bootstrap builds of C and C++, both with and without configure > --disable-canonical-system-headers. Okay for google/integration? Seems like a reasonable candidate for trunk, and I'd rather have fewer patches in google/integration than more. Can you send a copy of this patch for inclusion there? Let's at least see what people say. Ollie
On 31 August 2012 16:31, Ollie Wild <aaw@google.com> wrote: > > On Fri, Aug 31, 2012 at 7:20 AM, Simon Baldwin <simonb@google.com> wrote: > > Add a configure option to disable system header canonicalizations. > > > > Libcpp may canonicalize system header paths with lrealpath() for > > diagnostics, > > dependency output, and similar. If gcc is held in a symlink farm the > > canonicalized paths may be meaningless to users, and will also conflict > > with > > build frameworks that (for example) disallow absolute paths to header > > files. > > > > Tested with bootstrap builds of C and C++, both with and without > > configure > > --disable-canonical-system-headers. Okay for google/integration? > > Seems like a reasonable candidate for trunk, and I'd rather have fewer > patches in google/integration than more. Can you send a copy of this > patch for inclusion there? Let's at least see what people say. The patch exactly meets the definition of google/integration only, which is that it fixes up something that affects only Google's use of gcc. --no-canonical-prefixes is similar. That too is only in our branches and not in trunk, for the same reason. I'd rather keep this out of trunk unless there are known external use cases where it's beneficial. That keeps both the review and the testing load to acceptable -- though still extremely high -- levels. -- Google UK Limited | Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ | Registered in England Number: 3977902
On Fri, Aug 31, 2012 at 10:01 AM, Simon Baldwin <simonb@google.com> wrote: > On 31 August 2012 16:31, Ollie Wild <aaw@google.com> wrote: >> > > The patch exactly meets the definition of google/integration only, > which is that it fixes up something that affects only Google's use of > gcc. The criterion is more subtle than that. The google/integration branch is for things which: (a) cannot be submitted to trunk, and (b) are required for inter-operability with our build/test systems. The goal is to keep any changes relative to trunk as minimal as possible, and frankly, much of the stuff that's there now should be cleaned up and submitted upstream. > --no-canonical-prefixes is similar. That too is only in our > branches and not in trunk, for the same reason. But -no-canonical-prefixes *is* in trunk. Presumably the same people who benefit from that will also benefit from this. In fact, I think a reasonable case could be made that header canonicalization should be gated on the same flag. > > I'd rather keep this out of trunk unless there are known external use > cases where it's beneficial. That keeps both the review and the > testing load to acceptable -- though still extremely high -- levels. The same argument could be made about *any* patch we submit. Pushing upstream is always more work, but if we don't do it, we end up paying for it later. Ollie
On 31 August 2012 17:25, Ollie Wild <aaw@google.com> wrote: > > On Fri, Aug 31, 2012 at 10:01 AM, Simon Baldwin <simonb@google.com> wrote: > > On 31 August 2012 16:31, Ollie Wild <aaw@google.com> wrote: > >> > > > > The patch exactly meets the definition of google/integration only, > > which is that it fixes up something that affects only Google's use of > > gcc. > > The criterion is more subtle than that. The google/integration branch > is for things which: (a) cannot be submitted to trunk, and (b) are > required for inter-operability with our build/test systems. The goal > is to keep any changes relative to trunk as minimal as possible, and > frankly, much of the stuff that's there now should be cleaned up and > submitted upstream. > > > --no-canonical-prefixes is similar. That too is only in our > > branches and not in trunk, for the same reason. > > But -no-canonical-prefixes *is* in trunk. Presumably the same people > who benefit from that will also benefit from this. In fact, I think a > reasonable case could be made that header canonicalization should be > gated on the same flag. Yes. I meant --disable-canonical-prefixes. That is a gcc configure flag that we use to control the default setting for -[no-]canonical-prefixes where neither flag is supplied on the gcc command line. --disable/enable-canonical-prefixes is only in google branches. > > > > > I'd rather keep this out of trunk unless there are known external use > > cases where it's beneficial. That keeps both the review and the > > testing load to acceptable -- though still extremely high -- levels. > > The same argument could be made about *any* patch we submit. Pushing > upstream is always more work, but if we don't do it, we end up paying > for it later. Not at all. An ICE that only occurred with Google code as gcc input is a perfect counterexample. -- Google UK Limited | Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ | Registered in England Number: 3977902
On Fri, Aug 31, 2012 at 10:30 AM, Simon Baldwin <simonb@google.com> wrote: > > Yes. I meant --disable-canonical-prefixes. That is a gcc configure > flag that we use to control the default setting for > -[no-]canonical-prefixes where neither flag is supplied on the gcc > command line. --disable/enable-canonical-prefixes is only in google > branches. I did a little archaeology. AFAICT, there was no specific objection to pushing --disable-canonical-prefixes into upstream trunk. The feedback I see to your initial post was "send us a trunk-based patch" and "here are some minor nits to cleanup." It basically sounds like upstream was neutral to the patch and would probably accept it if we actually sent something for review. I still think this is something that is both reasonable and feasible to push upstream. We should at least try to get some feedback first. While there aren't a lot of people using symlink farms, I'd be surprised if we were the only ones. Ollie
Index: gcc/doc/install.texi =================================================================== --- gcc/doc/install.texi (revision 190830) +++ gcc/doc/install.texi (working copy) @@ -1747,6 +1747,14 @@ and may be disabled using @option{--disa See @option{-canonical-prefixes} or @option{-no-canonical-prefixes} for more details, including how to override this configuration option when compiling. + +@item --enable-canonical-system-headers +@itemx --disable-canonical-system-headers +Enable system header path canonicalization for @file{libcpp}. This can +produce shorter header file paths in diagnostics and dependency output +files, but these changed header paths may conflict with some compilation +environments. Enabled by default, and may be disabled using +@option{--disable-canonical-system-headers}. @end table @subheading Cross-Compiler-Specific Options Index: libcpp/configure =================================================================== --- libcpp/configure (revision 190830) +++ libcpp/configure (working copy) @@ -703,6 +703,7 @@ enable_rpath with_libiconv_prefix enable_maintainer_mode enable_checking +enable_canonical_system_headers ' ac_precious_vars='build_alias host_alias @@ -1337,6 +1338,8 @@ Optional Features: --disable-rpath do not hardcode runtime library paths --enable-maintainer-mode enable rules only needed by maintainers --enable-checking enable expensive run-time checks + --enable-canonical-system-headers + enable or disable system headers canonicalization Optional Packages: --with-PACKAGE[=ARG] use PACKAGE [ARG=yes] @@ -7366,6 +7369,19 @@ $as_echo "#define ENABLE_CHECKING 1" >>c fi +# Check whether --enable-canonical-system-headers was given. +if test "${enable_canonical_system_headers+set}" = set; then : + enableval=$enable_canonical_system_headers; +else + enable_canonical_system_headers=yes +fi + +if test $enable_canonical_system_headers != no; then + +$as_echo "#define ENABLE_CANONICAL_SYSTEM_HEADERS 1" >>confdefs.h + +fi + case $target in alpha*-*-* | \ Index: libcpp/files.c =================================================================== --- libcpp/files.c (revision 190830) +++ libcpp/files.c (working copy) @@ -345,6 +345,7 @@ pch_open_file (cpp_reader *pfile, _cpp_f shorter, otherwise return NULL. This function does NOT free the memory pointed by FILE. */ +#ifdef ENABLE_CANONICAL_SYSTEM_HEADERS static char * maybe_shorter_path (const char * file) { @@ -359,6 +360,7 @@ maybe_shorter_path (const char * file) return NULL; } } +#endif /* Try to open the path FILE->name appended to FILE->dir. This is where remap and PCH intercept the file lookup process. Return true @@ -384,6 +386,7 @@ find_file_in_dir (cpp_reader *pfile, _cp char *copy; void **pp; +#ifdef ENABLE_CANONICAL_SYSTEM_HEADERS /* We try to canonicalize system headers. */ if (file->dir->sysp) { @@ -396,6 +399,7 @@ find_file_in_dir (cpp_reader *pfile, _cp path = canonical_path; } } +#endif hv = htab_hash_string (path); if (htab_find_with_hash (pfile->nonexistent_file_hash, path, hv) != NULL) Index: libcpp/configure.ac =================================================================== --- libcpp/configure.ac (revision 190830) +++ libcpp/configure.ac (working copy) @@ -146,6 +146,16 @@ if test $enable_checking != no ; then [Define if you want more run-time sanity checks.]) fi +AC_ARG_ENABLE(canonical-system-headers, +[ --enable-canonical-system-headers + enable or disable system headers canonicalization], +[], +enable_canonical_system_headers=yes) +if test $enable_canonical_system_headers != no; then + AC_DEFINE(ENABLE_CANONICAL_SYSTEM_HEADERS, + 1, [Define to enable system headers canonicalization.]) +fi + m4_changequote(,) case $target in alpha*-*-* | \ Index: libcpp/config.in =================================================================== --- libcpp/config.in (revision 190830) +++ libcpp/config.in (working copy) @@ -11,6 +11,9 @@ /* Define to 1 if using `alloca.c'. */ #undef C_ALLOCA +/* Define to enable system headers canonicalization. */ +#undef ENABLE_CANONICAL_SYSTEM_HEADERS + /* Define if you want more run-time sanity checks. */ #undef ENABLE_CHECKING