Support byte-sized enums

[mjg]

Summary

commit 31dab65916568140c3921ef1599e002315ca95bd
Author: Mateusz Guzik <[e-mail]>
Date:   Sun Mar 12 19:31:27 2023 +0000

    vfs: use __enum_uint8 for vtype and vstate
    
    Reviewed by:
    Differential Revision:

commit 73129bc43e901a063a0a2fa3ec024a53b773a83a
Author: Mateusz Guzik <[e-mail]>
Date:   Sun Mar 12 19:29:20 2023 +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

commit 93f3e9dba8b886058964c7acbcc6ce170a1d1f6a
Author: Mateusz Guzik <[e-mail]>
Date:   Fri Apr 21 18:06:13 2023 +0000

    vfs: list enums on separate lines
    
    Requested by:   kib

[mjg]

sys/sys/namei.h

44 ↗

(On Diff #118690)

ignore this bit

[arichardson]

Using the C23 underlying enum type extension is IMO better and safer. Support with clang (since version 8) and GCC 13+: https://godbolt.org/z/n4EshMY1j

[mjg]

This is not a strict equivalent of what I proposed here (my stuff resizes as needed), but would also to the job and I don't care past that how it looks like.

The goal is to let v_type and v_state by byte-sized enums without being bitfields.

Feel free to commandeer the review or post your own, in which case I'll abandon this one

[mjg]

ping?

i also note "byte num" is a bad name, how do you call a variant which takes 2? :)

[mjg]

• use the new c support

[emaste]

Not an expert but it seems reasonable to me

[mjg]

sys/kern/vfs_cache.c

3680

i think it is time for atomic_load which figures out the size btw

[kib]

sys/kern/vfs_cache.c

3680

Or, for the sake of this commit, add atomic_load_enum_uint8

sys/sys/cdefs.h

935

The general intent with sys/cdefs.h is to provide common facilities to adjust to compiler quirks, for any compilation environment. Also, you explicitly use types (uint8_t) provided by cdefs.h conumers, while sys/cdefs.h is supposed to be self-contained: it is the root of all header dependencies.

Put your hack into implementation namespace (prepend with two _) and find a better way to deal with type dependency.

945

sys/sys/vnode.h

59

Can you reformat these to have one symbol per-line?

[mjg]

sys/sys/cdefs.h

935

perhaps this should land in sys/types.h then? again under _KERNEL

935

point being, arguably it is a new type

[kib]

sys/sys/cdefs.h

935

sys/types.h indeed sound not bad, but I do not agree with the _KERNEL braces. The type name should be put into impl namespace, and be accessible to any consumer. Note that we have usermode that wants the struct vnode definition.

[arichardson]

I think the second macro is unnecessary based on my experiments with godbolt. Otherwise LGTM once we've decided where to place it (and I'd suggest with the _KERNEL guard).

sys/sys/cdefs.h

938

Plain "enum" appears to work fine with both.

945–946

949

I looks like this is unnecessary, GCC 9 only needs it on the enum def, uses will automatically be sized correctly:

https://godbolt.org/z/PvW9s5obx

sys/sys/vnode.h

114

Plain enum should be fine.

[mjg]

• move to types.h
• force a unique name

[mjg]

-

sys/sys/vnode.h

59

i can do it if you insist but i don't see much point

114

plain enum should fail to compile so that it is apparent everywhere what the size is

[kib]

sys/sys/vnode.h

59

I do think that one line per symbol improves the code layout and makes future diffs smaller. Also, I think you could do

   ...
   VMARKER,
   VLASTTYPE = VMARKER,
};

[arichardson]

I'd prefer if the __enum_uint8(vtype) changes were only inside structs and not in the function signatures but I don't feel strongly about it.

sys/sys/vnode.h

114

If the definition is visible the compiler knows the size so plain enum does the right thing. If you prefer it this way to be explicit that's fine by me although I'd limit the change to just the struct definition, in the function signatures it's unnecessary IMO.

[mjg]

ping? this is the last call before api freeze

[zlei]

sys/sys/types.h

351 ↗

(On Diff #120824)

Can we respect the original name so that we can use enum name directly instead of __enum_uint8(name) ? Then the diff will be much smaller.

#define __enum_uint8_decl(name)	enum  (name) : uint8_t