libgcc: Use inline variable instead of function-local static

On Tue, Oct 01, 2024 at 12:36:21PM +0200, Jakub Jelinek wrote:
> On Tue, Oct 01, 2024 at 11:10:03AM +0100, Jonathan Wakely wrote:
> > Let's use an inline variable. A function-local static requires
> > __cxa_guard_acquire, which (for some targets, including the ones
> > affected by this change) uses __gthread_active_p which will
> > recursively re-enter the variable's initialization.
> > 
> > So something like:
> > 
> > #pragma GCC diagnostic push
> > #pragma GCC diagnostic ignored "-Wc++17-extensions"
> > inline volatile int __gthread_active = -1;
> > #pragma GCC diagnostic pop
> 
> Note, only for #ifdef __cplusplus, for C there is no such thing as inline
> variables and in that case it should use
> static volatile int __ghtread_active = -1;
> instead.
> 
> 	Jakub
> 

So something like this maybe; bootstrapped and regtested on
x86_64-pc-linux-gnu and aarch64-unknown-linux-gnu, OK for trunk?

Or maybe it would be clearer to do the #ifdef __cplusplus here locally
rather than introducing a new __GTHREAD_VAR macro?

-- >8 --

Subject: [PATCH] libgcc: Use inline variable instead of function-local static

To support C++ ODR rules but remain compatible with C, I had initially
changed these variables to be function-local statics rather than global
internal-linkage decls.

However, both GCC and Clang support using namespace-scope 'inline'
variables even in pre-C++17 (with a warning), so let's simplify and do
that instead.  For C we instead revert back to using an internal-linkage
variable.

	PR c++/115126

libgcc/ChangeLog:

	* gthr-posix.h (__GTHREAD_VAR): New macro.
	(__gthread_active): Be a possibly-inline variable instead of a
	function.  Ignore any resultant diagnostics.
	(__gthread_trigger): __gthread_active is now a variable.
	(__gthread_active_p): Likewise.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 libgcc/gthr-posix.h | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

On Tue, Oct 08, 2024 at 08:18:56PM +1100, Nathaniel Shead wrote:
> On Tue, Oct 01, 2024 at 12:36:21PM +0200, Jakub Jelinek wrote:
> > On Tue, Oct 01, 2024 at 11:10:03AM +0100, Jonathan Wakely wrote:
> > > Let's use an inline variable. A function-local static requires
> > > __cxa_guard_acquire, which (for some targets, including the ones
> > > affected by this change) uses __gthread_active_p which will
> > > recursively re-enter the variable's initialization.
> > > 
> > > So something like:
> > > 
> > > #pragma GCC diagnostic push
> > > #pragma GCC diagnostic ignored "-Wc++17-extensions"
> > > inline volatile int __gthread_active = -1;
> > > #pragma GCC diagnostic pop
> > 
> > Note, only for #ifdef __cplusplus, for C there is no such thing as inline
> > variables and in that case it should use
> > static volatile int __ghtread_active = -1;
> > instead.
> > 
> > 	Jakub
> > 
> 
> So something like this maybe; bootstrapped and regtested on
> x86_64-pc-linux-gnu and aarch64-unknown-linux-gnu, OK for trunk?
> 
> Or maybe it would be clearer to do the #ifdef __cplusplus here locally
> rather than introducing a new __GTHREAD_VAR macro?

Actually, had a look again at this and I don't see what this is trying to
fix.
A function-local static requires __cxa_guard_acquire only if it needs
dynamic initialization, which is not the case here (the initializers are
= -1).  The non-dynamic ones are just initialized that way without any
__cxa_guard_acquire.

So, the function-local static seems more portable than hoping inline vars
will be supported even for older C++ versions.

	Jakub

On Tue, 8 Oct 2024 at 10:28, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Oct 08, 2024 at 08:18:56PM +1100, Nathaniel Shead wrote:
> > On Tue, Oct 01, 2024 at 12:36:21PM +0200, Jakub Jelinek wrote:
> > > On Tue, Oct 01, 2024 at 11:10:03AM +0100, Jonathan Wakely wrote:
> > > > Let's use an inline variable. A function-local static requires
> > > > __cxa_guard_acquire, which (for some targets, including the ones
> > > > affected by this change) uses __gthread_active_p which will
> > > > recursively re-enter the variable's initialization.
> > > >
> > > > So something like:
> > > >
> > > > #pragma GCC diagnostic push
> > > > #pragma GCC diagnostic ignored "-Wc++17-extensions"
> > > > inline volatile int __gthread_active = -1;
> > > > #pragma GCC diagnostic pop
> > >
> > > Note, only for #ifdef __cplusplus, for C there is no such thing as inline
> > > variables and in that case it should use
> > > static volatile int __ghtread_active = -1;
> > > instead.
> > >
> > >     Jakub
> > >
> >
> > So something like this maybe; bootstrapped and regtested on
> > x86_64-pc-linux-gnu and aarch64-unknown-linux-gnu, OK for trunk?
> >
> > Or maybe it would be clearer to do the #ifdef __cplusplus here locally
> > rather than introducing a new __GTHREAD_VAR macro?
>
> Actually, had a look again at this and I don't see what this is trying to
> fix.
> A function-local static requires __cxa_guard_acquire only if it needs
> dynamic initialization, which is not the case here (the initializers are
> = -1).  The non-dynamic ones are just initialized that way without any
> __cxa_guard_acquire.

