Page MenuHomeFreeBSD

Add atomic and bswap functions to libcompiler_rt
ClosedPublic

Authored by dim on Aug 22 2020, 7:46 PM.

Details

Summary

There have been several mentions on mailing lists about missing atomic
functions in our system libraries (e.g. __atomic_load_8 and
friends), and recently I also saw __bswapdi2 and __bswapsi2
mentioned too.

We already have these as source files in our libcompiler_rt
Makefile.inc, but currently:

  • The atomic.c file is only enabled for 32 bit PowerPC, and only for clang (since gcc apparently already has it?). This was added by @alfredo in D22549 and committed by @jhibbits in rS356104.
  • The bswap[ds]i C implementations are only enabled for mips and riscv, apparently because "on some archs gcc 6.3 requires them".
  • The bswap[ds]i assembly implementations are only enabled for arm.

I think it would be nice to just always make both the atomic builtins
and the bswap functions available in our libcompiler_rt, at least in the
.a file. That would probably solve quite a lot of user complaints about
not being to able to compile ports, and having them switch to gcc
instead.

Note that with respect to compiler-rt's atomic.c, I am not totally
convinced that its implementation will work in all possible situations,
but having it it better than nothing...

Test Plan

I'm building a universe now, if that goes without any fallout, I see no
reason why it could not be committed. If there is fallout, we can
attempt to address it, and other remarks that reviewers have here.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 33120
Build 30487: arc lint + arc unit

Event Timeline

dim requested review of this revision.Aug 22 2020, 7:46 PM
dim created this revision.
lib/libcompiler_rt/Makefile.inc
222–225

So unfortunately this review doesn't change the broken situation for 32-bit mips, which was actually one of my intents:

ld: error: /home/dim/obj/head/home/dim/src/head/mips.mips/tmp/usr/lib/libc++.so.1: undefined reference to __atomic_fetch_add_8 [--no-allow-shlib-undefined]
ld: error: /home/dim/obj/head/home/dim/src/head/mips.mips/tmp/usr/lib/libc++.so.1: undefined reference to __atomic_load_8 [--no-allow-shlib-undefined]
ld: error: /home/dim/obj/head/home/dim/src/head/mips.mips/tmp/usr/lib/libc++.so.1: undefined reference to __atomic_fetch_sub_8 [--no-allow-shlib-undefined]

because obviously compiler-rt's atomic.c is skipped in favor of sys/mips/mips/stdatomic.c.

This latter file compiles to an empty object file, since it is entirely conditional on the define __SYNC_ATOMICS, and this doesn't appear to be defined. Furthermore, it appears to only implement those __sync_* functions, *not* the __atomic_ variants.

I guess we could just add compiler-rt's atomic.c unconditionally instead?

Simplify: always build atomic.c, and add a bsd.compiler.mk include to
correctly setup COMPILER_TYPE, otherwise the addition of the warning
option won't work as intended.

One minor question left now is of symbol versioning. GCC's libatomic.so
has LIBATOMIC_1.0, LIBATOMIC_1.1 and LIBATOMIC_1.2, and various symbols
appears under various versions. But I'm unsure whether we will need this
added complexity?

No success yet, for mips.mips this still gives rise to:

ld: error: /home/dim/obj/head/home/dim/src/head/mips.mips/tmp/usr/lib/libgcc_s.so: undefined reference to __atomic_is_lock_free [--no-allow-shlib-undefined]

so that's another problem to solve.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 25 2020, 6:49 AM
This revision was automatically updated to reflect the committed changes.

FWIW, the issues I've seen were on i386 (fixed by changing i386-gcc to default to "i686" the way we do for clang) and on risc-v which needs __atomic_exchange_1.

The versions in compiler_rt are not efficient in some cases (using locks, etc.). For example, for RISC-V the way to implement byte atomics is to do atomics on the containing word in a LR/SC loop where the byte in question is manipulated which will be much more efficient.