Bug 70722 - include_next in cmath skips user-defined wrapper
Summary: include_next in cmath skips user-defined wrapper
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: libstdc++ (show other bugs)
Version: 6.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 70872 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-04-19 05:07 UTC by Martin Thomson
Modified: 2016-07-21 11:54 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Thomson 2016-04-19 05:07:19 UTC
Firefox has a bunch of wrappers for system headers.  I'm not 100% on the reasoning, but most just play with visibility.  For instance, the math.h wrapper looks like:

   #pragma GCC system_header
   #pragma GCC visibility push(default)
   #include_next <math.h>
   #pragma GCC visibility pop

Firefox fails to link when built with libstdc++ 6.0.0 because it includes <cmath> before it includes <math.h>.  This leads to inclusion of <math.h> with different visibility than is desired.

The version of cmath I have includes this:

   #define _GLIBCXX_INCLUDE_NEXT_C_HEADERS
   #include_next <math.h>
   #undef _GLIBCXX_INCLUDE_NEXT_C_HEADERS

Removing the "_next" causes the build to succeed.

You can see my flailings regarding this issue here: https://bugzilla.mozilla.org/show_bug.cgi?id=1264534
Comment 1 Martin Thomson 2016-04-19 05:14:39 UTC
My ViewCVS-fu isn't that good, it took me long enough to find this:

https://gcc.gnu.org/viewcvs/gcc/trunk/libstdc%2B%2B-v3/include/c_global/cmath?r1=232585&r2=232586&
Comment 2 Markus Trippelsdorf 2016-04-19 05:59:47 UTC
See: http://gcc.gnu.org/gcc-6/porting_to.html

For Firefox the following patch works for me:

diff --git a/nsprpub/config/make-system-wrappers.pl b/nsprpub/config/make-system-wrappers.pl                                                                                  
index fa0873a78e0a..6a3aee337908 100644                                                                                                                                       
--- a/nsprpub/config/make-system-wrappers.pl                                                                                                                                  
+++ b/nsprpub/config/make-system-wrappers.pl                                                                                                                                  
@@ -19,7 +19,9 @@ while (<STDIN>) {                                                                                                                                           
     open OUT, ">$output_dir/$_";                                                                                                                                             
     print OUT "#pragma GCC system_header\n";  # suppress include_next warning                                                                                                
     print OUT "#pragma GCC visibility push(default)\n";                                                                                                                      
+   print OUT "#define _GLIBCXX_INCLUDE_NEXT_C_HEADERS\n";                                                                                                                    
     print OUT "#include_next \<$_\>\n";                                                                                                                                      
+   print OUT "#undef _GLIBCXX_INCLUDE_NEXT_C_HEADERS\n";                                                                                                                     
     print OUT "#pragma GCC visibility pop\n";                                                                                                                                
     close OUT;                                                                                                                                                               
 }
Comment 3 Jonathan Wakely 2016-04-19 10:37:01 UTC
As discussed when I introduced #include_next into <cmath>, if Firefox wants to play games redefining system headers (which is explicitly undefined behaviour according to the C++ standard) then it is Firefox's problem to make it work, not ours.

This is documented at https://gcc.gnu.org/gcc-6/porting_to.html

  Programs which provide their own wrappers for <stdlib.h> or other standard
  headers are operating outside the standard and so are responsible for ensuring
  their headers work correctly with the headers in the C++ standard library.
Comment 4 Markus Trippelsdorf 2016-04-29 09:57:51 UTC
*** Bug 70872 has been marked as a duplicate of this bug. ***
Comment 5 guido 2016-05-12 15:20:00 UTC
I am also hitting the problem reported as a duplicate of this bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70872

Unfortunately, the patch suggested in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70722#c2 does not work for me.

If this is invalid as a gcc bug, then perhaps it can be opened as a bug in firefox?!?
Comment 6 guido 2016-05-13 06:22:13 UTC
Reported as a RESOLVED FIXED bug in firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1245076
Comment 7 Vittorio Zecca 2016-05-13 15:52:30 UTC
Yes, this fixed my problem with mozilla firefox compilation,
Thank you!
Comment 8 Nadav Har'El 2016-07-21 06:13:07 UTC
This issue was closed as supposedly it only affects Firefox's build. It definitely does not - it affects any project which attempts to replace the C header files (or just one or few of them) but leave the C++ header files untouched.

