lkml.org 
[lkml]   [2023]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2023-03-26 23:41    [W:0.122 / U:1.696 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site