2023-03-02 14:16:47

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: [PATCH 1/1] rust: bindgen: Add `alt_instr` as opaque type

To address this build error:

BINDGEN rust/bindings/bindings_generated.rs
BINDGEN rust/bindings/bindings_helpers_generated.rs
EXPORTS rust/exports_core_generated.h
RUSTC P rust/libmacros.so
RUSTC L rust/compiler_builtins.o
RUSTC L rust/alloc.o
RUSTC L rust/bindings.o
RUSTC L rust/build_error.o
EXPORTS rust/exports_alloc_generated.h
error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
--> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10094:1
|
10094 | / pub struct alt_instr {
10095 | | pub instr_offset: s32,
10096 | | pub repl_offset: s32,
10097 | | pub __bindgen_anon_1: alt_instr__bindgen_ty_1,
10098 | | pub instrlen: u8_,
10099 | | pub replacementlen: u8_,
10100 | | }
| |_^
|
note: `alt_instr__bindgen_ty_1__bindgen_ty_1` has a `#[repr(align)]` attribute
--> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10111:1
|
10111 | / pub struct alt_instr__bindgen_ty_1__bindgen_ty_1 {
10112 | | pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize], u16>,
10113 | | }
| |_^
note: `alt_instr` contains a field of type `alt_instr__bindgen_ty_1`
--> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10097:9
|
10097 | pub __bindgen_anon_1: alt_instr__bindgen_ty_1,
| ^^^^^^^^^^^^^^^^
note: ...which contains a field of type `alt_instr__bindgen_ty_1__bindgen_ty_1`
--> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10104:9
|
10104 | pub __bindgen_anon_1: alt_instr__bindgen_ty_1__bindgen_ty_1,
| ^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0588`.
make[1]: *** [rust/Makefile:389: rust/bindings.o] Error 1
make: *** [Makefile:1293: prepare] Error 2

Cc: Derek Barbosa <[email protected]>
Cc: Miguel Ojeda <[email protected]>
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
rust/bindgen_parameters | 1 +
1 file changed, 1 insertion(+)

diff --git a/rust/bindgen_parameters b/rust/bindgen_parameters
index be4963bf720304da..552d9a85925b9945 100644
--- a/rust/bindgen_parameters
+++ b/rust/bindgen_parameters
@@ -6,6 +6,7 @@
--opaque-type local_apic

# Packed type cannot transitively contain a `#[repr(align)]` type.
+--opaque-type alt_instr
--opaque-type x86_msi_data
--opaque-type x86_msi_addr_lo

--
2.39.2



2023-03-02 14:58:40

by Miguel Ojeda

[permalink] [raw]
Subject: Re: [PATCH 1/1] rust: bindgen: Add `alt_instr` as opaque type

On Thu, Mar 2, 2023 at 3:16 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> To address this build error:

This is due to commit 5d1dd961e743 ("x86/alternatives: Add
alt_instr.flags") that will land in v6.3-rc1.

Vincenzo reported it in Zulip too, so I will add a `Reported-by` for
him. There was a LKP report too in tip.

Thanks!

Cheers,
Miguel

Subject: Re: [PATCH 1/1] rust: bindgen: Add `alt_instr` as opaque type

On 3/2/23 11:16, Arnaldo Carvalho de Melo wrote:
> To address this build error:
>
> BINDGEN rust/bindings/bindings_generated.rs
> BINDGEN rust/bindings/bindings_helpers_generated.rs
> EXPORTS rust/exports_core_generated.h
> RUSTC P rust/libmacros.so
> RUSTC L rust/compiler_builtins.o
> RUSTC L rust/alloc.o
> RUSTC L rust/bindings.o
> RUSTC L rust/build_error.o
> EXPORTS rust/exports_alloc_generated.h
> error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
> --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10094:1
> |
> 10094 | / pub struct alt_instr {
> 10095 | | pub instr_offset: s32,
> 10096 | | pub repl_offset: s32,
> 10097 | | pub __bindgen_anon_1: alt_instr__bindgen_ty_1,
> 10098 | | pub instrlen: u8_,
> 10099 | | pub replacementlen: u8_,
> 10100 | | }
> | |_^
> |
> note: `alt_instr__bindgen_ty_1__bindgen_ty_1` has a `#[repr(align)]` attribute
> --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10111:1
> |
> 10111 | / pub struct alt_instr__bindgen_ty_1__bindgen_ty_1 {
> 10112 | | pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize], u16>,
> 10113 | | }
> | |_^
> note: `alt_instr` contains a field of type `alt_instr__bindgen_ty_1`
> --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10097:9
> |
> 10097 | pub __bindgen_anon_1: alt_instr__bindgen_ty_1,
> | ^^^^^^^^^^^^^^^^
> note: ...which contains a field of type `alt_instr__bindgen_ty_1__bindgen_ty_1`
> --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10104:9
> |
> 10104 | pub __bindgen_anon_1: alt_instr__bindgen_ty_1__bindgen_ty_1,
> | ^^^^^^^^^^^^^^^^
>

