Re: enabling -Werror=assign-enum for kernel

On 8/26/26 13:23, Gleb Smirnoff wrote:
>    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?

No, it won't change anything, but over time we might find we have to convert some
enums to constants.  I would only do those on an as-needed basis though, I wouldn't
go about doing it now.  I might suggest that we should avoid adding _new_ enums
that are treated as flags.

> 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?

__flag_enum seems fine to me.  sys/cdefs.h is full of these types of things already.

> 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.

Eh, I think the namespacing is fine.  We could perhaps make use of
__constructor though (I think hps@ even had a prototype of this) which
lets the linker do the sorting for us.  You'd have to use a helper macro to map
SI_SUB_FOO and SI_ORDER_FOO into a single integer, but that's not the end of the
world.

-- 
John Baldwin