[PATCH] libstdc++: don't use #include_next in c_global headers

[PATCH] libstdc++: don't use #include_next in c_global headers

[Helmut Grohne]

The <cmath> and <cstdlib> headers need their counter parts <math.h> and
<stdlib.h> from the libc respectively, but libstdc++ wraps these
headers. Now <cmath> and <cstdlib> include these headers using

$ echo '#include <cstdlib>' | g++ -x c++ -E - -isystem /usr/include >/dev/null
In file included from <stdin>:1:
/usr/include/c++/9/cstdlib:75:15: fatal error: stdlib.h: No such file or directory
   75 | #include_next <stdlib.h>
      |               ^~~~~~~~~~
compilation terminated.
$

What happens here is that g++ includes
libstdc++-v3/include/c_global/cstdlib. That header temporarily #defines
_GLIBCXX_INCLUDE_NEXT_C_HEADERS and then does #include_next <stdlib.h>.
libstdc++-v3's replacement libstdc++-v3/include/c_comaptibility/stdlib.h
happens to come earlier and is not considered.  Unfortunately, the
-isystem above inserted glibc's header before the location containing
<cstdlib>, so the #include_next continues searching and fails to find
<stdlib.h>.

Now you are probably going to say that "-isystem /usr/include" is a bad
idea and that you shouldn't do that. I'm inclined to agree. This isn't a
problem just yet. Debian wants to move /usr/include/stdlib.h to
/usr/include/<multiarch>/stdlib.h. After that move, the problematic flag
becomes "-isystem /usr/include/<multiarch>". Unfortunately, around 30
Debian packages[1] do pass exactly that flag. Regardless whether doing
so is a bad idea, I guess we will have to support that.

I am proposing to replace those two #include_next with plain #include.
That'll solve the problem described above, but it is not entirely
obvious that doing so doesn't break something else.

After switching those #include_next to #include,
libstdc++-v3/include/c_global/cstdlib will continue to temporarily
will #include <stdlib.h>. Now, it'll search all include directories. It
may find libstdc++-v3/include/c_comaptibility/stdlib.h or the libc's
version. We cannot tell which. If it finds the one from libstdc++-v3,
the header will notice the _GLIBCXX_INCLUDE_NEXT_C_HEADERS macro and
immediately #include_next <stdlib.h> skipping the rest of the header.
That in turn will find the libc version. So in both cases, it ends up
using the right one. Precisely what we wanted. #include_next is simply
not useful here.

The #include_next was originally added via PRs libstdc++/14608 and
libstdc++/60401. At that time, the _GLIBCXX_INCLUDE_NEXT_C_HEADERS guard
macro was also added. It seems like the #include_next was a meant as an
extra safe-guard, but actually breaks a practical use case.

For these reasons, I think that using #include_next here is harmful and
that replacing it with plain #include solves the problem without
introducing regressions.

[1] Including but not limited chromium-browser, inkscape, various kde
    packages, opencv, and vtk.

libstdc++-v3/ChangeLog:

	* include/c_global/cmath: Don't use #include_next.
	* include/c_global/cstdlib: Likewise.
---
 libstdc++-v3/include/c_global/cmath   | 2 +-
 libstdc++-v3/include/c_global/cstdlib | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Given the patch's size, I think that the copyright dance is not
necessary. The issue affects at least gcc-8 to gcc-10. Please Cc me in
replies.

Helmut

diff --git a/libstdc++-v3/include/c_global/cmath b/libstdc++-v3/include/c_global/cmath
index b99aaf8df40..8b2bb7c0785 100644
--- a/libstdc++-v3/include/c_global/cmath
+++ b/libstdc++-v3/include/c_global/cmath
@@ -42,7 +42,7 @@
 #include <bits/cpp_type_traits.h>
 #include <ext/type_traits.h>
 #define _GLIBCXX_INCLUDE_NEXT_C_HEADERS
-#include_next <math.h>
+#include <math.h>
 #undef _GLIBCXX_INCLUDE_NEXT_C_HEADERS
 #include <bits/std_abs.h>
 
