[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[palmer at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

palmer at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |palmer at gcc dot gnu.org

--- Comment #3 from palmer at gcc dot gnu.org ---
I'm not sure what the right way to go about fixing this is.

Assuming the inline atomics
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104338#c3> work out we'll end up
with libstdc++ using atomics when compiled with newer GCCs and using locks when
compiled with older GCCs.  Those two implementations aren't compatible with
each other, as the direct atomics won't respect the lock.  That certainly means
these two implementations can't coexist, but I'm not actually sure if this is
an ABI break because I don't know what the ABI surface is supposed to be here.

As far as I can tell simple uses cases are safe here, as __exchange_and_add
isn't inlined outside of the shared library (though I'm not seeing anything
guaranteeing that, so I may be wrong here).  Doing something like trying to
mmap a shared_ptr and access from both flavors of the library (maybe one's
statically linked, for example) would also break, and I could imagine that
existing in real code.

It looks like LLVM already has inline atomics, so presumably the same issues
would arise when mixing libstdc++ libraries compiled with LLVM and GCC.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #4 from Andreas Schwab <schwab@linux-m68k.org> ---
Not having inline atomics already breaks so many things.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |redi at gcc dot gnu.org

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to palmer from comment #3)
> I'm not sure what the right way to go about fixing this is.
> 
> Assuming the inline atomics
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104338#c3> work out we'll end
> up with libstdc++ using atomics when compiled with newer GCCs and using
> locks when compiled with older GCCs.  Those two implementations aren't
> compatible with each other, as the direct atomics won't respect the lock. 
> That certainly means these two implementations can't coexist, but I'm not
> actually sure if this is an ABI break because I don't know what the ABI
> surface is supposed to be here.

It's definitely an ABI break. You can't pass a std::shared_ptr (or anything
using it, like std::future) between translation units that disagree about the
atomics policy.


> As far as I can tell simple uses cases are safe here, as __exchange_and_add
> isn't inlined outside of the shared library (though I'm not seeing anything
> guaranteeing that, so I may be wrong here).

But user code can use std::shared_ptr and other reference-counted types, and
inline those functions. This isn't just about libstdc++.so itself.


> Doing something like trying to
> mmap a shared_ptr and access from both flavors of the library (maybe one's
> statically linked, for example) would also break, and I could imagine that
> existing in real code.

You don't need to do anything fancy like that to hit the problem. Just two
translation units, one compiled with the old gcc and one compiled with the new.

> It looks like LLVM already has inline atomics, so presumably the same issues
> would arise when mixing libstdc++ libraries compiled with LLVM and GCC.

