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