Reading the kernel sources this field corresponds to an u16 which indeed
represents a bit set and it says so in a comment on the field. I
couldn't replicate this issue, though, because this struct is used only
inside arch pretty much internally, then there's no problem to make it
opaque. Still, we have to be careful if these kind of things appear in
the future.

And I notice that You haven't mentioned the version of Bindgen that
You've used, including its linked libclang too. Otherwise I think this
could be accepted.

Reviewed-by: Martin Rodriguez Reboredo <[email protected]>

> error: aborting due to previous error
>
> For more information about this error, try `rustc --explain E0588`.
> make[1]: *** [rust/Makefile:389: rust/bindings.o] Error 1
> make: *** [Makefile:1293: prepare] Error 2
>
> Cc: Derek Barbosa <[email protected]>
> Cc: Miguel Ojeda <[email protected]>
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> ---
> rust/bindgen_parameters | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/rust/bindgen_parameters b/rust/bindgen_parameters
> index be4963bf720304da..552d9a85925b9945 100644
> --- a/rust/bindgen_parameters
> +++ b/rust/bindgen_parameters
> @@ -6,6 +6,7 @@
> --opaque-type local_apic
>
> # Packed type cannot transitively contain a `#[repr(align)]` type.
> +--opaque-type alt_instr
> --opaque-type x86_msi_data
> --opaque-type x86_msi_addr_lo
>

2023-03-02 15:15:46

by Vincenzo Palazzo

[permalink] [raw]
Subject: Re: [PATCH 1/1] rust: bindgen: Add `alt_instr` as opaque type

> To address this build error:
>
> BINDGEN rust/bindings/bindings_generated.rs
> BINDGEN rust/bindings/bindings_helpers_generated.rs
> EXPORTS rust/exports_core_generated.h
> RUSTC P rust/libmacros.so
> RUSTC L rust/compiler_builtins.o
> RUSTC L rust/alloc.o
> RUSTC L rust/bindings.o
> RUSTC L rust/build_error.o
> EXPORTS rust/exports_alloc_generated.h
> error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
> --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10094:1
> |
> 10094 | / pub struct alt_instr {
> 10095 | | pub instr_offset: s32,
> 10096 | | pub repl_offset: s32,
> 10097 | | pub __bindgen_anon_1: alt_instr__bindgen_ty_1,
> 10098 | | pub instrlen: u8_,
> 10099 | | pub replacementlen: u8_,
> 10100 | | }
> | |_^
> |
> note: `alt_instr__bindgen_ty_1__bindgen_ty_1` has a `#[repr(align)]` attribute
> --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10111:1
> |
> 10111 | / pub struct alt_instr__bindgen_ty_1__bindgen_ty_1 {
> 10112 | | pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize], u16>,
> 10113 | | }
> | |_^
> note: `alt_instr` contains a field of type `alt_instr__bindgen_ty_1`
> --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10097:9
> |
> 10097 | pub __bindgen_anon_1: alt_instr__bindgen_ty_1,
> | ^^^^^^^^^^^^^^^^
> note: ...which contains a field of type `alt_instr__bindgen_ty_1__bindgen_ty_1`
> --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10104:9
> |
> 10104 | pub __bindgen_anon_1: alt_instr__bindgen_ty_1__bindgen_ty_1,
> | ^^^^^^^^^^^^^^^^
>
> error: aborting due to previous error
>
> For more information about this error, try `rustc --explain E0588`.
> make[1]: *** [rust/Makefile:389: rust/bindings.o] Error 1
> make: *** [Makefile:1293: prepare] Error 2
>
> Cc: Derek Barbosa <[email protected]>
> Cc: Miguel Ojeda <[email protected]>
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>