diff --git a/libstdc++-v3/include/c_global/cstdlib b/libstdc++-v3/include/c_global/cstdlib
index f42db41fc51..80b39f6144f 100644
--- a/libstdc++-v3/include/c_global/cstdlib
+++ b/libstdc++-v3/include/c_global/cstdlib
@@ -72,7 +72,7 @@ namespace std
 // Need to ensure this finds the C library's <stdlib.h> not a libstdc++
 // wrapper that might already be installed later in the include search path.
 #define _GLIBCXX_INCLUDE_NEXT_C_HEADERS
-#include_next <stdlib.h>
+#include <stdlib.h>
 #undef _GLIBCXX_INCLUDE_NEXT_C_HEADERS
 #include <bits/std_abs.h>
 
-- 
2.26.0

Re: [PATCH] libstdc++: don't use #include_next in c_global headers

[Marc Glisse]

On Mon, 20 Apr 2020, Helmut Grohne wrote:

> Now you are probably going to say that "-isystem /usr/include" is a bad
> idea and that you shouldn't do that. I'm inclined to agree. This isn't a
> problem just yet. Debian wants to move /usr/include/stdlib.h to
> /usr/include/<multiarch>/stdlib.h. After that move, the problematic flag
> becomes "-isystem /usr/include/<multiarch>". Unfortunately, around 30
> Debian packages[1] do pass exactly that flag. Regardless whether doing
> so is a bad idea, I guess we will have to support that.

Urgh, no to "support that". I don't like those #include_next of a header 
with a different name and wouldn't mind seeing them go. But even if your 
patch, or some other patch, happens to make things kind of work, please do 
**not** consider this a supported feature, and keep fixing those broken 
packages (including the big bad cmake which regularly adds such flags to 
innocent packages).

With (or without) your patch, if a user has the bad -isystem and does 
#include <stdlib.h>, it will never see libstdc++'s version of stdlib.h, 
which contains important extra content, so that's still not working 
properly.

-- 
Marc Glisse

Bug#798955: Info received ([PATCH] libstdc++: don't use #include_next in c_global headers)

[Debian Bug Tracking System]

Thank you for the additional information you have supplied regarding
this Bug report.

This is an automatically generated reply to let you know your message
has been received.

Your message is being forwarded to the package maintainers and other
interested parties for their attention; they will reply in due course.

Your message has been sent to the package maintainer(s):
 GNU Libc Maintainers <debian-glibc@lists.debian.org>

If you wish to submit further information on this problem, please
send it to 798955@bugs.debian.org.

Please do not send mail to owner@bugs.debian.org unless you wish
to report a problem with the Bug-tracking system.

-- 
798955: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=798955
Debian Bug Tracking System
Contact owner@bugs.debian.org with problems

Re: [PATCH] libstdc++: don't use #include_next in c_global headers

[Jonathan Wakely]

On 20/04/20 07:01 +0200, Helmut Grohne wrote:
>The <cmath> and <cstdlib> headers need their counter parts <math.h> and
><stdlib.h> from the libc respectively, but libstdc++ wraps these
>headers. Now <cmath> and <cstdlib> include these headers using
>
>$ echo '#include <cstdlib>' | g++ -x c++ -E - -isystem /usr/include >/dev/null
>In file included from <stdin>:1:
>/usr/include/c++/9/cstdlib:75:15: fatal error: stdlib.h: No such file or directory
>   75 | #include_next <stdlib.h>
>      |               ^~~~~~~~~~
>compilation terminated.
>$
>
>What happens here is that g++ includes
>libstdc++-v3/include/c_global/cstdlib. That header temporarily #defines
>_GLIBCXX_INCLUDE_NEXT_C_HEADERS and then does #include_next <stdlib.h>.
>libstdc++-v3's replacement libstdc++-v3/include/c_comaptibility/stdlib.h
>happens to come earlier and is not considered.  Unfortunately, the
>-isystem above inserted glibc's header before the location containing
><cstdlib>, so the #include_next continues searching and fails to find
><stdlib.h>.
>
>Now you are probably going to say that "-isystem /usr/include" is a bad
>idea and that you shouldn't do that.

