Re: Re: [committed] libstdc++: Allow Clang to use <stdatomic.h> before C++23

Re: Re: [committed] libstdc++: Allow Clang to use <stdatomic.h> before C++23

[Jonathan Wakely]

On Wed, 4 Jan 2023 at 16:31, Alexander Hans wrote:
>
> Hi Jonathan,

Hi,

> I'm trying to integrate gcc 12.2.0 in a Bazel build and have an issue
> compiling some C code that includes stdatomic.h. Instead of using the C
> stdatomic.h, it includes the C++23 one, which there does nothing. Your

It should be impossible for C code to include that header.


> patch mentioned in the subject seems to address a similar issue.
> However, in my case, it doesn't fix the issue, because when using gcc,
> #if __clang__ is not true. When I change the line to be an
> unconditional #else instead of the #else if, things work as expected.
>
> Why did you put the if __clang__? I can imagine you wanted to address
> this one usecase when people use clang++, but wouldn't the same issue
> occur for g++?

With Clang, including the C version of <stdatomic.h> actually works,
because clang++ supports the _Atomic qualifier even in C++ code. So
there is non-portable C++17 and C++20 code out there already which
includes <stdatomic.h> and expects it to work (as long as it's only
compiled with Clang).

My change to the libstdc++ <stdatomic.h> allows that code to keep
compiling when using Clang. Such code never compiled with g++ at any
time, so there is no need to support it. It never worked anyway.

> I suppose in a usual system-installation, the C++23
> file is not part of the builtin include paths when compiling C code.

Right. Not just a system-installation. In any GCC installation, the
libstdc++ headers are in a C++-specific directory that the C compiler
never looks in.

> However, when having the toolchain managed by Bazel, I can only give
> a single set of include directories [1]:
>
> > cxx_builtin_include_directories
> > [...]
> > We currently use the C++ paths also for C compilation, which is
> > safe as long as there are no name clashes between C++ and C header
> > files.
>
> I can work around this by patching my local version of C++23's
> stdatomic.h, but I suppose other Bazel users will face that issue as
> well, since the assumption of no name clashes is not true anymore.

Then Bazel is broken. The assumption that "there are no name clashes
between C++ and C header files" has never been true.

I suppose we could support that silliness like this:

--- a/libstdc++-v3/include/c_compatibility/stdatomic.h
+++ b/libstdc++-v3/include/c_compatibility/stdatomic.h
@@ -124,7 +124,7 @@ using std::atomic_flag_clear_explicit;
using std::atomic_thread_fence;
using std::atomic_signal_fence;

-#elif defined __clang__
+#elif defined __clang__ || !defined __cplusplus
# include_next <stdatomic.h>
#endif // C++23
#endif // _GLIBCXX_STDATOMIC_H

But I don't really like having to patch libstdc++ to support broken tools.



> I wasn't sure if posting to the mailing list was appropriate. Feel
> free to CC to the list!

Yes, this should be on the mailing list, CC'd.


>
>
> Thanks and cheers!
>
> Alex
>
>
> [1] https://bazel.build/rules/lib/cc_common#create_cc_toolchain_config_info
>

Re: Re: [committed] libstdc++: Allow Clang to use <stdatomic.h> before C++23

[Jonathan Wakely]