Ah good catch!

Reviewed-by: Vincenzo Palazzo <[email protected]>

2023-03-02 15:35:55

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 1/1] rust: bindgen: Add `alt_instr` as opaque type

Em Thu, Mar 02, 2023 at 11:59:00AM -0300, Martin Rodriguez Reboredo escreveu:
> On 3/2/23 11:16, Arnaldo Carvalho de Melo wrote:
> > To address this build error:
> >
> > BINDGEN rust/bindings/bindings_generated.rs
> > BINDGEN rust/bindings/bindings_helpers_generated.rs
> > EXPORTS rust/exports_core_generated.h
> > RUSTC P rust/libmacros.so
> > RUSTC L rust/compiler_builtins.o
> > RUSTC L rust/alloc.o
> > RUSTC L rust/bindings.o
> > RUSTC L rust/build_error.o
> > EXPORTS rust/exports_alloc_generated.h
> > error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type
> > --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10094:1
> > |
> > 10094 | / pub struct alt_instr {
> > 10095 | | pub instr_offset: s32,
> > 10096 | | pub repl_offset: s32,
> > 10097 | | pub __bindgen_anon_1: alt_instr__bindgen_ty_1,
> > 10098 | | pub instrlen: u8_,
> > 10099 | | pub replacementlen: u8_,
> > 10100 | | }
> > | |_^
> > |
> > note: `alt_instr__bindgen_ty_1__bindgen_ty_1` has a `#[repr(align)]` attribute
> > --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10111:1
> > |
> > 10111 | / pub struct alt_instr__bindgen_ty_1__bindgen_ty_1 {
> > 10112 | | pub _bitfield_1: __BindgenBitfieldUnit<[u8; 4usize], u16>,
> > 10113 | | }
> > | |_^
> > note: `alt_instr` contains a field of type `alt_instr__bindgen_ty_1`
> > --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10097:9
> > |
> > 10097 | pub __bindgen_anon_1: alt_instr__bindgen_ty_1,
> > | ^^^^^^^^^^^^^^^^
> > note: ...which contains a field of type `alt_instr__bindgen_ty_1__bindgen_ty_1`
> > --> /var/home/acme/git/linux/rust/bindings/bindings_generated.rs:10104:9
> > |
> > 10104 | pub __bindgen_anon_1: alt_instr__bindgen_ty_1__bindgen_ty_1,
> > | ^^^^^^^^^^^^^^^^
> >
>
> Reading the kernel sources this field corresponds to an u16 which indeed
> represents a bit set and it says so in a comment on the field. I
> couldn't replicate this issue, though, because this struct is used only
> inside arch pretty much internally, then there's no problem to make it
> opaque. Still, we have to be careful if these kind of things appear in
> the future.

ok

> And I notice that You haven't mentioned the version of Bindgen that
> You've used, including its linked libclang too. Otherwise I think this
> could be accepted.

