Page MenuHomeFreeBSD

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

Authored by jhb on Thu, Feb 11, 10:10 PM.

Details

Summary

The fallback for align_up() used by roundup2() uses typeof__()
which doesn't work for bitfields. This fixes the build on GCC which
uses the fallback.

Diff Detail

Repository
R10 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

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.Thu, Feb 11, 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.