Right.

>I'm inclined to agree. This isn't a
>problem just yet. Debian wants to move /usr/include/stdlib.h to
>/usr/include/<multiarch>/stdlib.h. After that move, the problematic flag
>becomes "-isystem /usr/include/<multiarch>". Unfortunately, around 30
>Debian packages[1] do pass exactly that flag. Regardless whether doing
>so is a bad idea, I guess we will have to support that.

Or Debian should fix what they're going to break.

>I am proposing to replace those two #include_next with plain #include.
>That'll solve the problem described above, but it is not entirely
>obvious that doing so doesn't break something else.
>
>After switching those #include_next to #include,
>libstdc++-v3/include/c_global/cstdlib will continue to temporarily
>will #include <stdlib.h>. Now, it'll search all include directories. It
>may find libstdc++-v3/include/c_comaptibility/stdlib.h or the libc's
>version. We cannot tell which. If it finds the one from libstdc++-v3,
>the header will notice the _GLIBCXX_INCLUDE_NEXT_C_HEADERS macro and
>immediately #include_next <stdlib.h> skipping the rest of the header.
>That in turn will find the libc version. So in both cases, it ends up
>using the right one. Precisely what we wanted.

As Marc said, this doesn't work.

If a program tries to include <stdlib.h> it needs to get the libstdc++
version, otherwise only the libc versions of certain functions are
defined. That means the additional C++ overloads such as ::abs(long)
and ::abs(long long) won't be defined. That is the reason why
libstdc++ provides its own <stdlib.h>.

