Page MenuHomeFreeBSD

lib/libc/string/ffs*.c: fix problems introduced with D40730
ClosedPublic

Authored by fuz on Jul 10 2023, 10:51 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jun 19, 3:29 AM
Unknown Object (File)
May 20 2024, 4:21 PM
Unknown Object (File)
May 4 2024, 9:02 PM
Unknown Object (File)
Apr 30 2024, 8:54 PM
Unknown Object (File)
Apr 24 2024, 1:52 AM
Unknown Object (File)
Feb 28 2024, 8:05 AM
Unknown Object (File)
Feb 11 2024, 5:20 AM
Unknown Object (File)
Feb 10 2024, 5:05 AM

Details

Summary

Gcc warns of infinite recursion if we use builtin_ffs*() to
implement ffs*(). This is because gcc uses ffs() to implement
these on some platforms. Sidestep the warning by using
builtin_ctz*() for these when compiling with gcc.

This patch could also be changed to always use builtin_ctz
as I had originally planned, but @mhorne recommended I use
builtin_ffs instead.

Also apply a patch sent to me by kevans@ to remove stale
dependencies on the old assembly ffs implementations.

I apologise of these errors. I am new to src development and
am not yet cognizant of all the possible error sources.

Sponsored by: FreeBSD Foundation
Reported by: jlduran@gmail.com, jhb
Fixes: ee8b0c43 (D40730)

Test Plan

manually tested that ffs.o, ffsl.o, and ffsll.o can
now build with gcc. Unfortunately I didn't get world to build
with gcc due to unrelated errors. Perhaps I just set it up
wrong. Would appreciate a pointer to some instructions on how
to build world with gcc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

fuz requested review of this revision.Jul 10 2023, 10:51 PM

Unfortunately I didn't get world to build
with gcc due to unrelated errors.

If the error you are seeing is: https://cirrus-ci.com/task/4693509148508160?logs=build_world#L8079, then it is indeed unrelated. I'll have to look into (probably D40780).

No, the error was about some missing symbols preventing a link of some atf test files.

I guess this is partially my oops! :)

Per jhb's comment on D40730, it sounds like it would be best to unconditionally use the ctz version.

Also, it is preferable to refer to the commit hash rather than the DR in the commit title+body.

The best changes are the ones which teach you more than you expect to learn, rather than the ones which go smoothly. Ultimately, no harm done here.

lib/libc/string/ffs*.c: unconditionally use __builtin_ctz

As recommended by @jhb and @mhorne

depend-cleanup.sh has been partially patched upstream, so you will want to rebase. I would use two separate commits, one for using __builtin_ctz* and one for updating depend-cleanup.sh to cover all of the functions, not just ffs.S.

This revision is now accepted and ready to land.Jul 12 2023, 8:08 PM

@jhb Which other functions are there to fix?

In D40966#934254, @fuz wrote:

@jhb Which other functions are there to fix?

I mean once you rebase to get the existing commits to depend-cleanup.sh you will find there's already a change to add ffs.S and you will still want to add all the others from your patch (fls* and ffsl*). I would just split those into two commits, one for the libc changes, and one for the updated patch to depend-cleanup.sh.

Okay, I see. Will proceed as recommended.