Add atomic and bswap functions to libcompiler_rt
ClosedPublic
Actions

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

Details

Reviewers

alfredo
bdragon
emaste
imp
jhb
jhibbits
luporl

Commits

rGb148c7b87148: Add atomic and bswap functions to libcompiler_rt
rG7b67d47c70cc: MFC r364753: Add atomic and bswap functions to libcompiler_rt
rS365721: MF12 r365661:
rS365661: MFC r364753:
rS365472: Revert r365471 as it is breaking with old gcc on various arches:
rS365471: MFC r364753:
rS364753: Add atomic and bswap functions to libcompiler_rt

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

dim requested review of this revision.Aug 22 2020, 7:46 PM

dim created this revision.

Harbormaster completed remote builds in B33108: Diff 76093.Aug 22 2020, 7:46 PM

dim edited the summary of this revision. (Show Details)Aug 22 2020, 7:46 PM

dim added inline comments.Aug 23 2020, 11:07 AM

lib/libcompiler_rt/Makefile.inc
215 ↗	(On Diff #76093)	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?

Harbormaster completed remote builds in B33120: Diff 76108.Aug 23 2020, 7:10 PM

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

Closed by commit rS364753: Add atomic and bswap functions to libcompiler_rt (authored by dim). · Explain Why

This revision was automatically updated to reflect the committed changes.

dim added a commit: rS364753: Add atomic and bswap functions to libcompiler_rt.

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.

dim added a commit: rS365471: MFC r364753:.Sep 8 2020, 8:02 PM

dim added a commit: rS365472: Revert r365471 as it is breaking with old gcc on various arches:.Sep 8 2020, 8:28 PM

dim added a commit: rS365661: MFC r364753:.Sep 12 2020, 4:33 PM

dim added a commit: rS365721: MF12 r365661:.Sep 14 2020, 2:52 PM

emaste added a commit: rG7b67d47c70cc: MFC r364753: Add atomic and bswap functions to libcompiler_rt.Jan 29 2021, 12:19 AM

dim added a commit: rGb148c7b87148: Add atomic and bswap functions to libcompiler_rt.Dec 22 2021, 10:08 AM