Whoops, yes of course. Raising the topic of __cxa_guard_acquire was my
fault, and doesn't apply here, sorry.

But we still want to get rid of the STB_GNU_UNIQUE binding for the
static variables, don't we?

I've lost track a bit, so please correct me if I've got this wrong:

We originally had global static variables, which means a different
variable per TU. That causes ODR violations which were silently
ignored until we try to use them in modules, where they're diagnosed.
So we need to replace them.

Currently on trunk we have local statics, which avoid the ODR
violations, but emit STB_GNU_UNIQUE symbols, which are not zero-cost.
(There are no __cxa_guard_acquire issues, that was just my mistake.)

Jason suggested inline variables instead of local statics. That just
uses vague linkage for them (comdat), so no STB_GNU_UNIQUE, right?

Whether we choose local static or inline globals, these symbols will
now be visible across TUs and so become part of the ABI. Previously
they weren't, because every TU had its own copy which was manipulated
by always_inline static functions. That meant each TU had to
initialize its own variable, so we did the pthread_once trigger
multiple times (once per TU). But we could change the type of the
variable or the body of the always_inline static functions because
nothing affected other TUs. Whatever we do now, solving the ODR
violations for modules seems to require sharing variables between TUs
in one way or another, right?






>
> So, the function-local static seems more portable than hoping inline vars
> will be supported even for older C++ versions.
>
>         Jakub
>

On Tue, Oct 08, 2024 at 10:57:06AM +0100, Jonathan Wakely wrote:
> Whoops, yes of course. Raising the topic of __cxa_guard_acquire was my
> fault, and doesn't apply here, sorry.
> 
> But we still want to get rid of the STB_GNU_UNIQUE binding for the
> static variables, don't we?

I think it is the other way around.  At least when I try:
static inline volatile int *foo (void) { static volatile int v = -1; return &v; }
inline volatile int z = -1;
volatile int *p = foo ();

z is STB_GNU_UNIQUE, _ZZL3foovE1v is not.  Whether that is a bug or not, I
don't know...

	Jakub

On Tue, 8 Oct 2024 at 11:11, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Tue, Oct 08, 2024 at 10:57:06AM +0100, Jonathan Wakely wrote:
> > Whoops, yes of course. Raising the topic of __cxa_guard_acquire was my
> > fault, and doesn't apply here, sorry.
> >
> > But we still want to get rid of the STB_GNU_UNIQUE binding for the
> > static variables, don't we?
>
> I think it is the other way around.  At least when I try:
> static inline volatile int *foo (void) { static volatile int v = -1; return &v; }
> inline volatile int z = -1;
> volatile int *p = foo ();
>
> z is STB_GNU_UNIQUE, _ZZL3foovE1v is not.  Whether that is a bug or not, I
> don't know...

But that's because foo() is declared static, which we can't do because
of C++ modules. That was the original problem Nathaniel was fixing.

If you remove the 'static' from foo() then both z and _ZZ3foovE1v use
@gnu_unique_object

On Tue, Oct 08, 2024 at 11:32:16AM +0100, Jonathan Wakely wrote:
> But that's because foo() is declared static, which we can't do because
> of C++ modules. That was the original problem Nathaniel was fixing.
> 
> If you remove the 'static' from foo() then both z and _ZZ3foovE1v use
> @gnu_unique_object

Oops, pilot error.

	Jakub

Hello,

On Tue, 8 Oct 2024, Jonathan Wakely wrote:

> We originally had global static variables, which means a different
> variable per TU. That causes ODR violations which were silently
> ignored until we try to use them in modules, where they're diagnosed.
> So we need to replace them.

Aren't these variables implementation detail?  If so violating language 
rules like ODR should be okay.  (Imagine libgcc would be written in some 
language where such rules doesn't exist).

So isn't the actual problem that needs solving rather that c++ modules 
trip over these here?


Ciao,
Michael.

On Tue, 8 Oct 2024 at 13:59, Michael Matz <matz@suse.de> wrote:
>
> Hello,
>
> On Tue, 8 Oct 2024, Jonathan Wakely wrote:
>
> > We originally had global static variables, which means a different
> > variable per TU. That causes ODR violations which were silently
> > ignored until we try to use them in modules, where they're diagnosed.
> > So we need to replace them.
>
> Aren't these variables implementation detail?  If so violating language
> rules like ODR should be okay.  (Imagine libgcc would be written in some
> language where such rules doesn't exist).
>
> So isn't the actual problem that needs solving rather that c++ modules
> trip over these here?

Yes, I was thinking the same thing. Maybe we need a new attribute that
tells the front end not to diagnose the ODR violations for these even
in C++ modules.

On 10/8/24 8:58 AM, Michael Matz wrote:
> Hello,
> 
> On Tue, 8 Oct 2024, Jonathan Wakely wrote:
> 
>> We originally had global static variables, which means a different
>> variable per TU. That causes ODR violations which were silently
>> ignored until we try to use them in modules, where they're diagnosed.
>> So we need to replace them.
> 
> Aren't these variables implementation detail?  If so violating language
> rules like ODR should be okay.  (Imagine libgcc would be written in some
> language where such rules doesn't exist).
> 
> So isn't the actual problem that needs solving rather that c++ modules
> trip over these here?

That's an interesting point; we need to handle internal variables in 
header units, I'm not sure how much trouble it would be to provide a way 
to suppress the exposure diagnostic for particular statics in a named 
module.

Jason