[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.