ffs*.c: fix problems introduced with D40730
ClosedPublic
Actions

Authored by fuz on Jul 10 2023, 10:51 PM.

Details

Reviewers

kevans
mhorne
jhb

Commits

rG3f5788e0ed8e: lib/libc/string/ffs*.c: work around gcc warning
rGe74bc7757a2e: lib/libc/string/ffs*.c: fix problems introduced with D40730

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 created this revision.Jul 10 2023, 10:51 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 10 2023, 10:51 PM

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

Harbormaster completed remote builds in B52551: Diff 124463.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

Harbormaster completed remote builds in B52606: Diff 124589.Jul 12 2023, 7:24 PM

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.

Closed by commit rGe74bc7757a2e: lib/libc/string/ffs*.c: fix problems introduced with D40730 (authored by kevans, committed by fuz). · Explain WhyJul 14 2023, 8:28 PM

This revision was automatically updated to reflect the committed changes.

fuz added a commit: rGe74bc7757a2e: lib/libc/string/ffs*.c: fix problems introduced with D40730.

fuz added a commit: rG3f5788e0ed8e: lib/libc/string/ffs*.c: work around gcc warning.

Landed as:

fuz mentioned this in rG2c03221cce0b: lib/libc/string: replace ffs/fls implementations with clang builtins.Jul 21 2023, 8:56 AM