Re: [PATCH net-next v5 3/9] lib: packing: add pack_fields() and unpack_fields()

[Jacob Keller <jacob.e.keller@intel.com>]

On 11/15/2024 12:48 PM, Masahiro Yamada wrote:
> On Thu, Nov 14, 2024 at 6:04 AM Jacob Keller <jacob.e.keller@intel.com> wrote:
>> With the goal of maintaining compile time checks, we end up either
>> needing to use generated macros which are O(N^2) if we allow arbitrary
>> overlap. If we instead allow only only ascending or descending order,
>> this would drop to O(N) which would avoid needing to have 20k lines of
>> generated code for the case with 50. I think we could implement them
>> without forcing drivers to specifically call the correct macro by using
>> something like __builtin_choose_expr(), tho implementing that macro to
>> select could be quite long.
> 
> 
> WIth Clang, the following check seems to work,
> but with GCC, it works only when the array size is small.
> 
> 
> #define PACKED_FIELDS_OUT_OF_ORDER(fields) \
> ({ \
>         bool res = false; \
>         for (unsigned int i = 1; i < ARRAY_SIZE(fields); i++) \
>                 res |= fields[i - 1].startbit < fields[i].startbit; \
>         res; \
> })
> 
> #define PACKED_FIELDS_OVERWRAP(fields) \
> ({ \
>         bool res = false; \
>         for (unsigned int i = 1; i < ARRAY_SIZE(fields); i++) \
>                 res |= fields[i - 1].endbit <= fields[i].startbit; \
>         res; \
> })
> 
> /*
>  * Clang cleverly computes this at compile time.
>  * Unfortunately, GCC gives it up when the array size becomes large.
>  * Turn on this check only when building the kernel with Clang.
>  */
> #ifdef CONFIG_CC_IS_CLANG
> #define PACKED_FIELDS_SANITY_CHECKS(fields) \
>         BUILD_BUG_ON_MSG(PACKED_FIELDS_OUT_OF_ORDER(fields), \
>                          #fields ": not sorted decending order"); \
>         BUILD_BUG_ON_MSG(PACKED_FIELDS_OVERWRAP(fields), \
>                          #fields ": contains overwrap")
> #else
> #define PACKED_FIELDS_SANITY_CHECKS(fields)
> #endif
> 

This definitely seems compiler specific.

> 
> 
> 
> 
>> Otherwise we can fall back to either module load time checks, or go all
>> the way back to only sanity checking at executing of pack_fields or
>> unpack_fields.
> 
> Is it a big deal?
> One solution is a run-time check (for GCC), which is a one-time
> for booting or module loading.
> 
> Another is to rely on CICD running with Clang to detect overwraps.
> 
> 
> It is horrible to include kernel-space structures from user-space
> programs that run in a different architecture.
> 
> file2alias.c does this because it is only possible at compile-time,
> but it is always the source of troubles.
> I am search for a way to generate MODULE_ALIAS() without
> including mod_devicetable.h from modpost.
> 

Yea, I agree modpost is pretty ugly, and I'm happy to drop it.

I think I've managed to get something that works in GCC and Clang with
~3k lines of macro vs the original 20K lines we had for sizes up to 50.

The version I have now works, but does require the 3K lines of macro to
effectively unwind the loops.

Its also slightly brittle because some slight alterations of the checks
no longer get detected by GCC as compile-time constants :(

I am not a huge fan of only testing on CI, since not every developer
will have things go through CI, so we end up with late reports.

I'm going to post the next version which uses the macros again, but
manages to limit things so that the calls are all done from within
<linux/packing.h> without driver intervention, and seem to work reliably
on my test systems.

> 
> 
> 
> --
> Best Regards
> Masahiro Yamada