Yup.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #5)
> (In reply to palmer from comment #3)
> > It looks like LLVM already has inline atomics, so presumably the same issues
> > would arise when mixing libstdc++ libraries compiled with LLVM and GCC.
> 
> Yup.

But not just "when mixing libstdc++ libraries". When mixing pretty much any C++
code that uses libstdc++ headers.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #6)
> (In reply to Jonathan Wakely from comment #5)
> > (In reply to palmer from comment #3)
> > > It looks like LLVM already has inline atomics, so presumably the same issues
> > > would arise when mixing libstdc++ libraries compiled with LLVM and GCC.
> > 
> > Yup.
> 
> But not just "when mixing libstdc++ libraries". When mixing pretty much any
> C++ code that uses libstdc++ headers.

Oh actually, sorry, that's wrong. The atomic policy for libstdc++ is set at
configure time, based on the GCC building it. We define a macro, and that is
fixed for the lifetime of that libstdc++ installation. So it doesn't matter if
you compile those same headers with Clang, which _could_ use atomic built-ins,
the atomic policy is still decided by the macro which doesn't change after
installation.

So it's only a problem when mixing user code compiled with old and new
libstdc++ headers.

And I've been confusing the _GLIBCXX_ATOMIC_BUILTINS macro with the
_GLIBCXX_HAVE_ATOMIC_LOCK_POLICY macro.

If _GLIBCXX_ATOMIC_BUILTINS changes from undefined to 1, I think that's OK. Old
code will still call the non-inline functions in libstdc++.so, but those will
now be consistent with the inline ones that new code is calling.

The problem for std::shared_ptr and similar ref-counts is if
_GLIBCXX_HAVE_ATOMIC_LOCK_POLICY changes from undefined to 1. What is the state
of that macro on RISC-V today?

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #8 from Andreas Schwab <schwab@linux-m68k.org> ---
It's #undef in 11.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Thanks. Changing that will cause an ABI break in the headers (and so affect
user code, not just the libstdc++.so library).

Clang and GCC will still be compatible, because the macros are still set once
by configure when building libstdc++.

One solution would be to override the checks in libstdc++-v3/acinclude.m4 so
that _GLIBCXX_HAVE_ATOMIC_LOCK_POLICY is also #undef for RISC-V, even after the
atomic built-ins are supported. That would preserve the ABI, but would mean
ref-counting in libstdc++ is sub-optimal.

Or let the default change, and vendors who want to preserve the old ABI can
configure with --with-libstdcxx-lock-policy=mutex to override the default.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[palmer at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #10 from palmer at gcc dot gnu.org ---
(In reply to Jonathan Wakely from comment #7)
> (In reply to Jonathan Wakely from comment #6)
> > (In reply to Jonathan Wakely from comment #5)
> > > (In reply to palmer from comment #3)
> > > > It looks like LLVM already has inline atomics, so presumably the same issues
> > > > would arise when mixing libstdc++ libraries compiled with LLVM and GCC.
> > > 
> > > Yup.
> > 
> > But not just "when mixing libstdc++ libraries". When mixing pretty much any
> > C++ code that uses libstdc++ headers.
> 
> Oh actually, sorry, that's wrong. The atomic policy for libstdc++ is set at
> configure time, based on the GCC building it. We define a macro, and that is
> fixed for the lifetime of that libstdc++ installation. So it doesn't matter
> if you compile those same headers with Clang, which _could_ use atomic
> built-ins, the atomic policy is still decided by the macro which doesn't
> change after installation.
> 
> So it's only a problem when mixing user code compiled with old and new
> libstdc++ headers.
> 
> And I've been confusing the _GLIBCXX_ATOMIC_BUILTINS macro with the
> _GLIBCXX_HAVE_ATOMIC_LOCK_POLICY macro.

I hadn't even noticed the _GLIBCXX_HAVE_ATOMIC_LOCK_POLICY macro, thanks!

> If _GLIBCXX_ATOMIC_BUILTINS changes from undefined to 1, I think that's OK.
> Old code will still call the non-inline functions in libstdc++.so, but those
> will now be consistent with the inline ones that new code is calling.

My specific worry was users mixing in routines from two different versions of
libstdc++: for example, maybe there's some statically linked executable that
puts a shared pointer into a mmap'd region, which it then expects to work when 

After signing off last night I realized that none of that would work anyway,
though, as even with the same library on both ends users would end up with a
different mutex and thus races.  So I think that one isn't worth worrying
about.

[snip, I'm mixing two replies here]

> Thanks. Changing that will cause an ABI break in the headers (and so affect
> user code, not just the libstdc++.so library).
> 
> Clang and GCC will still be compatible, because the macros are still set
> once by configure when building libstdc++.

>From my reading of this, GCC and clang will build libstdc++ binaries with
incompatible ABIs: clang has inline atomics, so the 2-byte CAS check will
succeed and we'll end up with libstdcxx_atomic_lock_policy=atomic .  I don't
actually have a clang build around to test that with, though, and I'm not sure
if folks are shipping clang-built libstdc++ anywhere (and if so, are expecting
it to be compatible with a GCC-built libstdc++).

> One solution would be to override the checks in libstdc++-v3/acinclude.m4 so
> that _GLIBCXX_HAVE_ATOMIC_LOCK_POLICY is also #undef for RISC-V, even after
> the atomic built-ins are supported. That would preserve the ABI, but would
> mean ref-counting in libstdc++ is sub-optimal.
> 
> Or let the default change, and vendors who want to preserve the old ABI can
> configure with --with-libstdcxx-lock-policy=mutex to override the default.

I guess I'm not really sure here: normally I'd say we're stuck with the default
being ABI compatible, but I don't know the rules in libstdc++.  I'm assuming
that forcing the default to be mutex could still allow users who want atomic to
configure with --with-libstdcxx-lock-policy=atomic, so at least there's a path
forward.  IIUC users will get link errors when moving between the two flavors
(the explicit template instantiations will have different integer values), so
at least there's a way for distros to make sure they've re-built everything if
they want to change.

I could also imagine a much more complicated third option: essentially
upgrading the mutex to an RW-like lock and allowing the atomic-based routines
to proceed concurrently.  I poked around the locking code a bit have no idea if
this would even be possible, it's all complicated enough that it seems like at
best a bad idea.

I guess this is really a distro sort of question, but I'd lean towards forcing
the default to mutex on RISC-V, thus keeping ABI compatibility.  Then at least
the distros can pick if they want to have a flag day around this.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to palmer from comment #10)
> After signing off last night I realized that none of that would work anyway,
> though, as even with the same library on both ends users would end up with a
> different mutex and thus races.  So I think that one isn't worth worrying
> about.

Agreed.

> I'm not
> sure if folks are shipping clang-built libstdc++ anywhere (and if so, are

They're not, and it's unsupported. Libstdc++ is part of GCC and must be built
by GCC.

> I guess I'm not really sure here: normally I'd say we're stuck with the
> default being ABI compatible, but I don't know the rules in libstdc++.  I'm
> assuming that forcing the default to be mutex could still allow users who
> want atomic to configure with --with-libstdcxx-lock-policy=atomic, so at

Yup.

> least there's a path forward.  IIUC users will get link errors when moving
> between the two flavors (the explicit template instantiations will have
> different integer values),

Not necessarily. The std::shared_ptr type isn't affected by that template
parameter, only its base class is. So APIs defined in terms of the public
std::shared_ptr type do not get linker errors if the private std::__shared_ptr
type differs.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:3fc22eedb033cbff1bf5a70c6a032d3c5ce2d867

commit r12-8161-g3fc22eedb033cbff1bf5a70c6a032d3c5ce2d867
Author: Palmer Dabbelt <palmer@rivosinc.com>
Date:   Thu Apr 14 17:22:58 2022 +0100

    libstdc++: Default to mutex-based atomics on RISC-V

    The RISC-V port requires libatomic to be linked in order to resolve
    various atomic functions, which results in builds that have
    "--with-libstdcxx-lock-policy=auto" defaulting to mutex-based locks.
    Changing this to direct atomics breaks the ABI, this forces the auto
    detection mutex-based atomics on RISC-V in order to avoid a silent ABI
    break for users.

    See Bug 84568 for more discussion.  In the long run there may be a way
    to get the higher-performance atomics without an ABI flag day, but
    that's going to be a much more complicated operation.  We don't even
    have support for the inline atomics yet, but given that some folks have
    been discussing hacks to make these libatomic routines appear implicitly
    it seems prudent to just turn off the automatic detection for RISC-V.

    libstdc++-v3/ChangeLog:

            * acinclude.m4 (GLIBCXX_ENABLE_LOCK_POLICY): Force auto to mutex
            for RISC-V.
            * configure: Regenerate.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[palmer at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #13 from palmer at gcc dot gnu.org ---
I just stumbled back into this one.  I think it's fixed?

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #14 from Andreas Schwab <schwab@linux-m68k.org> ---
Yes, with inline atomics no libatomic is needed any more.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at gcc dot gnu.org
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #15 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Per c#13 and c#14.

[Bug libstdc++/84568] libstdc++-v3 configure checks for atomic operations fail on riscv

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84568

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> ---
It's a shame the fix happened after the inferior shared_ptr implementation was
frozen into libstdc++ though :-(