On Wed, 4 Jan 2023 at 16:57, Jonathan Wakely wrote:
>
> On Wed, 4 Jan 2023 at 16:31, Alexander Hans wrote:
> >
> > Hi Jonathan,
>
> Hi,
>
> > I'm trying to integrate gcc 12.2.0 in a Bazel build and have an issue
> > compiling some C code that includes stdatomic.h. Instead of using the C
> > stdatomic.h, it includes the C++23 one, which there does nothing. Your
>
> It should be impossible for C code to include that header.
>
>
> > patch mentioned in the subject seems to address a similar issue.
> > However, in my case, it doesn't fix the issue, because when using gcc,
> > #if __clang__ is not true. When I change the line to be an
> > unconditional #else instead of the #else if, things work as expected.
> >
> > Why did you put the if __clang__? I can imagine you wanted to address
> > this one usecase when people use clang++, but wouldn't the same issue
> > occur for g++?
>
> With Clang, including the C version of <stdatomic.h> actually works,
> because clang++ supports the _Atomic qualifier even in C++ code. So
> there is non-portable C++17 and C++20 code out there already which
> includes <stdatomic.h> and expects it to work (as long as it's only
> compiled with Clang).
>
> My change to the libstdc++ <stdatomic.h> allows that code to keep
> compiling when using Clang. Such code never compiled with g++ at any
> time, so there is no need to support it. It never worked anyway.
>
> > I suppose in a usual system-installation, the C++23
> > file is not part of the builtin include paths when compiling C code.
>
> Right. Not just a system-installation. In any GCC installation, the
> libstdc++ headers are in a C++-specific directory that the C compiler
> never looks in.
>
> > However, when having the toolchain managed by Bazel, I can only give
> > a single set of include directories [1]:
> >
> > > cxx_builtin_include_directories
> > > [...]
> > > We currently use the C++ paths also for C compilation, which is
> > > safe as long as there are no name clashes between C++ and C header
> > > files.
> >
> > I can work around this by patching my local version of C++23's
> > stdatomic.h, but I suppose other Bazel users will face that issue as
> > well, since the assumption of no name clashes is not true anymore.
>
> Then Bazel is broken. The assumption that "there are no name clashes
> between C++ and C header files" has never been true.

Why does Bazel even need to know those directories? It should not be
adding them for compiling C code nor for compiling C++ code, because
GCC does that automatically (and gets it right).


>
> I suppose we could support that silliness like this:
>
> --- a/libstdc++-v3/include/c_compatibility/stdatomic.h
> +++ b/libstdc++-v3/include/c_compatibility/stdatomic.h
> @@ -124,7 +124,7 @@ using std::atomic_flag_clear_explicit;
> using std::atomic_thread_fence;
> using std::atomic_signal_fence;
>
> -#elif defined __clang__
> +#elif defined __clang__ || !defined __cplusplus
> # include_next <stdatomic.h>
> #endif // C++23
> #endif // _GLIBCXX_STDATOMIC_H
>
> But I don't really like having to patch libstdc++ to support broken tools.
>
>
>
> > I wasn't sure if posting to the mailing list was appropriate. Feel
> > free to CC to the list!
>
> Yes, this should be on the mailing list, CC'd.
>
>
> >
> >
> > Thanks and cheers!
> >
> > Alex
> >
> >
> > [1] https://bazel.build/rules/lib/cc_common#create_cc_toolchain_config_info
> >

Re: Re: [committed] libstdc++: Allow Clang to use <stdatomic.h> before C++23

[Alexander Hans]

Hi,

Thanks for your prompt reply. Now things are much clearer!


> Then Bazel is broken. The assumption that "there are no name clashes
> between C++ and C header files" has never been true.

> I suppose we could support that silliness like this:
> [...]
> But I don't really like having to patch libstdc++ to support broken tools.

I agree that libstdc++ is not the right place to fix this. But thanks for the
patch in any case, that's better than my unconditional #else and I'll keep
it in mind. That would also allow me to work around the issue.

Regarding your question why this happens in the first place:

> Why does Bazel even need to know those directories? It should not be
> adding them for compiling C code nor for compiling C++ code, because
> GCC does that automatically (and gets it right).

