Re: enabling -Werror=assign-enum for kernel

  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.

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?

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.

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.

-- 
Gleb Smirnoff