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

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

[Borislav Petkov]

Resending because rust ML doesn't like big messages:

From: rust-for-linux-owner@vger.kernel.org
To: rust-for-linux-approval@vger.kernel.org, bp@alien8.de
Subject: BOUNCE rust-for-linux@vger.kernel.org:     Message too long (>100000 chars)

Rust folks, you can check out the whole thing here:

https://lore.kernel.org/all/202212272003.rgQDX8DQ-lkp@intel.com/

(and maybe raise the limit on that ML of yours :))

Thx.

On Tue, Dec 27, 2022 at 02:52:25PM +0100, Borislav Petkov wrote:
> On Tue, Dec 27, 2022 at 08:36:11PM +0800, kernel test robot wrote:
> > Hi Borislav,
> > 
> > FYI, the error/warning was bisected to this commit, please ignore it if it's irrelevant.
> > 
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git tip-x86-alternatives
> > head:   82db736201e76889825efe8899ad55976111691a
> > commit: 82db736201e76889825efe8899ad55976111691a [1/1] x86/alternatives: Add alt_instr.flags
> > config: x86_64-rhel-8.3-rust
> > compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
> > reproduce (this is a W=1 build):
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/commit/?id=82db736201e76889825efe8899ad55976111691a
> >         git remote add bp https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git
> >         git fetch --no-tags bp tip-x86-alternatives
> >         git checkout 82db736201e76889825efe8899ad55976111691a
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 prepare
> 
> These reproduction instructions look insufficient to me. The env needs a
> rust compiler installed. Which I don't have:
> 
> ./scripts/rust_is_available.sh -v
> ***
> *** Rust compiler '' could not be found.
> ***
> 
> Or does some of that make.cross magic install one? I don't see a "rustc"
> mentioned there at all but I see
> 
> CONFIG_RUSTC_VERSION_TEXT="rustc 1.62.0 (a8314ef7d 2022-06-27)"
> 
> in the .config so apparently that rustc thing has come from somewhere...
> 
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <lkp@intel.com>
> > 
> > All errors (new ones prefixed by >>):
> > 
> > >> error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
> 
> -ENOPARSE this error.
> 
> Lemme add rust and toolchain MLs to Cc and leave the rest for them with
> the hope that they can translate this linenoise for me.

Well, not leaving it because rust ML can't take such big emails.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Alexander Altman]

> On Dec 27, 2022, at 6:16 AM, Borislav Petkov <bp@alien8.de> wrote:
> 
> Resending because rust ML doesn't like big messages:
> 
> From: rust-for-linux-owner@vger.kernel.org
> To: rust-for-linux-approval@vger.kernel.org, bp@alien8.de
> Subject: BOUNCE rust-for-linux@vger.kernel.org:     Message too long (>100000 chars)
> 
> Rust folks, you can check out the whole thing here:
> 
> https://lore.kernel.org/all/202212272003.rgQDX8DQ-lkp@intel.com/
> 
> (and maybe raise the limit on that ML of yours :))
> 
> Thx.
> 
> On Tue, Dec 27, 2022 at 02:52:25PM +0100, Borislav Petkov wrote:
>> On Tue, Dec 27, 2022 at 08:36:11PM +0800, kernel test robot wrote:
>>> Hi Borislav,
>>> 
>>> FYI, the error/warning was bisected to this commit, please ignore it if it's irrelevant.
>>> 
>>> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git tip-x86-alternatives
>>> head:   82db736201e76889825efe8899ad55976111691a
>>> commit: 82db736201e76889825efe8899ad55976111691a [1/1] x86/alternatives: Add alt_instr.flags
>>> config: x86_64-rhel-8.3-rust
>>> compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
>>> reproduce (this is a W=1 build):
>>>        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>>        chmod +x ~/bin/make.cross
>>>        # https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/commit/?id=82db736201e76889825efe8899ad55976111691a
>>>        git remote add bp https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git
>>>        git fetch --no-tags bp tip-x86-alternatives
>>>        git checkout 82db736201e76889825efe8899ad55976111691a
>>>        # save the config file
>>>        mkdir build_dir && cp config build_dir/.config
>>>        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
>>>        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 prepare
>> 
>> These reproduction instructions look insufficient to me. The env needs a
>> rust compiler installed. Which I don't have:
>> 
>> ./scripts/rust_is_available.sh -v
>> ***
>> *** Rust compiler '' could not be found.
>> ***
>> 
>> Or does some of that make.cross magic install one? I don't see a "rustc"
>> mentioned there at all but I see
>> 
>> CONFIG_RUSTC_VERSION_TEXT="rustc 1.62.0 (a8314ef7d 2022-06-27)"
>> 
>> in the .config so apparently that rustc thing has come from somewhere...
>> 
>>> If you fix the issue, kindly add following tag where applicable
>>> | Reported-by: kernel test robot <lkp@intel.com>
>>> 
>>> All errors (new ones prefixed by >>):
>>> 
>>>>> error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type


This is indeed directly because of your change, although fixing it beyond my
skill.  Explanation:

