Page MenuHomeFreeBSD

Fix geom build with clang 17 and KTR enabled
ClosedPublic

Authored by dim on Sep 12 2023, 6:59 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Oct 16, 5:44 PM
Unknown Object (File)
Wed, Oct 16, 5:43 PM
Unknown Object (File)
Wed, Oct 16, 5:43 PM
Unknown Object (File)
Wed, Oct 16, 5:42 PM
Unknown Object (File)
Wed, Oct 16, 5:23 PM
Unknown Object (File)
Oct 6 2024, 7:25 AM
Unknown Object (File)
Oct 3 2024, 6:12 AM
Unknown Object (File)
Oct 2 2024, 9:45 PM
Subscribers

Details

Summary

When building a kernel with clang 17 and KTR enabled, such as with the
LINT configurations, a -Werror warning is emitted:

sys/geom/geom_io.c:145:31: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand]
  145 |         if ((KTR_COMPILE & KTR_GEOM) && (ktr_mask & KTR_GEOM)) {
      |             ~~~~~~~~~~~~~~~~~~~~~~~~ ^
sys/geom/geom_io.c:145:31: note: use '&' for a bitwise operation
  145 |         if ((KTR_COMPILE & KTR_GEOM) && (ktr_mask & KTR_GEOM)) {
      |                                      ^~
      |                                      &
sys/geom/geom_io.c:145:31: note: remove constant to silence this warning

Since the KTR_xxx constants and the ktr_mask variable are all intended
as bitmasks, use '&' instead.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 53563
Build 50454: arc lint + arc unit

Event Timeline

dim requested review of this revision.Sep 12 2023, 6:59 AM
dim created this revision.

is this the only occurence of this "error" in the tree?

this either should not be a warning altogether or the compile should ship a way to indicate "yes, this is what was meant".

In D41823#953284, @mjg wrote:

is this the only occurence of this "error" in the tree?

this either should not be a warning altogether or the compile should ship a way to indicate "yes, this is what was meant".

Yep, of course you can suppress the warning. But I think it makes sense to warn about mixing logical and bitwise operators. You could also write (KTR_COMPILE & KTR_GEOM) != 0 && (ktr_mask & KTR_GEOM) != 0) but that is effectively the same.

Maybe use == instead of & here. And maybe make is a macro?

So (a & b) == (c & b) or

A & b & c != 0

Since a & c & b & c == a & b & c

Which makes sense: if the compile enabled geom tr is there and we have it enabled at run time.

I suggested a macro/inline because I think this crosses the line from trivial check, which we inline directly to more tricky and difficult to get right, which we abstract out.

Replace multiple expressions with one macro, and use logical operands in
that macro.

if we're going to make a change like this any reason not to go all the way and just make it a static inline bool?

if we're going to make a change like this any reason not to go all the way and just make it a static inline bool?

A function, you mean? Or a variable? (Since ktr_mask is already a variable.)

I really think the better fix for this is to rework the CTRSTACK wrapper macro to include the call to stack_save() and for it to incorporate the compile and runtime check. It would make this geom code quite a bit cleaner.

sys/geom/geom_io.c
72

The comparisons of != 0 jointed with && is the right expression IMO.

This is fine for now though.

This revision is now accepted and ready to land.Sep 17 2023, 11:19 AM
This revision was automatically updated to reflect the committed changes.