Re: enabling -Werror=assign-enum for kernel

On 8/26/26 13:02, Gleb Smirnoff wrote:
>    John,
> 
> On Wed, Aug 26, 2026 at 09:17:36AM -0400, John Baldwin wrote:
> J> > modern clang has a nice feature: it allows to catch enum misuse.  For example:
> J> >
> J> > enum foo { A = 1, B } var;
> J> > var = 1;	/* success */
> J> > var = A;	/* success */
> J> > var = B;	/* success */
> J> > var = 3;	/* fail */
> J> >
> J> > What about enums that are used as flags? For that it has
> J> > __attribute__((flag_enum)).  This will allow using combination of values and
> J> > clearing them:
> J> >
> J> > var = A | B;	/* success */
> J> > var &= ~A;	/* success */
> J> > var = 3;	/* fail */
> J>
> J> Note that C++ doesn't allow this at the language level.  When I converted ctld to
> J> C++ I had to adjust some enums to be simple #define's instead in the ctl headers.
> 
> Noted.  I can't make a judgement if it is a smart restriction by C++ or not.
> In our case using enums as flags is common and handy and Werror=assign-enum in
> combination with __attribute__((flag_enum)) will make this use fortified
> against mistakes.

Mostly my point is that over time we may be forced to convert away from enums to
plain constants if more of the base system starts using C++ anyway.  At least for
enums exposed to userspace.

> Question: what is our policy of adding attributes into code?  The convetion is
> that compilers warn and don't fail on unknown attributes, so adding them as
> bare words definitely works.  I personally prefer to avoid use of preprocessor
> when it is possible, so I'm for adding the __attribute__((flag_enum)) as is.
> However, few people who compile with gcc may be unhappy about lots of warnings.
> The best practice to shut these warning is:
> 
> #ifndef __flag_enum
> #ifdef __clang__
> #define	__flag_enum	__attribute__((flag_enum))
> #else
> #define	__flag_enum
> #endif
> #endif
> 
> This would require us use FreeBSD-specific keyword in the code.  Which one is
> the preferred way to move forward?

Normally we define a new wrapper in <sys/cdefs.h> so that the only #ifdef's are in
that header, and the rest of the code uses the wrapper macro unconditionally.

> J> > This seems like a nice enforcement of a good code.  I already found a few
> J> > hamrless bugs with it and one in e1000 that could be a real bug.  It also
> J> > highlights some sketchy code that better be refactored.
> J> >
> J> > I have a branch where LINT is compilable with -Werror=assign-enum:
> J> >
> J> > https://github.com/freebsd/freebsd-src/compare/main...glebius:FreeBSD:Werror%3Dassign-enum
> J> >
> J> > There are basically three parts that require work:
> J> >
> J> > 1) The most common violators are SYSINIT(9)s that use (SI_SUB_FOO + 1) as
> J> > argument.  I have posted a pack of reviews to cover that.
> J>
> J> So I think this is probably sensible for SI_SUB_* to define new values.  I
> J> worry about SI_ORDER_*.  Possibly we should use plain #define's for SI_ORDER_*
> J> instead of an enum if we aren't already.  I wonder if you ran into any cases
> J> where we use an SI_ORDER_* value that is an expression?
> 
> Nope.  Everything in SYSINIT(9) that was violating Werror=assign-enum is
> already in main, except ipfw.  One bit left is ipfw's sysinits, which I left
> for later, since I wanted more changes to it.
> 
> I don't agree we should use plain #define for SI_ORDER_*.  The current code
> combined with Werror=assign-enum will enforce intentionality and thoughtfulness
> when adding new sysinits.  Would prevent quick adhoc additions in the "works
> for me" style.

At some point 'SI_ORDER_<ENGLISH ORDINAL TERM>' isn't really more readable than
just using an integer.  In particular for offsets relative to SI_ORDER_FIRST.
Today we probably don't have more than SI_ORDER_THIRD, but something like
SI_ORDER_FOUR_HUNDRED_FIFTY_SEVENTH would be a mouthful compared to
'SI_ORDER_FIRST + 456'.  For SI_SUB_* the names are generally descriptive of
some subsystem, but SI_ORDER are inherently a range of explicitly numbered values
along with a few special cases such as SI_ORDER_ANY/LAST.  The special cases
make sense as names.  The numeric range makes less sense as names.  However, if
we all fit fine today with just SI_ORDER_THIRD then it's probably ok.  I worried
we might have a broader range than just FIRST/SECOND/THIRD.

> J> > 2) hwpmc has enum pmc_event that is generated by preprocessor and its use is
> J> > sketchy,    thus files that utilize HWPMC_HOOKS are temporarily excluded from
> J> > the enforcements.
> J> >
> J> > 3) I was too tired to look at OFED, thus just disabled it in ${OFED_C} in
> J> > kern.mk
> J>
> J> Also, we should avoid gratuitous diffs to OFED since it is really contrib code,
> J> so your approach here is fine.  OTOH, note that other Linuxy bits like the wifi
> J> drivers and drm drivers might also need to opt out of this via CFLAGS.
> 
> I touched ZFS though.  It appeared to be 100% clean and pretty.  Just adding
> __attribute__((flag_enum)) to all enums that are used as flags was enough.
> Zero other corrections, zero failures.  I will look into upstreaming the ZFS
> part when we got overall agreement that we enable Werror=assign-enum for the
> kernel.

Yes but I wouldn't want to require getting Linux as a whole to adopt this as a
prerequisite for us to adopt it, and local diffs we have to OFED make future
merges a headache so should be avoided when possible.  Some day I'd like to do
true vendor imports into sys/contrib/rdma or the like, but OFED wasn't maintained
that way in the past and is a giant rats nest of partially merged bits and missing
bug fixes, etc.  It's a bit of work to get to a more maintainble thing, and needless
local diffs only make that work even harder.

-- 
John Baldwin