Bazel can detect a local toolchain and use it. Then everything works as
expected. But it can also support some other toolchain, contained in some
archive you tell it to download from somewhere. I'm trying to integrate
gcc 12 via this route and do this based on some existing Bazel rules that are
pretty close to what I need [1]. They call gcc passing -nostdinc and giving
the include directories via -isystem arguments. However, some preliminary
testing suggests you should actually use only --sysroot for that and avoid
-nostdinc and any extra -isystem arguments. If that's true, the fix should
happen in the gcc toolchain-specific Bazel rules. The section I quoted
about the cxx_builtin_include_directories parameter and the assumption
that there are no name clashes is probably not an issue: I believe this
is only about telling Bazel where those files live, so that it can make them
available (Bazel is all about hermetic builds and to that end it wants to
know exactly what is used for the build). Using --sysroot, they will be
made available for C compilations as well, but ignored as you explained.
The only unclean part about this is that we cannot tell Bazel which
toolchain-supplied files a C-only build depends on (it will always be both,
C and C++ files), but that is acceptable.

Thanks again!

Alex

[1] https://github.com/aspect-build/gcc-toolchain

Re: Re: [committed] libstdc++: Allow Clang to use <stdatomic.h> before C++23

[Jonathan Wakely]

On Wed, 4 Jan 2023 at 21:07, Alexander Hans wrote:
>
> Hi,
>
> Thanks for your prompt reply. Now things are much clearer!
>
>
> > Then Bazel is broken. The assumption that "there are no name clashes
> > between C++ and C header files" has never been true.
>
> > I suppose we could support that silliness like this:
> > [...]
> > But I don't really like having to patch libstdc++ to support broken tools.
>
> I agree that libstdc++ is not the right place to fix this. But thanks for the
> patch in any case, that's better than my unconditional #else and I'll keep
> it in mind. That would also allow me to work around the issue.

I did post a slightly tweaked version of that patch for review:
https://gcc.gnu.org/pipermail/gcc-patches/2023-January/609833.html
(I forgot that this discussion had been on the mailing list, not a
private email, sorry!)

I didn't get any feedback, positive or negative.

> Regarding your question why this happens in the first place:
>
> > Why does Bazel even need to know those directories? It should not be
> > adding them for compiling C code nor for compiling C++ code, because
> > GCC does that automatically (and gets it right).
>
> Bazel can detect a local toolchain and use it. Then everything works as
> expected. But it can also support some other toolchain, contained in some
> archive you tell it to download from somewhere. I'm trying to integrate
> gcc 12 via this route and do this based on some existing Bazel rules that are
> pretty close to what I need [1]. They call gcc passing -nostdinc and giving
> the include directories via -isystem arguments. However, some preliminary
> testing suggests you should actually use only --sysroot for that and avoid
> -nostdinc and any extra -isystem arguments. If that's true,

Yes, I do think that's true.

The GCC you unpack from the archive is "relocatable" in that you can
place the files anywhere, and gcc and g++ will determine where to find
their own headers relative to the location of the gcc and g++
binaries. So as long as you maintain the directory structure, you can
relocate the entire installation tree to another directory. GCC can't
know where to find libc and other headers that are not part of the GCC
installation, which is what --sysroot is used for (if you don't want
it to use the default system headers in /usr/include etc.)

But for GCC's own headers that are part of the GCC installation, it
does the right thing automatically. You break that by using -nostdinc
and then adding an incorrect set of directories back again using
-isystem.

> the fix should
> happen in the gcc toolchain-specific Bazel rules. The section I quoted
> about the cxx_builtin_include_directories parameter and the assumption
> that there are no name clashes is probably not an issue: I believe this
> is only about telling Bazel where those files live, so that it can make them
> available (Bazel is all about hermetic builds and to that end it wants to
> know exactly what is used for the build). Using --sysroot, they will be
> made available for C compilations as well, but ignored as you explained.
> The only unclean part about this is that we cannot tell Bazel which
> toolchain-supplied files a C-only build depends on (it will always be both,
> C and C++ files), but that is acceptable.

OK, I'm glad you got it working. I'm not seeing much motivation to
push the patched linked above, but on the other hand it doesn't do any
harm either.