Re: [bp:tip-x86-alternatives 1/1] error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type

[Gary Guo <gary@garyguo.net>]

On Thu, 12 Jan 2023 17:14:52 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Sat, Jan 07, 2023 at 01:38:42AM +0100, Miguel Ojeda wrote:
> > You are of course right that the instructions are not complete, I just
> > meant to add a bit of context, i.e. that Rust got enabled due to the
> > config, but as far as I understand, it shouldn't be getting enabled in
> > the other ones for the moment.  
> 
> Right, or at least the repro instructions should state it clear.
> 
> Btw, this is part of a long-running feedback process we're giving to the 0day
> bot in order to make their reports as user friendly as possible.
> 
> > My point was that the script expects some variables set by `Makefile`,
> > similar to `$CC` etc., so that output does not imply you have (or not)
> > a suitable Rust toolchain installed (i.e. it will currently also fail
> > if you have it installed).  
> 
> Aha.
> 
> > Meanwhile (of course it is not the same as proper reproduction
> > instructions since the LKP team may do something different), the
> > documentation on how to set it up for a normal developer is at:
> > https://www.kernel.org/doc/html/latest/rust/quick-start.html, in case
> > it helps (if you are up for it... :)  
> 
> Probably that link should be part of those reproduction instructions.
> 
> > > And while we're reporting bugs: the error message from the compiler itself could
> > > use some "humanization" - I have zero clue what it is trying to tell me.  
> > 
> > What would you want to see? We can ask the relevant Rust team to see
> > if they can improve it.
> > 
> > In general, note that you can ask `rustc` to further explain an error
> > giving it the code with `--explain`. The compiler suggests this
> > itself, but sadly the robot cut it out :(  
> 
> Well, I find having an --explain option too much. But there are perhaps reasons
> for it.
> 
> One improvement could be, IMHO, they could turn on --explain automatically when
> it results in a build error. So that you don't have to do it yourself.
> 
> What would be better, tho, is if there were no --explain option at all and the
> warnings are as human readable as possible.
> 
> >     For more information about this error, try `rustc --explain E0588`
> > 
> > In this case, it gives:
> > 
> >     A type with `packed` representation hint has a field with `align`
> >     representation hint.
> > ...  
> 
> so the struct is:
> 
> struct alt_instr {
>         s32 instr_offset;       /* original instruction */
>         s32 repl_offset;        /* offset to replacement instruction */
> 
>         union {
>                 struct {
>                         u32 cpuid: 16;  /* CPUID bit set for replacement */
>                         u32 flags: 16;  /* patching control flags */
>                 };
>                 u32 ft_flags;
>         };
> 
>         u8  instrlen;           /* length of original instruction */
>         u8  replacementlen;     /* length of new instruction */
> } __packed;
> 
> and everything is naturally aligned.
> 
> So I'm guessing this is a rust bindings glue shortcoming or so...
> 
> Thx.
> 

Hi Borislav,

Thanks for the MCVE. I'm able to figure out what exactly went
wrong.

In the struct you shown, `alt_instr.cpuid` and `alt_instr.flags` are
16-bit aligned (TIL bitfields alignments are related to their bit width
only, *NOT* the declared type), while the whole anonymous struct
containing them is 32-bit aligned (because u32 is used as type of
bitfields).

When generating bindings, bindgen decides to put a `#[repr(align(4))]`
when generating the anonymous struct to raise its alignment from 16 to
32 so that the struct is ABI compatible with C again. As a result, it
generates a `#[repr(align(...))` struct nested within `#[repr(packed)]`
struct, which is in turn rejected by rustc.

This isn't the only issue however, it seems that bindgen doesn't
consider alignment of bitfields when deciding if an explicit
`#[repr(align)]` is needed anyway, so it will stick such an attribute
to all struct containing only bitfields. So it doesn't help if `u32` is
changed to `u16` here.

This is a definitely a bindgen bug. I'll have a think about how to fix
it...

Best,
Gary