Re: enabling -Werror=assign-enum for kernel

  John,

On Wed, Aug 26, 2026 at 01:15:27PM -0400, John Baldwin wrote:
J> > J> Note that C++ doesn't allow this at the language level.  When I converted ctld to
J> > J> C++ I had to adjust some enums to be simple #define's instead in the ctl headers.
J> > 
J> > Noted.  I can't make a judgement if it is a smart restriction by C++ or not.
J> > In our case using enums as flags is common and handy and Werror=assign-enum in
J> > combination with __attribute__((flag_enum)) will make this use fortified
J> > against mistakes.
J> 
J> Mostly my point is that over time we may be forced to convert away from enums to
J> plain constants if more of the base system starts using C++ anyway.  At least for
J> enums exposed to userspace.

But if C++ doesn't allow flag enums in principle, then they just can't go into
userspace headers, no matter how we compile the kernel.  Basically we already
are there, and enabling Werror=assign-enum for kernel won't change anything,
will it?

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

So your answer is that I need to add the above to <sys/cdefs.h> and rewrite my
branch to use a defined keyword instead of __attribute__((flag_enum))?  What
would be the keyword? __flag_enum or any other spelling?

J> > J> > This seems like a nice enforcement of a good code.  I already found a few
J> > J> > hamrless bugs with it and one in e1000 that could be a real bug.  It also
J> > J> > highlights some sketchy code that better be refactored.
J> > J> >
J> > J> > I have a branch where LINT is compilable with -Werror=assign-enum:
J> > J> >
J> > J> > https://github.com/freebsd/freebsd-src/compare/main...glebius:FreeBSD:Werror%3Dassign-enum
J> > J> >
J> > J> > There are basically three parts that require work:
J> > J> >
J> > J> > 1) The most common violators are SYSINIT(9)s that use (SI_SUB_FOO + 1) as
J> > J> > argument.  I have posted a pack of reviews to cover that.
J> > J>
J> > J> So I think this is probably sensible for SI_SUB_* to define new values.  I
J> > J> worry about SI_ORDER_*.  Possibly we should use plain #define's for SI_ORDER_*
J> > J> instead of an enum if we aren't already.  I wonder if you ran into any cases
J> > J> where we use an SI_ORDER_* value that is an expression?
J> > 
J> > Nope.  Everything in SYSINIT(9) that was violating Werror=assign-enum is
J> > already in main, except ipfw.  One bit left is ipfw's sysinits, which I left
J> > for later, since I wanted more changes to it.
J> > 
J> > I don't agree we should use plain #define for SI_ORDER_*.  The current code
J> > combined with Werror=assign-enum will enforce intentionality and thoughtfulness
J> > when adding new sysinits.  Would prevent quick adhoc additions in the "works
J> > for me" style.
J> 
J> At some point 'SI_ORDER_<ENGLISH ORDINAL TERM>' isn't really more readable than
J> just using an integer.  In particular for offsets relative to SI_ORDER_FIRST.
J> Today we probably don't have more than SI_ORDER_THIRD, but something like
J> SI_ORDER_FOUR_HUNDRED_FIFTY_SEVENTH would be a mouthful compared to
J> 'SI_ORDER_FIRST + 456'.  For SI_SUB_* the names are generally descriptive of
J> some subsystem, but SI_ORDER are inherently a range of explicitly numbered values
J> along with a few special cases such as SI_ORDER_ANY/LAST.  The special cases
J> make sense as names.  The numeric range makes less sense as names.  However, if
J> we all fit fine today with just SI_ORDER_THIRD then it's probably ok.  I worried
J> we might have a broader range than just FIRST/SECOND/THIRD.

So far we are good with FIRST/SECOND/THIRD.  IMHO, if we really really want to
improve SYSINIT substantially, not just fix daily problems, we should
unironically use rcorder(8) as kernel compilation tool.

-- 
Gleb Smirnoff