Fix test case from #2246 #2248

youknowone · 2022-07-26T21:38:05Z

close #2246
fix align codegen of #2240
fix align codegen of #2159
fix align codegen of #1538

tests/headers/packed-align-conflict.h

youknowone · 2022-07-27T19:57:27Z

mm.. test passes only because test doesn't include case about:

typedef unsigned char uint8_t;
typedef unsigned short uint16_t;

struct A {
  uint8_t a;
  uint16_t b;
  uint8_t c;
} __attribute__((aligned(2)));

struct B {
  uint8_t a;
  uint16_t b;
  uint8_t c;
} __attribute__((aligned(2), packed));


#include <assert.h>
#include <stdio.h>
int main() {
  printf("%lu %lu\n", sizeof(struct A), sizeof(struct B));
  assert(sizeof(struct A) == sizeof(struct B));
}

youknowone · 2022-07-27T20:05:17Z

is this the answer?

#[repr(C, align(2))]
#[derive(Debug, Default, Copy, Clone)]
pub struct B(B__packed);

#[repr(C, packed)]
#[derive(Debug, Default, Copy, Clone)]
pub struct B__packed {
    pub a: u8,
    pub b: u16,
    pub c: u8,
}

#[test]
fn bindgen_test_layout_B() {
    const UNINIT: ::std::mem::MaybeUninit<B> =
        ::std::mem::MaybeUninit::uninit();
    let ptr = UNINIT.as_ptr();
    assert_eq!(
        ::std::mem::size_of::<B>(),
        4usize,
        concat!("Size of: ", stringify!(B))
    );
    assert_eq!(
        ::std::mem::align_of::<B>(),
        2usize,
        concat!("Alignment of ", stringify!(B))
    );
    assert_eq!(
        unsafe { ::std::ptr::addr_of!((*ptr).0.a) as usize - ptr as usize },
        0usize,
        concat!("Offset of field: ", stringify!(B), "::", stringify!(a))
    );
    assert_eq!(
        unsafe { ::std::ptr::addr_of!((*ptr).0.b) as usize - ptr as usize },
        1usize,
        concat!("Offset of field: ", stringify!(B), "::", stringify!(b))
    );
    assert_eq!(
        unsafe { ::std::ptr::addr_of!((*ptr).0.c) as usize - ptr as usize },
        3usize,
        concat!("Offset of field: ", stringify!(B), "::", stringify!(c))
    );
}

youknowone

Still a few problems, but I think I made enough proof of concept how it works.

Now, packed struct also getting align attribute because I couldn't remove those case from requires_explicit_align
Also bitfield got problem. I need your advice for this case.

Except for those 2 cases, everything seems working.
The design probably is not very good. I feel like bindgen may already have a tool to handle those wrapper types.

youknowone · 2022-07-27T21:54:28Z

src/codegen/struct_layout.rs

@@ -365,7 +365,7 @@ impl<'a> StructLayoutTracker<'a> {
            return true;
        }

