Page MenuHomeFreeBSD

Add a weak alias from ffs() to __ffssi2() to pacify modern GCC.
ClosedPublic

Authored by jhb on Mar 21 2017, 10:36 PM.

Details

Summary

Add a weak alias from ffs() to __ffssi2() to pacify modern GCC.

GCC 6.2 emits calls to __ffssi2() when compiling libc and some userland
programs in the base system.

Suggested by: jmallett

There may be better ways of doing this. In particular, GCC 6.2 will
probably act the same way when compiling 3rd party applications, so a
symbol in FBSD_private doesn't really seem correct. It seems odd that
GCC 6.2 is even calling __ffssi2() given that older versions did not.

Test Plan
  • compile and run o32 and n64 kernel + worlds in QEMU with GCC 6.2.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Mar 21 2017, 10:36 PM
jmallett edited edge metadata.Mar 21 2017, 10:47 PM

This looks reasonable, but I share the confusion about correctness. It seems like this should be in libgcc generally. I know we spoke about this, but I've forgotten; can you remind me why libgcc isn't providing this symbol?

jhb added a comment.Mar 21 2017, 11:37 PM

Probably because mips-gcc from ports doesn't provide a mips libgcc that we can link against. Instead, the options we have are the older gcc 4.2 libgcc in base or libcompiler_rt from the llvm folks. Looks like GCC might have added this in GCC 4.3:

%inherit GCC_4.3.0 GCC_4.2.0
GCC_4.3.0 {
  # byte swapping routines
  __PFX__bswapsi2
  __PFX__bswapdi2

  __emutls_get_address
  __emutls_register_common
  __PFX__ffssi2

We could perhaps add it to libcompiler_rt. For some reason this seemed more complex when I looked before, but we could use ffsdi2.c as a basis. I think it would just take a 'si_int' and would just need to use __builtin_ctz() on the value after checking for 0.

kan edited edge metadata.Mar 23 2017, 6:05 PM

This looks reasonable, but I share the confusion about correctness. It seems like this should be in libgcc generally. I know we spoke about this, but I've forgotten; can you remind me why libgcc isn't providing this symbol?

Yes, this does not belong in libc and should be provided with compiler_rt. As long as libcompiler_rt pretends to be libgcc and as long as we insist shipping our own libgcc even when external toolchain is used, the responsibility for keeping our libgcc usable by said external toolchain is on us. This is a losing proposition in a long term, IMHO.

jhb updated this revision to Diff 26998.Apr 3 2017, 10:22 PM
  • Add __ffssi2() to libcompiler_rt.
  • Drop __ffssi2() alias from libc.
  • Use builtin_ctz() not ctzsi2() directly.
jhb added a comment.Apr 3 2017, 10:28 PM

I've reworked this to add the symbol to libcompiler_rt instead. It was derived from the existing ffsdi2 (but simpler). Would be nice to upstream this if possible?

jhb added a reviewer: dim.Apr 3 2017, 10:28 PM

Added dim@

jmallett accepted this revision.Apr 4 2017, 2:05 AM

Awesome, thank you for implementing it in the right place! And we're confident this doesn't produce any referential loops, right? Where builtin_ctz would generate a call to ffssi2?

This revision is now accepted and ready to land.Apr 4 2017, 2:05 AM
jhb added a comment.Apr 4 2017, 3:51 PM

Yes, __builtin_ctz() already has special magic in place for MIPS to avoid recursing.

dim accepted this revision.Apr 4 2017, 10:25 PM

Yeah, this looks OK to me. I'll submit it upstream. One question is if it should be *only* enabled for mips, or in general?

jhb added a comment.Apr 5 2017, 1:41 AM
In D10086#212578, @dim wrote:

Yeah, this looks OK to me. I'll submit it upstream. One question is if it should be *only* enabled for mips, or in general?

Looking at the libgcc sources from /usr/ports/lang/gcc it seems to always be present:

# Library members defined in libgcc2.c.
lib2funcs = _muldi3 _negdi2 _lshrdi3 _ashldi3 _ashrdi3 _cmpdi2 _ucmpdi2    \
            _clear_cache _trampoline __main _absvsi2 \
            _absvdi2 _addvsi3 _addvdi3 _subvsi3 _subvdi3 _mulvsi3 _mulvdi3 \
            _negvsi2 _negvdi2 _ctors _ffssi2 _ffsdi2 _clz _clzsi2 _clzdi2  \
            _ctzsi2 _ctzdi2 _popcount_tab _popcountsi2 _popcountdi2        \
            _paritysi2 _paritydi2 _powisf2 _powidf2 _powixf2 _powitf2      \
            _mulsc3 _muldc3 _mulxc3 _multc3 _divsc3 _divdc3 _divxc3        \
            _divtc3 _bswapsi2 _bswapdi2 _clrsbsi2 _clrsbdi2
This revision was automatically updated to reflect the committed changes.