And if you do -isystem /usr/include (or any other option that causes
libstdc++'s <stdlib.h> to be skipped) that doesn't work. Only
::abs(int) gets defined.

So -isystem /usr/include breaks code, with or without your patch.

Re: [PATCH] libstdc++: don't use #include_next in c_global headers

[Helmut Grohne]

Hi,

On Mon, Apr 20, 2020 at 10:12:37AM +0100, Jonathan Wakely wrote:
> > Now you are probably going to say that "-isystem /usr/include" is a bad
> > idea and that you shouldn't do that.
> 
> Right.
> 
> > I'm inclined to agree. This isn't a
> > problem just yet. Debian wants to move /usr/include/stdlib.h to
> > /usr/include/<multiarch>/stdlib.h. After that move, the problematic flag
> > becomes "-isystem /usr/include/<multiarch>". Unfortunately, around 30
> > Debian packages[1] do pass exactly that flag. Regardless whether doing
> > so is a bad idea, I guess we will have to support that.
> 
> Or Debian should fix what they're going to break.

This is not quite precise. The offending -isystem
/usr/include/<multiarch> flag is already being passed. According to what
you write later, doing so is broken today. It just happens to work by
accident. So all we do is making the present breakage visible.

> > I am proposing to replace those two #include_next with plain #include.
> > That'll solve the problem described above, but it is not entirely
> > obvious that doing so doesn't break something else.
> > 
> > After switching those #include_next to #include,
> > libstdc++-v3/include/c_global/cstdlib will continue to temporarily
> > will #include <stdlib.h>. Now, it'll search all include directories. It
> > may find libstdc++-v3/include/c_comaptibility/stdlib.h or the libc's
> > version. We cannot tell which. If it finds the one from libstdc++-v3,
> > the header will notice the _GLIBCXX_INCLUDE_NEXT_C_HEADERS macro and
> > immediately #include_next <stdlib.h> skipping the rest of the header.
> > That in turn will find the libc version. So in both cases, it ends up
> > using the right one. Precisely what we wanted.
> 
> As Marc said, this doesn't work.

That is not very precise either. Marc said that it won't fix all cases.
In practice, it would make those work that don't #include <stdlib.h> but
use #include <cstdlib> instead.

Marc also indicated that using include_next for a header of a different
name is wrong. So this is a bug in libstdc++ regardless of whether it
breaks or unbreaks other pieces of software.

> If a program tries to include <stdlib.h> it needs to get the libstdc++
> version, otherwise only the libc versions of certain functions are
> defined. That means the additional C++ overloads such as ::abs(long)
> and ::abs(long long) won't be defined. That is the reason why
> libstdc++ provides its own <stdlib.h>.
> 
> And if you do -isystem /usr/include (or any other option that causes
> libstdc++'s <stdlib.h> to be skipped) that doesn't work. Only
> ::abs(int) gets defined.
> 
> So -isystem /usr/include breaks code, with or without your patch.

It is very difficult to disagree with -isystem /usr/include or -isystem
/usr/include/<triplet> being broken and unsupported. Having you state it
that clearly does help with communicating to other upstreams. For this
reason, I've looked into the remaining cases. It turns out that there
aren't that many left. In particular chromium, opencv and vtk got fixed
in the mean time. Basically all remaining failures could be attributed
to qmake, which passes all directories below /usr/include (including
/usr/include and /usr/include/<triplet> if a .pc file mentions them)
using -isystem. I've sent a patch https://bugs.debian.org/958479 to make
qmake stop doing that.

I therefore agree with you that the patch I sent for libstdc++ is not
necessary to make packages build on Debian. Removing the offending
-isystem flags from the respective builds is a manageable option and has
already happened to a large extend.

We can conclude that the motivation for my patch is not a good one,
because it embraces broken behaviour. However, the use of include_next
remains a bug, because the name of the including and the name of the
included header differ, and it should be fixed on that ground.

Helmut

Re: [PATCH] libstdc++: don't use #include_next in c_global headers

[Jonathan Wakely]

On 23/04/20 06:32 +0200, Helmut Grohne wrote:
>Hi,
>
>On Mon, Apr 20, 2020 at 10:12:37AM +0100, Jonathan Wakely wrote:
>> > Now you are probably going to say that "-isystem /usr/include" is a bad
>> > idea and that you shouldn't do that.
>>
>> Right.
>>
>> > I'm inclined to agree. This isn't a
>> > problem just yet. Debian wants to move /usr/include/stdlib.h to
>> > /usr/include/<multiarch>/stdlib.h. After that move, the problematic flag
>> > becomes "-isystem /usr/include/<multiarch>". Unfortunately, around 30
>> > Debian packages[1] do pass exactly that flag. Regardless whether doing
>> > so is a bad idea, I guess we will have to support that.
>>
>> Or Debian should fix what they're going to break.
>
>This is not quite precise. The offending -isystem
>/usr/include/<multiarch> flag is already being passed. According to what
>you write later, doing so is broken today. It just happens to work by
>accident. So all we do is making the present breakage visible.
>
>> > I am proposing to replace those two #include_next with plain #include.
>> > That'll solve the problem described above, but it is not entirely
>> > obvious that doing so doesn't break something else.
>> >
>> > After switching those #include_next to #include,
>> > libstdc++-v3/include/c_global/cstdlib will continue to temporarily
>> > will #include <stdlib.h>. Now, it'll search all include directories. It
>> > may find libstdc++-v3/include/c_comaptibility/stdlib.h or the libc's
>> > version. We cannot tell which. If it finds the one from libstdc++-v3,
>> > the header will notice the _GLIBCXX_INCLUDE_NEXT_C_HEADERS macro and
>> > immediately #include_next <stdlib.h> skipping the rest of the header.
>> > That in turn will find the libc version. So in both cases, it ends up
>> > using the right one. Precisely what we wanted.
>>
>> As Marc said, this doesn't work.
>
>That is not very precise either. Marc said that it won't fix all cases.
>In practice, it would make those work that don't #include <stdlib.h> but
>use #include <cstdlib> instead.
>
>Marc also indicated that using include_next for a header of a different
>name is wrong. So this is a bug in libstdc++ regardless of whether it
>breaks or unbreaks other pieces of software.

He said he doesn't like it, that doesn't mean it's a bug or actually
causes incorrect results.

Whereas using -isystem provably *does* break the implementation,
making it impossible for #include <stdlib.h> to meet the requirements
of the C++ standard. And your proposed patch doesn't prevent that.


>> If a program tries to include <stdlib.h> it needs to get the libstdc++
>> version, otherwise only the libc versions of certain functions are
>> defined. That means the additional C++ overloads such as ::abs(long)
>> and ::abs(long long) won't be defined. That is the reason why
>> libstdc++ provides its own <stdlib.h>.
>>
>> And if you do -isystem /usr/include (or any other option that causes
>> libstdc++'s <stdlib.h> to be skipped) that doesn't work. Only
>> ::abs(int) gets defined.
>>
>> So -isystem /usr/include breaks code, with or without your patch.
>
>It is very difficult to disagree with -isystem /usr/include or -isystem
>/usr/include/<triplet> being broken and unsupported. Having you state it
>that clearly does help with communicating to other upstreams. For this
>reason, I've looked into the remaining cases. It turns out that there
>aren't that many left. In particular chromium, opencv and vtk got fixed
>in the mean time. Basically all remaining failures could be attributed
>to qmake, which passes all directories below /usr/include (including
>/usr/include and /usr/include/<triplet> if a .pc file mentions them)
>using -isystem. I've sent a patch https://bugs.debian.org/958479 to make
>qmake stop doing that.
>
>I therefore agree with you that the patch I sent for libstdc++ is not
>necessary to make packages build on Debian. Removing the offending
>-isystem flags from the respective builds is a manageable option and has
>already happened to a large extend.

Yes, I introduced the current <stdlib.h> and <math.h> wrappers years
ago in GCC 6, and so I'm surprised to see it coming up again now.
Several packages had problems and already fixed them.

>We can conclude that the motivation for my patch is not a good one,
>because it embraces broken behaviour. However, the use of include_next
>remains a bug, because the name of the including and the name of the
>included header differ, and it should be fixed on that ground.

Not liking something is not a bug.

You need to demonstrate an actual bug (e.g. failure to compile,
non-conformance to the C++ standard) that is not caused by user error
(like misuse of -isystem) to argue for fixing something.

Re: [PATCH] libstdc++: don't use #include_next in c_global headers

[Jonathan Wakely]

On 23/04/20 09:23 +0100, Jonathan Wakely wrote:
>On 23/04/20 06:32 +0200, Helmut Grohne wrote:
>>Hi,
>>
>>On Mon, Apr 20, 2020 at 10:12:37AM +0100, Jonathan Wakely wrote:
>>>> Now you are probably going to say that "-isystem /usr/include" is a bad
>>>> idea and that you shouldn't do that.
>>>
>>>Right.
>>>
>>>> I'm inclined to agree. This isn't a
>>>> problem just yet. Debian wants to move /usr/include/stdlib.h to
>>>> /usr/include/<multiarch>/stdlib.h. After that move, the problematic flag
>>>> becomes "-isystem /usr/include/<multiarch>". Unfortunately, around 30
>>>> Debian packages[1] do pass exactly that flag. Regardless whether doing
>>>> so is a bad idea, I guess we will have to support that.
>>>
>>>Or Debian should fix what they're going to break.
>>
>>This is not quite precise. The offending -isystem
>>/usr/include/<multiarch> flag is already being passed. According to what
>>you write later, doing so is broken today. It just happens to work by
>>accident. So all we do is making the present breakage visible.
>>
>>>> I am proposing to replace those two #include_next with plain #include.
>>>> That'll solve the problem described above, but it is not entirely
>>>> obvious that doing so doesn't break something else.
>>>>
>>>> After switching those #include_next to #include,
>>>> libstdc++-v3/include/c_global/cstdlib will continue to temporarily
>>>> will #include <stdlib.h>. Now, it'll search all include directories. It
>>>> may find libstdc++-v3/include/c_comaptibility/stdlib.h or the libc's
>>>> version. We cannot tell which. If it finds the one from libstdc++-v3,
>>>> the header will notice the _GLIBCXX_INCLUDE_NEXT_C_HEADERS macro and
>>>> immediately #include_next <stdlib.h> skipping the rest of the header.
>>>> That in turn will find the libc version. So in both cases, it ends up
>>>> using the right one. Precisely what we wanted.
>>>
>>>As Marc said, this doesn't work.
>>
>>That is not very precise either. Marc said that it won't fix all cases.
>>In practice, it would make those work that don't #include <stdlib.h> but
>>use #include <cstdlib> instead.
>>
>>Marc also indicated that using include_next for a header of a different
>>name is wrong. So this is a bug in libstdc++ regardless of whether it
>>breaks or unbreaks other pieces of software.
>
>He said he doesn't like it, that doesn't mean it's a bug or actually
>causes incorrect results.
>
>Whereas using -isystem provably *does* break the implementation,
>making it impossible for #include <stdlib.h> to meet the requirements
>of the C++ standard. And your proposed patch doesn't prevent that.
>
>
>>>If a program tries to include <stdlib.h> it needs to get the libstdc++
>>>version, otherwise only the libc versions of certain functions are
>>>defined. That means the additional C++ overloads such as ::abs(long)
>>>and ::abs(long long) won't be defined. That is the reason why
>>>libstdc++ provides its own <stdlib.h>.
>>>
>>>And if you do -isystem /usr/include (or any other option that causes
>>>libstdc++'s <stdlib.h> to be skipped) that doesn't work. Only
>>>::abs(int) gets defined.
>>>
>>>So -isystem /usr/include breaks code, with or without your patch.
>>
>>It is very difficult to disagree with -isystem /usr/include or -isystem
>>/usr/include/<triplet> being broken and unsupported. Having you state it
>>that clearly does help with communicating to other upstreams. For this
>>reason, I've looked into the remaining cases. It turns out that there
>>aren't that many left. In particular chromium, opencv and vtk got fixed
>>in the mean time. Basically all remaining failures could be attributed
>>to qmake, which passes all directories below /usr/include (including
>>/usr/include and /usr/include/<triplet> if a .pc file mentions them)
>>using -isystem. I've sent a patch https://bugs.debian.org/958479 to make
>>qmake stop doing that.
>>
>>I therefore agree with you that the patch I sent for libstdc++ is not
>>necessary to make packages build on Debian. Removing the offending
>>-isystem flags from the respective builds is a manageable option and has
>>already happened to a large extend.
>
>Yes, I introduced the current <stdlib.h> and <math.h> wrappers years
>ago in GCC 6, and so I'm surprised to see it coming up again now.
>Several packages had problems and already fixed them.
>
>>We can conclude that the motivation for my patch is not a good one,
>>because it embraces broken behaviour. However, the use of include_next
>>remains a bug, because the name of the including and the name of the
>>included header differ, and it should be fixed on that ground.
>
>Not liking something is not a bug.
>
>You need to demonstrate an actual bug (e.g. failure to compile,
>non-conformance to the C++ standard) that is not caused by user error
>(like misuse of -isystem) to argue for fixing something.

N.B. the GCC docs are quite clear that reordering include directories
risk breaking GCC's necessary use of #include_next:

   If a standard system include directory, or a directory specified
   with -isystem, is also specified with -I, the -I option is ignored.
   The directory is still searched but as a system directory at its
   normal position in the system include chain.  This is to ensure that
   GCC's procedure to fix buggy system headers and the ordering for the
   "#include_next" directive are not inadvertently changed.  If you
   really need to change the search order for system directories, use
   the -nostdinc and/or -isystem options.

Why do people think they "really need to change the search order"? If
they're going to play silly games, they should be prepared to do it
properly, and also put the standard C++ dirs back in the right order.

It seems to be this is simple user error, and those packages should be
fixed to stop misusing an option that is documented to have a risk of
breaking things.