Re: git: cebb8646c4ee - main - Support byte-sized enums

On 7/5/23, Jessica Clarke <jrtc27@freebsd.org> wrote:
> On 5 Jul 2023, at 16:38, Mateusz Guzik <mjg@FreeBSD.org> wrote:
>>
>> The branch main has been updated by mjg:
>>
>> URL:
>> https://cgit.FreeBSD.org/src/commit/?id=cebb8646c4ee559aedcbc1b27bf30faa70a1f716
>>
>> commit cebb8646c4ee559aedcbc1b27bf30faa70a1f716
>> Author:     Mateusz Guzik <mjg@FreeBSD.org>
>> AuthorDate: 2023-03-12 19:29:20 +0000
>> Commit:     Mateusz Guzik <mjg@FreeBSD.org>
>> CommitDate: 2023-07-05 14:46:30 +0000
>>
>>    Support byte-sized enums
>>
>>    To that end add __enum_uint8_decl and __enum_uint8.
>>
>>    By default enums take 4 bytes, but vast majority of them have values
>>    which would fit in a byte.
>>
>>    One can try to workaround the problem by using bitfields, like so:
>>    enum some_small_enum foo:8;
>>
>>    but that's ugly and runs into trouble when using atomic_load (you
>> can't
>>    take an address of a bitfield, even if it is sized to a multiply of a
>>    byte).
>>
>>    Both gcc 13 and clang support specifying the size, and for older
>>    variants one can fallback to the "packed" attribute.
>>
>>    Names are mangled in order to avoid mix use with plain enum.
>>
>>    Reviewed by:
>>    Differential Revision:  https://reviews.freebsd.org/D39031
>> ---
>> sys/sys/types.h | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/sys/sys/types.h b/sys/sys/types.h
>> index 0846f2a57670..c6f7abe1f524 100644
>> --- a/sys/sys/types.h
>> +++ b/sys/sys/types.h
>> @@ -347,6 +347,18 @@ __makedev(int _Major, int _Minor)
>>    ((dev_t)(_Minor & 0xff00) << 24) | (_Minor & 0xffff00ff));
>> }
>>
>> +#if (defined(__clang__) || (defined(__GNUC__) && __GNUC__ >= 13))
>> +#define __enum_uint8_decl(name) enum enum_ ## name ## _uint8 : uint8_t
>> +#define __enum_uint8(name) enum enum_ ## name ## _uint8
>
> What happened to zlei@’s feedback about making it so __enum_uint8 isn’t
> needed everywhere?
>

The feedback got addressed before it got issued, both in the commit
message (pasted in the review) and my earlier comment. It explicitly,
albeit tersely, states the point was to disallow accidental size
mismatch.

Here is a trivial I ran into: there was struct xvnode containing 'enum
vtype vtype;' which was exported to userspace. Changing the size of
the enum changed the size in struct vnode (as excpected) and
uintentionally changed the size of the field in that struct xvnode as
well and there was no indication of it at compilation time. (Turns out
the struct was stale so it got whacked altogether btw.)

Making sure a resized enum is only accessible with dedicated naming
prevents the above kind of a problem from occurring and makes it clear
for all consumers what size they are getting.

>
>> +#else
>> +/*
>> + * Note: there is no real size checking here, but the code below can be
>> + * removed once we require GCC 13.
>> + */
>> +#define __enum_uint8_decl(name) enum __attribute__((packed)) enum_ ##
>> name ## _uint8
>> +#define __enum_uint8(name) enum __attribute__((packed)) enum_ ## name ##
>> _uint8
>> +#endif
>> +
>> /*
>>  * These declarations belong elsewhere, but are repeated here and in
>>  * <stdio.h> to give broken programs a better chance of working with
>
>


-- 
Mateusz Guzik <mjguzik gmail.com>