Silence warnings about no-op alignment operations
ClosedPublic
Actions

Authored by markj on Feb 10 2021, 5:23 PM.

Details

Reviewers

arichardson
• hselasky

Commits

rG03bb5b837d89: mlx4, mthca: Silence warnings about no-op alignment operations
rG29ed53850e72: mlx4, mthca: Silence warnings about no-op alignment operations

Summary

In these cases the page size and firmware page size are the same, and
clang warns that the roundup operation is a no-op
(-Wtautological-compare). I can't see a way to fix this other than to
conditionally compile the code in question, or to disable the
diagnostic. Suggestions for alternate solutions are welcome.

BTW it is kind of perplexing to me that clang warns about a no-op here,
but the same expression performs a right-shift by zero and we don't get
a warning about that. Perhaps clang should be more accomodating here,
perhaps by providing a different warning flag? -Wtautological-compare
is generally useful I think.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36866
Build 33755: arc lint + arc unit

Event Timeline

markj created this revision.Feb 10 2021, 5:23 PM

Herald added a subscriber: imp. · View Herald TranscriptFeb 10 2021, 5:23 PM

markj requested review of this revision.Feb 10 2021, 5:23 PM

Harbormaster completed remote builds in B36866: Diff 83651.Feb 10 2021, 5:23 PM

markj added reviewers: arichardson, • hselasky.Feb 10 2021, 5:24 PM

That warning is my fault, possibly clang should have a -Wtautological-compare-align warning flag to turn this off.
I think using the pragma is the only way to silence it right now.

The only other option I can think of is an inline function wrapper:
https://godbolt.org/z/1cYc5G

In D28576#639718, @arichardson wrote:

That warning is my fault, possibly clang should have a -Wtautological-compare-align warning flag to turn this off.

It's conceivable that this warning could be useful, but it obviously isn't here given the comments.

I think using the pragma is the only way to silence it right now.

The only other option I can think of is an inline function wrapper:
https://godbolt.org/z/1cYc5G

I have no real preference either way. Using pragmas is a bit more surgical and I think I prefer that since I can't easily test these drivers.

It should probably be #pragma clang, I'm not sure GCC implements -Wtautological-compare (and it won't warn since it doesn't support the builtin yet).

In D28576#639730, @arichardson wrote:

It should probably be #pragma clang, I'm not sure GCC implements -Wtautological-compare (and it won't warn since it doesn't support the builtin yet).

Hmm, the amd64 xtoolchain gcc fails to build the kernel with a related error:

/usr/home/markj/src/freebsd-dev/sys/dev/firewire/fwohci.c:2699:17: error: 'typeof' applied to a bit-field                                                                                                                                                                                                                     
   r += roundup2(fp->mode.wreqb.len, sizeof(uint32_t));                                                                                                                                                                                                                                                                       
                 ^                                                                                                                                                                                                                                                                                                            
/usr/home/markj/src/freebsd-dev/sys/sys/cdefs.h:895:15: note: in definition of macro '__builtin_align_up'                                                                                                                                                                                                                     
  ((__typeof__(x))(((__uintptr_t)(x)+((align)-1))&(~((align)-1))))                                                                                             
               ^                                                                                                                                                                                                                                                                                                              
/usr/home/markj/src/freebsd-dev/sys/sys/param.h:310:24: note: in expansion of macro '__align_up'                                                                                                                                                                                                                              
 #define roundup2(x, y) __align_up(x, y) /* if y is powers of two */

@jhb noticed that too, and @jrtc27 suggested __typeof__(x+0) might be a possible workaround for GCC.

If these are preprocessor-time constants you could do #if MTHCA_ICM_PAGE_SIZE < PAGE_SIZE instead.

In D28576#639736, @jrtc27 wrote:

If these are preprocessor-time constants you could do #if MTHCA_ICM_PAGE_SIZE < PAGE_SIZE instead.

*_ICM_PAGE_SIZE is an enum. Initially I converted them to preprocessor constants but just decided to use the pragma since the prevailing style in these drivers is to use enum to define constants.