-        if self.max_field_align >= layout.align {
+        if !self.is_packed && self.max_field_align >= layout.align {


I want to check if it has explicit align in C code to avoid packed-only also get align attribute in rust side.

I'm confused about this change, can you elaborate? What case does this make a difference on?

Because packed or aligned cases are already being handled well, I need to check packed and aligned case here:
https://github.com/rust-lang/rust-bindgen/pull/2248/files#diff-01720a19d4518b9c16feea9cefbee210a853a1c99d1e9b19aec23e72877e52f0R2019-R2023

Without change, any packed type will not be marked as explicit_align. So the new handling condition doesn't hit.
The new problem introduced by this is only packed struct is also marked as aligned. Does bindgen have a feature to dinstinguish it?
Because the estimated size for clang is different, it looks like to be distinguishable.

emilio · 2022-07-28T17:28:55Z

src/codegen/mod.rs

                "packed".to_string()
            } else {
                format!("packed({})", n)
            };
            attributes.push(attributes::repr_list(&["C", &packed_repr]));
        } else {
            attributes.push(attributes::repr("C"));
-        }
-
-        if ctx.options().rust_features().repr_align {


Why removing the feature-check for repr(align)?

accidently happened during multiple revision 😂

emilio · 2022-07-28T17:29:28Z

src/codegen/mod.rs

+            .unwrap();
+            result.push(quote! {
+                #attributes
+                #[repr(C, align(#align))]


Probably better if it'd be #[repr(transparent)], that'd match the ABI exactly, though I don't think it would matter in practice.

It doesn't work. we need align here but transparent seems doesn't accept other things.

error[E0692]: transparent struct cannot have other repr hints --> tests/packed-align-conflict.rs:16:8 | 16 | #[repr(transparent, align(2))] |

emilio · 2022-07-28T17:32:04Z

src/codegen/struct_layout.rs

@@ -365,7 +365,7 @@ impl<'a> StructLayoutTracker<'a> {
            return true;
        }

-        if self.max_field_align >= layout.align {
+        if !self.is_packed && self.max_field_align >= layout.align {


I'm confused about this change, can you elaborate? What case does this make a difference on?

emilio · 2022-07-28T17:34:40Z

tests/expectations/tests/issue-537-repr-packed-n.rs

    pub i: ::std::os::raw::c_int,
 }
+#[derive(Debug, Default, Copy, Clone)]
+#[repr(C, align(2))]
+pub struct AlignedToTwo(AlignedToTwo__packed);


This should be public, how do folks access the fields otherwise? More to the point, we should try to only add this wrapper when absolutely needed (I think packed && max_field_align < explicit_align, right?), and also we should probably impl Deref and DerefMut on the struct to get more convenient access / avoid most code breakage as a result of this change.

About Deref, Is there another wrapper mechanism to do it? Or this is the first place to add those Deref stuff?
~~The condition is logically right but I have no idea how to do yet.~~

Counter example about the condition.

// sizeof(B) == 6 struct B { uint8_t a; uint16_t b; uint8_t c; } __attribute__((aligned(2), packed));

This is packed but not max_field_align < explicit_align (2 < 2).

The key idea we need to complete this task is distinguishing it from

// sizeof(B) == 4 struct B { uint8_t a; uint16_t b; uint8_t c; } __attribute__(packed));

…on hints. Fixes rust-lang#2240

youknowone · 2022-07-28T21:14:14Z

I added the last commit to check the CI result. I don't believe that's the correct way. Please suppose I am not understanding the remaining problems and how bindgen handle packed and aligned.

youknowone · 2022-08-01T11:25:09Z

@emilio do you have advices?

youknowone · 2022-08-18T14:51:53Z

@emilio can I get advice for remaining issues?

emilio · 2022-08-18T16:58:54Z

Sorry for the lag, it's on my list of things to do. I plan to dig a bit into how C++ compilers treat repr+aligned hints because I'm not super-convinced there's a way to emulate it in Rust, but if you've done that research and can point to it it'd be extremely helpful.

youknowone · 2022-08-18T17:57:03Z

I think emulating in rust is done here. I forgot the datails now but If you need explanation, I can review it again.

If bindgen cannot distinguish explicitly aligned and implicitly aligned struct, could you guide me to how to get clang-calcuated size and bindgen-calculated size for the struct? Then I will try to detect which alignment used by them.

bors-servo · 2022-10-05T01:59:34Z

☔ The latest upstream changes (presumably #2284) made this pull request unmergeable. Please resolve the merge conflicts.

CGMossa · 2023-04-10T15:10:10Z

Can I kindly ask that this gets looked at please?

pvdrz · 2023-04-10T15:44:09Z

I've seen several issues and PRs discussing problems with packed and align representations. I'll gather some info and have a proper discussion around it with other contributors.

emilio requested changes Jul 27, 2022

View reviewed changes

tests/headers/packed-align-conflict.h Show resolved Hide resolved

youknowone marked this pull request as draft July 27, 2022 19:03

youknowone force-pushed the packed-align-conflict branch 3 times, most recently from 065d903 to 7cc3a34 Compare July 27, 2022 19:43

youknowone marked this pull request as ready for review July 27, 2022 19:48

youknowone requested a review from emilio July 27, 2022 19:50

youknowone marked this pull request as draft July 27, 2022 20:07

youknowone force-pushed the packed-align-conflict branch 2 times, most recently from cc0ce21 to ec4094f Compare July 27, 2022 21:41

youknowone commented Jul 27, 2022

View reviewed changes

emilio requested changes Jul 28, 2022

View reviewed changes

codegen: Don't generate conflicting packed() and align() representati…

61f307d

…on hints. Fixes rust-lang#2240

youknowone force-pushed the packed-align-conflict branch 4 times, most recently from 2f67f2a to 03837ca Compare July 28, 2022 20:57

youknowone added 2 commits July 29, 2022 06:10

poc packed wrapper

3785a8e

XXX: I am not understanding how things going. don't merge this commit

8f31017

youknowone force-pushed the packed-align-conflict branch from aa0db92 to 8f31017 Compare July 28, 2022 21:11

pvdrz deleted the branch rust-lang:master November 2, 2023 17:50

pvdrz closed this Nov 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix test case from #2246 #2248

Fix test case from #2246 #2248

youknowone commented Jul 26, 2022 •

edited

youknowone commented Jul 27, 2022

youknowone commented Jul 27, 2022

youknowone left a comment

youknowone Jul 27, 2022

emilio Jul 28, 2022

youknowone Jul 28, 2022

emilio Jul 28, 2022

youknowone Jul 28, 2022

emilio Jul 28, 2022

youknowone Jul 28, 2022

emilio Jul 28, 2022

emilio Jul 28, 2022

youknowone Jul 28, 2022 •

edited

youknowone Jul 28, 2022 •

edited

youknowone commented Jul 28, 2022 •

edited

youknowone commented Aug 1, 2022

youknowone commented Aug 18, 2022

emilio commented Aug 18, 2022

youknowone commented Aug 18, 2022 •

edited

bors-servo commented Oct 5, 2022

CGMossa commented Apr 10, 2023

pvdrz commented Apr 10, 2023

Fix test case from #2246 #2248

Fix test case from #2246 #2248

Conversation

youknowone commented Jul 26, 2022 • edited

youknowone commented Jul 27, 2022

youknowone commented Jul 27, 2022

youknowone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowone Jul 28, 2022 • edited

Choose a reason for hiding this comment

youknowone Jul 28, 2022 • edited

Choose a reason for hiding this comment

youknowone commented Jul 28, 2022 • edited

youknowone commented Aug 1, 2022

youknowone commented Aug 18, 2022

emilio commented Aug 18, 2022

youknowone commented Aug 18, 2022 • edited

bors-servo commented Oct 5, 2022

CGMossa commented Apr 10, 2023

pvdrz commented Apr 10, 2023

youknowone commented Jul 26, 2022 •

edited

youknowone Jul 28, 2022 •

edited

youknowone Jul 28, 2022 •

edited

youknowone commented Jul 28, 2022 •

edited

youknowone commented Aug 18, 2022 •

edited