[Bug libgcc/105708] New: libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[Bug libgcc/105708] New: libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[keno at juliacomputing dot com]

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

            Bug ID: 105708
           Summary: libgcc: aarch64: init_lse_atomics can race with
                    user-defined constructors
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libgcc
          Assignee: unassigned at gcc dot gnu.org
          Reporter: keno at juliacomputing dot com
  Target Milestone: ---

Recent gcc versions provide the `-moutline-atomics` option that outlines
aarch64 atomics into calls to libgcc that dispatch to either lse atomics or
legacy ll/sc atomics depending on the availability of the feature on the target
platform.

This is useful for performance (since lse atomics have better performance
characteristics), but also for projects like the rr (https://rr-project.org/)
userspace record-and-replay debugger, which emulates an aarch64 machine without
ll/sc intrinsics (because ll/sc introduces non-deterministic control flow
divergences that rr cannot record).

The feature detection uses the following function in libgcc
(config/aarch64/lse-init.c):
```
static void __attribute__((constructor))
init_have_lse_atomics (void)
{
  unsigned long hwcap = __getauxval (AT_HWCAP);
  __aarch64_have_lse_atomics = (hwcap & HWCAP_ATOMICS) != 0;
}
```

Unfortunately, the order of this `init_have_lse_atomics` is not defined with
respect to other uses of `__attribute__((constructor))`. As a result, other
constructors using atomics may end up using ll/sc instructions, even if lse
atomics are supported on the target (usually only a performance penalty, but as
mentioned above, a significant concern for projects like rr).

Worse, the initialization order can change, with minor changes in the
environment. E.g. recent binary builds of debian testing initialize lse too
late in libpthread, breaking rr. Earlier builds had the opposite initialization
order allowing rr to work without issue.

I can see two possibilities to introduce more determinism here:
1. Use `__attribute__((constructor(100)))` (100 being the system library
priority used e.g. in libstdc++ as well) for `init_have_lse_atomics`, forcing a
deterministic initialization order wrt user (or libc)-defined constructors.
There are still ordering concerns wrt libstdc++ which also uses init_order 100,
but as far as I can tell does not use atomics in these constructors. If this
changes, the priority here could be further reduced in future iterations of
libgcc.

2. Switch the outlined atomics to probe lse support on first use rather than
using a constructor. If this is possible, I think it would be preferable to
avoid any possibility of initialization order problems, but I understand that
there may be code size and performance concerns.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |WONTFIX
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
> which emulates an aarch64 machine without ll/sc intrinsics (because ll/sc introduces non-deterministic control flow divergences that rr cannot record).

That is not emulating an aarch64 machine at all, since ll/sc is required to be
there for ARMv8-a. RR needs to be able to emulate those also.

I am going to close it as won't fix as the problem is with the rr emulator
which needs to emulate ll/sc for correctness.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There is no correctness issue here either since the check for atomics always
fall back to ll/sc style atomics if init_have_lse_atomics has not been run.
Once init_have_lse_atomics runs then it will use the ARMv8.1-a LSE atomics but
both are correct and still valid to use.

There are some use cases in even in userspace where you want to use both style
atomics (mostly with wfe really).

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[keno at juliacomputing dot com]

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

--- Comment #3 from Keno Fischer <keno at juliacomputing dot com> ---
> I am going to close it as won't fix as the problem is with the rr emulator which needs to emulate ll/sc for correctness.

No currently shipping aarch64 chip provides hardware support that would allow
such a capability (believe me, we've tried and have and many conversations with
the hardware vendors about it). I would like to strongly request that the
WONTFIX be reconsidered here. It's a relatively simple fix, that should not
have any downsides. If this is not fixed upstream in libgcc then distributions
that want to provide rr will need to start patching libgcc downstream or find
another solution, which will just be a huge mess.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
rr needs to be able to emulate ll/sc really. If it can't then there is not much
GCC can do because again people can and do use both in their programs.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[keno at juliacomputing dot com]

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

--- Comment #5 from Keno Fischer <keno at juliacomputing dot com> ---
Yes, rr cannot record ll/sc. I'm happy to go into depth here, but this is not
really an aarch64 issue. rr doesn't work on ppc64le either for this reason. The
introduction of lse has made rr feasible on aarch64, and there has been a
substantial effort to get to this point where rr is working on the
architecture. We're now working through the distribution issues, where this
cropped up (as mentioned, we didn't notice earlier, because the initialization
is not deterministic). For the moment, we're telling downstream users to avoid
manual use of ll/sc in programs that they want to record under rr. Obviously
this is a significant effort, but for many people it's worth it, because rr is
a critical tool. Perhaps in future hardware iterations, we'll get the ability
to fault on stxr abort or similar, which would allow rr to support ll/sc, but
until then we need to make due with what we have.

The issue here is that `-moutline-atomics` now introduces extra ll/sc
instructions even in software where the implementer was careful to avoid manual
uses of ll/sc and in particular also in system libraries like libc and rtld
that the user may have little control over. Of course we can keep telling
people to build their distribution images with `-march=armv8.3-a
-mno-outline-atomics` and avoid this issue or have them patch libgcc
downstream, but that really seems to defeat the point of `-moutline-atomics`,
which was exactly to avoid this kind of split.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[pinskia at gcc dot gnu.org]

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

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Again the bug is still in RR and RR should figure out a better way of fixing
this. If RR wants to support aarch64, they need to fix their code. I am sorry
but ARMv8-a requires ll/sc and any code which uses ll/sc which fail when using
rr, the user will complain and you should just declare rr being broken.

I am sorry but RR is non-complaint and is just broken.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[keno at juliacomputing dot com]

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

--- Comment #7 from Keno Fischer <keno at juliacomputing dot com> ---
I'm probably talking in circles at this point, but let me try just one more
time. I just want to clarify that I understand that this is not technically a
libgcc bug. But it's not really an rr bug either. Existing hardware already
barely supports rr and we're still working with a number of silicon vendors to
address microarchitecture bugs that are causing issues. We'll also keep
advocating for hardware support for ll/sc emulation/trapping, but as you can
imagine that's a big lift. It's not a matter of rr just fixing some code - the
hardware support required for this does not currently exist. These limitations
are proactively communicated to the user and there are appropriate error
messages for unsupported situations (e.g. use of a broken microarchitecture or
recording of an application that used ll/sc). I was hoping by making this
change in libgcc we could increase the number of situations where rr does work,
without putting significant extra burden on the distribution maintainers. That
said, if this really is a hard WONTFIX here, then we'll work with the
distribution maintainers to start carrying a libgcc patch as necessary :(. It's
gonna be messy and I'm not looking forward to it, but I don't really know what
else to say to make my case here.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[pinskia at gcc dot gnu.org]

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

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Oh one more note, the performance of the initialization code is going to be
very minor. So fixing this won't change the overall performance.
This is again a minor issue and you still might end up with LL/SC style atomics
in userland code, especially if you use older compilers or use golang or use
clang/LLVM. Or use a custom compiler.
Again this is a bug in RR in that it cannot handle LL/SC style atomics.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[roc at ocallahan dot org]

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

roc at ocallahan dot org <roc at ocallahan dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |roc at ocallahan dot org

--- Comment #9 from roc at ocallahan dot org <roc at ocallahan dot org> ---
It is a limitation of rr that we can't handle LL/SC, but it's not a bug we can
just fix or work around. There is just no way to handle it in rr without
hardware changes.

Thus, if we can't find a way around this bug here, many Aarch64 users won't be
able to use rr when they otherwise would have been able to. That would be
unfortunate.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[wilco at gcc dot gnu.org]

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

Wilco <wilco at gcc dot gnu.org> changed:

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

--- Comment #10 from Wilco <wilco at gcc dot gnu.org> ---
Increasing the priority of the constructor is perfectly reasonable given that
it has no effect on correctness and doing it as early as possible is better for
performance if other constructors use atomics.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
How can changing the constructor priority in libgcc affect anything?
Constructor priorities are within the same shared library or within the same
executable, not inside of the same process.  So, e.g. when using libgcc_s.so.1,
it might change order with other constructors inside of that shared library
(there are likely none), but nothing else.  For libgcc.a, it might affect even
ctors of the other objects with which the library is linked, but still not
between different shared libraries or binaries.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[wilco at gcc dot gnu.org]

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

--- Comment #12 from Wilco <wilco at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #11)
> How can changing the constructor priority in libgcc affect anything?
> Constructor priorities are within the same shared library or within the same
> executable, not inside of the same process.  So, e.g. when using
> libgcc_s.so.1,
> it might change order with other constructors inside of that shared library
> (there are likely none), but nothing else.  For libgcc.a, it might affect
> even ctors of the other objects with which the library is linked, but still
> not between different shared libraries or binaries.

The outline atomics are linked with each .so (to avoid the PLT and GOT), so
there are multiple copies and any initialization order issues are within the
.so.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[wilco at gcc dot gnu.org]

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

Wilco <wilco at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|RESOLVED                    |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |wilco at gcc dot gnu.org
   Last reconfirmed|                            |2022-05-24
         Resolution|WONTFIX                     |---

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Wilco Dijkstra <wilco@gcc.gnu.org>:

https://gcc.gnu.org/g:75c4e4909ae2667f56487434f99c2915b4570794

commit r13-764-g75c4e4909ae2667f56487434f99c2915b4570794
Author: Wilco Dijkstra <wilco.dijkstra@arm.com>
Date:   Wed May 25 14:29:03 2022 +0100

    AArch64: Prioritise init_have_lse_atomics constructor [PR 105708]

    Increase the priority of the init_have_lse_atomics constructor so it runs
    before other constructors. This improves chances that rr works when LSE
    atomics are supported.

    libgcc/
            PR libgcc/105708
            * config/aarch64/lse-init.c: Increase constructor priority.

[Bug libgcc/105708] libgcc: aarch64: init_lse_atomics can race with user-defined constructors

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
   Target Milestone|---                         |13.0
             Status|ASSIGNED                    |RESOLVED

--- Comment #14 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Fixed.