Messages in this thread | | | Date | Sat, 14 Jan 2023 02:18:35 +0000 | From | Gary Guo <> | Subject | Re: [bp:tip-x86-alternatives 1/1] error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type |
| |
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
| |