Page MenuHomeFreeBSD

fwohci: Cast bitfield to uint32_t before passing it to roundup2().
ClosedPublic

Authored by jhb on Feb 11 2021, 10:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 23, 5:52 AM
Unknown Object (File)
Sun, Oct 20, 10:39 AM
Unknown Object (File)
Oct 1 2024, 3:17 PM
Unknown Object (File)
Sep 30 2024, 6:59 PM
Unknown Object (File)
Sep 24 2024, 7:17 PM
Unknown Object (File)
Sep 23 2024, 1:45 PM
Unknown Object (File)
Sep 22 2024, 7:43 PM
Unknown Object (File)
Sep 17 2024, 8:00 PM
Subscribers

Details

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 36906
Build 33795: arc lint + arc unit

Event Timeline

LGTM. We could also try the typeof(X+0) approach but if there's only one use that needs fixing adding this one cast seems better than potential unexpected side effects of changing the macro.

This revision is now accepted and ready to land.Feb 11 2021, 11:30 PM

LGTM. We could also try the typeof(X+0) approach but if there's only one use that needs fixing adding this one cast seems better than potential unexpected side effects of changing the macro.

One thing to keep in mind is that this macro is visible to userspace, so this may come up again.

The typeof(X+0) approach looks nicer to me. It's hard to imagine what hidden gotchas might arise with that, but we can also revisit it later if the need arises, so the patch seems fine to me.

I think the typeof doesn't work if you use it on a void *, i.e. void *p; roundup2(p, 16). Granted, that code doesn't work without a cast with the old roundup2() either, but this would also mean you would break uses of the newer __align_up() which does support void * in both of its current forms.

Patch itself looks fine.

It's unfortunate that gcc is so picky about this.

It's strange that roundup2() and roundup() have differences in terms of expression types--roundup2() is supposed just to be an optimized roundup(), right? One might think that the next step would be to make them consistent in expression types. Did you happen to take a look at what the fallout of requiring callers to cast would be for roundup()/rounddown()?

I'm not sure how much roundup/roundup2 are going to be used outside of the tree. On Linux other things are used (e.g. DIV_ROUND_UP is like howmany()), so I suspect they aren't really portable and aren't used widely outside of the base system. I haven't tried building world with GCC, but this was the only outlier I found in the kernel. I think if we find more than a handful of places we have to fix, then we will need make roundup2() use its own fallback instead of using __align_up's fallback, but I want to wait to see how many of those we find in practice.