⬢[acme@toolbox linux]$ bindgen --version
bindgen 0.56.0
⬢[acme@toolbox linux]$ which bindgen
/var/home/acme/.cargo/bin/bindgen
⬢[acme@toolbox linux]$ ldd /var/home/acme/.cargo/bin/bindgen
linux-vdso.so.1 (0x00007ffe543be000)
libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f7b69e94000)
libm.so.6 => /lib64/libm.so.6 (0x00007f7b69db4000)
libc.so.6 => /lib64/libc.so.6 (0x00007f7b69bd7000)
/lib64/ld-linux-x86-64.so.2 (0x00007f7b6a235000)
⬢[acme@toolbox linux]$ clang --version &| head -2
bash: syntax error near unexpected token `|'
⬢[acme@toolbox linux]$ clang --version |& head -2
clang version 15.0.7 (Fedora 15.0.7-1.fc37)
Target: x86_64-redhat-linux-gnu
⬢[acme@toolbox linux]$

> Reviewed-by: Martin Rodriguez Reboredo <[email protected]>
>
> > error: aborting due to previous error
> >
> > For more information about this error, try `rustc --explain E0588`.
> > make[1]: *** [rust/Makefile:389: rust/bindings.o] Error 1
> > make: *** [Makefile:1293: prepare] Error 2
> >
> > Cc: Derek Barbosa <[email protected]>
> > Cc: Miguel Ojeda <[email protected]>
> > Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
> > ---
> > rust/bindgen_parameters | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/rust/bindgen_parameters b/rust/bindgen_parameters
> > index be4963bf720304da..552d9a85925b9945 100644
> > --- a/rust/bindgen_parameters
> > +++ b/rust/bindgen_parameters
> > @@ -6,6 +6,7 @@
> > --opaque-type local_apic
> >
> > # Packed type cannot transitively contain a `#[repr(align)]` type.
> > +--opaque-type alt_instr
> > --opaque-type x86_msi_data
> > --opaque-type x86_msi_addr_lo
> >

--

- Arnaldo

2023-03-02 21:03:13

by Miguel Ojeda

[permalink] [raw]
Subject: Re: [PATCH 1/1] rust: bindgen: Add `alt_instr` as opaque type

On Thu, Mar 2, 2023 at 3:59 PM Martin Rodriguez Reboredo
<[email protected]> wrote:
>
> Still, we have to be careful if these kind of things appear in
> the future.

Not entirely sure what you mean -- do you mean if some Rust
abstractions used its fields? But if we were in that case, it would
not compile, so we would notice. Or what do you mean?

> And I notice that You haven't mentioned the version of Bindgen that
> You've used, including its linked libclang too. Otherwise I think this
> could be accepted.

I could reproduce this with the expected versions. Since, for now,
those are the only ones supported and the build system emits a warning
otherwise, I think it is fair to assume those versions were used
unless otherwise stated.

Cheers,
Miguel

2023-03-02 22:08:02

by Miguel Ojeda

[permalink] [raw]
Subject: Re: [PATCH 1/1] rust: bindgen: Add `alt_instr` as opaque type

On Thu, Mar 2, 2023 at 3:16 PM Arnaldo Carvalho de Melo <[email protected]> wrote:
>
> To address this build error:

Applied to `rust-fixes` for tomorrow's -next run.

Thanks!

Cheers,
Miguel

Subject: Re: [PATCH 1/1] rust: bindgen: Add `alt_instr` as opaque type

On 3/2/23 18:02, Miguel Ojeda wrote:
> On Thu, Mar 2, 2023 at 3:59 PM Martin Rodriguez Reboredo
> <[email protected]> wrote:
>>
>> Still, we have to be careful if these kind of things appear in
>> the future.
>
> Not entirely sure what you mean -- do you mean if some Rust
> abstractions used its fields? But if we were in that case, it would
> not compile, so we would notice. Or what do you mean?
>

I've meant a general case with any abstraction, but that would be
noticed right away.

>> And I notice that You haven't mentioned the version of Bindgen that
>> You've used, including its linked libclang too. Otherwise I think this
>> could be accepted.
>
> I could reproduce this with the expected versions. Since, for now,
> those are the only ones supported and the build system emits a warning
> otherwise, I think it is fair to assume those versions were used
> unless otherwise stated.
>
> Cheers,
> Miguel

Sounds fair.