You made the following change:
> - u16 cpuid; /* cpuid bit set for replacement */
> +
> + union {
> + struct {
> + u32 cpuid: 16; /* CPUID bit set for replacement */
> + u32 flags: 16; /* patching control flags */
> + };
> + u32 ft_flgs;
> + };

That caused Rust’s bindgen (bindings generator) to generate a type for the
altered field that indirectly included a representation of the
bitfields...which have a greater-than-natural alignment because of their
encoding (they’re represented as an array of 4 8-bit unsigned integers, but
aligned as if they’re a single 16-bit unsigned integer).  This interacts
badly with the top-level command to make the alt_instr struct packed, which
bindgen faithfully translates from C __packed to Rust #[repr(packed)].

One way to resolve this temporarily would be to add the following line above
the offending struct:
/// <div rustbindgen hide></div>
This will cause bindgen to ignore the struct entirely and not translate it.  If it’s
actually needed for Rust code, now or later, then we can’t do that and need
to actually replace it with something translatable, or else leave it hidden and
manually create its translation on the Rust side.  For the latter, just using a
u32 for the entire bitfield-containing union would be sufficient.

>> 
>> -ENOPARSE this error.
>> 
>> Lemme add rust and toolchain MLs to Cc and leave the rest for them with
>> the hope that they can translate this linenoise for me.
> 
> Well, not leaving it because rust ML can't take such big emails.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 


Thanks,
     Laine Taffin Altman 

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

[Borislav Petkov]

On Tue, Dec 27, 2022 at 12:31:17PM -0800, Alexander Altman wrote:
> That caused Rust’s bindgen (bindings generator) to generate a type for the
> altered field that indirectly included a representation of the
> bitfields...which have a greater-than-natural alignment because of their
> encoding (they’re represented as an array of 4 8-bit unsigned integers, but
> aligned as if they’re a single 16-bit unsigned integer).  This interacts
> badly with the top-level command to make the alt_instr struct packed, which
> bindgen faithfully translates from C __packed to Rust #[repr(packed)].

This sounds like a rust problem to me. Because, AFAICT, this is
perfectly valid C and both compilers haven't complained even with all
the possible warnings turned on.

> One way to resolve this temporarily would be to add the following line above
> the offending struct:
> /// <div rustbindgen hide></div>

Nah, I don't think I'll accept a fix for the shortcomings of yet another tool.

> This will cause bindgen to ignore the struct entirely and not translate it.  If it’s
> actually needed for Rust code, now or later, then we can’t do that and need
> to actually replace it with something translatable, or else leave it hidden and
> manually create its translation on the Rust side.  For the latter, just using a
> u32 for the entire bitfield-containing union would be sufficient.

Yap, that sounds like the right thing to do.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Miguel Ojeda]

Hi Borislav,

On Tue, Dec 27, 2022 at 2:52 PM Borislav Petkov <bp@alien8.de> wrote:
>
> These reproduction instructions look insufficient to me. The env needs a
> rust compiler installed. Which I don't have:

Yeah, note the x86_64-rhel-8.3-rust config name. It is a config the
kernel test robot added for testing with Rust enabled (which explains
the version text you saw for `rustc`).

> ./scripts/rust_is_available.sh -v
> ***
> *** Rust compiler '' could not be found.
> ***

The script is meant to be called as a Make target, e.g. `make LLVM=1
rustavailable`. Perhaps we can put a message if the script was called
directly.

Cheers,
Miguel

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

[Miguel Ojeda]

On Tue, Dec 27, 2022 at 9:31 PM Alexander Altman <alexanderaltman@me.com> wrote:
>
> One way to resolve this temporarily would be to add the following line above
> the offending struct:
> /// <div rustbindgen hide></div>
> This will cause bindgen to ignore the struct entirely and not translate it.  If it’s
> actually needed for Rust code, now or later, then we can’t do that and need
> to actually replace it with something translatable, or else leave it hidden and
> manually create its translation on the Rust side.  For the latter, just using a
> u32 for the entire bitfield-containing union would be sufficient.

Thanks a lot Alexander for taking a look!

This is https://github.com/rust-lang/rust-bindgen/issues/2179.

What we do for constructs that `bindgen` cannot map is to add the
appropriate parameter/line in `rust/bindgen_parameters`. We hit a
similar case for `x86_msi_data` that you can see in that file. So
please feel free to add it there.

If we end up needing to access it from the Rust side, another
alternative is to write a C function that performs the required
operation on it that then the Rust side calls.

Cheers,
Miguel

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

[Borislav Petkov]

On Sat, Jan 07, 2023 at 12:25:17AM +0100, Miguel Ojeda wrote:
> Yeah, note the x86_64-rhel-8.3-rust config name. It is a config the
> kernel test robot added for testing with Rust enabled (which explains
> the version text you saw for `rustc`).

I figured as much.

As I said:

"These reproduction instructions look insufficient to me."

> The script is meant to be called as a Make target, e.g. `make LLVM=1
> rustavailable`. Perhaps we can put a message if the script was called
> directly.

No need - I ran it by hand just to show that I don't have a rust compiler
installed.

Bottom line is: if I get a build report involving a rust compiler, there better
be in the reproduction instructions a hint how to install one so that I can
reproduce. Alternatively, I can always simply ignore it.

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.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Miguel Ojeda]