The latest casualty of the "#include_next" thing is the OSv project (see OSv issue https://github.com/cloudius-systems/osv/issues/768). OSv is a unikernel which replaces glibc with its own C library implementation, including re-implementing all the glibc header files - but it does not replace the libstdc++ header files.

Before this "#include_next" thing in the C++ header files, to override the default C header files we just needed to add to the include path OSv's own header directory, and the C header files were picked up from there - while the C++ header files continued to be picked up as usual from their standard location. Now, we can no longer do that, because when the C++ header files try to "#include_next" the C header files, it can't find them, or picks up the wrong one (the standard one from /usr/include).

I do have a workaround for this, which includes -D_GLIBCXX_INCLUDE_NEXT_C_HEADERS *and* manually placing another copy of the default C++ header directory in the include path before our replacement C header directory (I've yet to figure out how to find this correctly for any version of gcc...). But it looks really convoluted, and unjustified.

I think the correct solution would have been NOT to have the same header file (such as stdlib.h and math.h) in two directories and two projects (libstdc++ and glibc). That duplication caused the mess that the "#include_next" was supposed to fix (and as collateral damage, caused this issue). I believe there is no reason why the C header files (probably in the glibc project) should not be enough for both C and C++ code. If the C++ standard dictates that "stdlib.h" should have something specific for C++, it is quite easy to add that with an #ifdef __cplusplus, and we shouldn't have to require a completely separate file with the same name.
Comment 9 Andrew Pinski 2016-07-21 06:20:03 UTC
(In reply to Nadav Har'El from comment #8)
> This issue was closed as supposedly it only affects Firefox's build. It
> definitely does not - it affects any project which attempts to replace the C
> header files (or just one or few of them) but leave the C++ header files
> untouched.

But all of those projects are broken the same way as Firefox was broken.
You can't expect replacing standard C library headers and something not breaking.
Comment 10 Nadav Har'El 2016-07-21 06:31:09 UTC
There is nothing "holy" about glibc, and nothing "broken" about wanting to replace it (or, as Firefox did, only a part of it). Sure, the replacement needs to be 100% compatible with glibc, and if it isn't, the replacement needs to be fixed (e.g., I just fixed this https://github.com/cloudius-systems/osv/issues/770 and didn't come complaining to the libstdc++ project).

HOWEVER, in this case, the projects in question (Firefox, OSv, and probably more to come) did nothing incorrect or incompatible in their glibc replacement. Their only "sin" was the order of the header files.

Moreover, playing with the order is not enough to fix this mess. The problem is that on one hand we need the replacement C library headers to come *after* the C++ headers (because of the #include_next thing in the C++ headers) but on the other hand when we do include "stdlib.h" we need it to come from the replacement C library, NOT from the C++ library, requiring the opposite path order! Luckily, the -D_GLIBCXX_INCLUDE_NEXT_C_HEADERS trick solves that issue. So it's not impossible to solve, it's just really awkward, and the awkwardness suggests that it was the wrong solution to have two copies of the same header file with "#include_next" as a trick to have one include the others.

I still don't understand why the glibc header files could not have been improved to accommodate also the C++ standard (and I have no idea what the issues were) instead of needing to have copies of the same header file in both libstdc++ and glibc.
Comment 11 Nadav Har'El 2016-07-21 06:46:02 UTC
By the way, I think I made a mistake in my comment and -D_GLIBCXX_INCLUDE_NEXT_C_HEADERS is not actually needed. The thing is that header files like <cmath> have:

#define _GLIBCXX_INCLUDE_NEXT_C_HEADERS
#include_next <math.h>
#undef _GLIBCXX_INCLUDE_NEXT_C_HEADERS

And this forces me to put the modified math.h after, not before as expected, the C++ headers in the include path.

It is actually fine for me that if someone includes <math.h> he gets the libstdc++ one - which then includes (in C++) <cmath> which eventually includes (as above) my modified C header, so I do not actually need to set _GLIBCXX_INCLUDE_NEXT_C_HEADERS. Just set the header file order in the "right" (according to libstdc++) order, which unfortunately requires me to figure out the system's default C++ header directory (not /usr/include!) and add it explicitly to the include path.
Comment 12 Jonathan Wakely 2016-07-21 11:47:55 UTC
(In reply to Nadav Har'El from comment #10)
> There is nothing "holy" about glibc, and nothing "broken" about wanting to
> replace it (or, as Firefox did, only a part of it). Sure, the replacement
> needs to be 100% compatible with glibc, and if it isn't, the replacement
> needs to be fixed (e.g., I just fixed this
> https://github.com/cloudius-systems/osv/issues/770 and didn't come
> complaining to the libstdc++ project).
> 
> HOWEVER, in this case, the projects in question (Firefox, OSv, and probably
> more to come) did nothing incorrect or incompatible in their glibc
> replacement. Their only "sin" was the order of the header files.

Are you sure about that?

 17.6.4.4  Headers  [alt.headers]
 If a file with a name equivalent to the derived file name for one of the C++
 standard library headers is not provided as part of the implementation, and
 a file with that name is placed in any of the standard places for a source
 file to be included (16.2), the behavior is undefined.

> I still don't understand why the glibc header files could not have been
> improved to accommodate also the C++ standard (and I have no idea what the
> issues were) instead of needing to have copies of the same header file in
> both libstdc++ and glibc.

Not everybody uses glibc.

The change is documented at https://gcc.gnu.org/gcc-6/porting_to.html so if you want to continue playing games with undefined behaviour then it's your job to fix it.
Comment 13 Jonathan Wakely 2016-07-21 11:54:08 UTC
The C standard has equivalent wording in 7.1.2:

If a file with the same name as one of the above < and > delimited sequences, not
provided as part of the implementation, is placed in any of the standard places that are searched for included source files, the behavior is undefined.