On Sat, Jan 7, 2023 at 12:58 AM Borislav Petkov <bp@alien8.de> wrote:
>
> I figured as much.

Sorry, I didn't mean otherwise, I should have quoted the next paragraph.

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.

> No need - I ran it by hand just to show that I don't have a rust compiler
> installed.

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).

> Bottom line is: if I get a build report involving a rust compiler, there better
> be in the reproduction instructions a hint how to install one so that I can
> reproduce. Alternatively, I can always simply ignore it.

Cc'ing Yujie for the robot instructions. Once I reported something
missing in the instructions (unrelated to Rust) and they were happy to
get the report, so I assume they will want to improve it here too.

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... :)

> 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 :(

    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.

    Erroneous code example:

    ```
    #[repr(align(16))]
    struct Aligned(i32);

    #[repr(packed)] // error!
    struct Packed(Aligned);
    ```

    Just like you cannot have both `align` and `packed` representation
hints on a
    same type, a `packed` type cannot contain another type with the `align`
    representation hint. However, you can do the opposite:

    ```
    #[repr(packed)]
    struct Packed(i32);

    #[repr(align(16))] // ok!
    struct Aligned(Packed);
    ```

You can also see it rendered in
https://doc.rust-lang.org/error_codes/E0588.html, which is also useful
if you don't have the toolchain around. Another option if you don't
remember the URL is going to Compiler Explorer, e.g.
https://godbolt.org/z/Ec17xnGsT.

Yujie: perhaps the robot could avoid dropping that line? Or even
better, you could automatically add a URL like above and/or run the
compiler with `--explain` and add it directly to the output (with a
size limit I guess)? That could probably be very helpful for kernel
developers that receive a Rust report.

Cheers,
Miguel

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

[Miguel Ojeda]

On Tue, Dec 27, 2022 at 3:16 PM Borislav Petkov <bp@alien8.de> wrote:
>
> Resending because rust ML doesn't like big messages:
>
> Rust folks, you can check out the whole thing here:
>
> https://lore.kernel.org/all/202212272003.rgQDX8DQ-lkp@intel.com/
>
> (and maybe raise the limit on that ML of yours :))

Thanks for the resend, by the way! Yeah, we had trouble with the limit
for the big initial patches too... I can ask the vger admins if they
are willing to raise it given we get these reports now.

Cheers,
Miguel

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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Gary Guo]

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

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

[Miguel Ojeda]

On Thu, Jan 12, 2023 at 5:14 PM Borislav Petkov <bp@alien8.de> wrote:
>
> 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.

Sounds very useful, thanks for the effort!

> 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.

Sometimes one can get quite a few errors/warnings, so printing all the
docs for all those would be probably too much.

I think `--explain` is intended to provide a way to have longer
documentation about a particular diagnostic message without filling
the terminal too much those that already know what the error/warning
is about (in this particular case, the error first line looks fine to
me -- but of course the machine-translated bindings from `bindgen` are
harder to read due to the generated identifiers).

> 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...

Yeah, this is https://github.com/rust-lang/rust-bindgen/issues/2179 (I
mentioned it in my reply to Alexander above, in case you didn't see
it, as well as the usual workarounds we use).

There are also other related issues related to GCC's `packed` and
`aligned` attributes: aligned fields in general are not supported
(including just adding the attribute), neither are structs declared
both packed and aligned. So only a subset of possibilities are handled
properly. I collected some links to related issues at
https://github.com/Rust-for-Linux/linux/issues/353.

From what I could tell reading some old discussions the other day, the
`bindgen` maintainer (Cc'ing Emilio) is aware of the issues but nobody
has put the time to solve it within the bindings generator.

Ideally, I think, Rust could support providing alignment for
individual members in `repr(C)` structs in order to tweak its
described layout algorithm, in order for users to mimic GCC/MSVC/...
extensions as needed. That way it can be useful also for everyone
(even those not using `bindgen`), and `bindgen`'s implementation would
be easier, I imagine.

Cheers,
Miguel

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

[Miguel Ojeda]

On Sat, Jan 14, 2023 at 3:19 AM Gary Guo <gary@garyguo.net> wrote:
>
> This is a definitely a bindgen bug. I'll have a think about how to fix
> it...

Added your PR to the list at
https://github.com/Rust-for-Linux/linux/issues/353, thanks Gary!

Cheers,
Miguel

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

[David Laight]

From: Borislav Petkov
> Sent: 12 January 2023 16:15
...
> > 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.

While there would be no padding between any of the stricture members
that is largely irrelevant.
The __packed removes the two pad bytes from the end of the structure
and also tells the compiler that the structure itself may not be
aligned in memory.

So the bitfields can be misaligned in memory.
Quite why they are bitfields is anybodies guess, u16 would suffice.
Using u16 would also generate a known memory layout (bitfields are
best part of compiler defined - certainly not endianness defined.)

If the intent of the __packed is to save 2 bytes on every copy
of the structure then __packed __aligned(2) will have the same
effect while allowing the compiler to use 16-bit accesses.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)