fix some bitfield size calculate bug #3215

qinghon · 2025-05-31T16:58:41Z

Now, we can perceive non-starting bitfields and non-ending bitfields

Also, I modified the calculation method of align: only calculate alignment for bitfield from structure using bitfield_width
The reason is that the code now no longer relies on #[repr(C)] to generate alignments, but always contains all bytes
The problem that is currently known is that the test case bitfield-linux-32.hpp was dropped #[repr(packed(4))]. I don't know the reason, hope someone can help me

Now, we can perceive non-starting bitfields and non-ending bitfields

qinghon · 2025-05-31T17:11:24Z

Issue known to be repaired:
#1377
#3105
#743
#981
#1314

qinghon · 2025-06-03T02:22:48Z

r? @emilio

rustbot · 2025-06-03T02:22:50Z

Error: The feature assign is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

emilio

Thanks! Generally it looks good but I suspect this is specific to 64-bit architectures? Or am I missing something? In any case some more comments would be good. Thanks

bindgen/ir/comp.rs

bindgen-tests/tests/expectations/tests/bitfield-linux-32.rs

bindgen/ir/comp.rs

For 32bit structures compiled across platforms, align_of is not accurate whether it is set to 8 or 4.

add bitfield pack offset bug

qinghon · 2025-06-10T15:59:12Z

fix and add #3104 case

this change add depended clang report offset

qinghon · 2025-06-20T02:07:48Z

@emilio Can you take some time to review this PR ？

wdouglass · 2025-06-25T13:18:53Z

Fixes #3238

nikvoid · 2025-07-03T14:20:50Z

~~Looks like some cases are still broken.~~

struct appHand_supportedAppProtocolRes {
    appHand_responseCodeType ResponseCode; // enum with few variants, offset = 0x0
    uint8_t SchemaID; // offset = 0x1 (why?, isn't enum i32?)
    unsigned int SchemaID_isUsed:1; // offset = 0x2 ?, size of struct = 4
};

Generated with bindgen 0.72:

#[repr(C)]
pub struct appHand_supportedAppProtocolRes {
    pub ResponseCode: appHand_responseCodeType::Type,
    pub SchemaID: u8,
    pub _bitfield_align_1: [u8; 0], 
    pub _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>, // offset = 0x5 ?
    pub __bindgen_padding_0: u16, // offset = 0x6, size of struct = 8
}

Generated with this PR:

#[repr(C)]
pub struct appHand_supportedAppProtocolRes {
    pub ResponseCode: appHand_responseCodeType::Type,
    pub SchemaID: u8,
    pub _bitfield_align_1: [u8; 0],
    pub _bitfield_1: __BindgenBitfieldUnit<[u8; 3usize]>, // offset = 0x5, still
}

Bitfield storage alignment is invalid

EDIT: Wrong original offsets
EDIT2: The actual problem is in size of enum, so this is a bit unrelated

qinghon · 2025-07-03T15:07:52Z

@nikvoid

Bitfield storage alignment is invalid

Yes

In the C compiler, SchemaID_isUsed and SchemaID will be packaged into an int and accessed in 4 byte alignment.

However, in bindgen, it is difficult to ensure this requirement under the current structure, which will cause some performance losses, but it is better than the wrong offset (the purpose of this PR is to fix the wrong offset/size, but at present, it does not destroy the alignment of the entire structure)

nikvoid · 2025-07-04T14:31:39Z

This actually was a thing with different compilers used from bindgen and C sides... GCC fitted enum to 1 byte, but clang did not.

emilio

Thanks! Looks good to me with some of the bits clarified / documented, and comments reworded.

emilio · 2025-07-11T19:40:10Z

bindgen/ir/comp.rs

        }
+        // depended clang report field offset, manual calculates sometime not true


Cannot make sense of this comment, can you reword?

I think you are saying something like:

// Use the clang-reported offset, since the manual calculation may be off.

Or so? Or the other way around?

emilio · 2025-07-11T19:40:55Z

bindgen/ir/comp.rs

@@ -643,8 +653,15 @@ where
            // NB: The `bitfield_width` here is completely, absolutely
            // intentional.  Alignment of the allocation unit is based on the
            // maximum bitfield width, not (directly) on the bitfields' types'
-            // alignment.
-            unit_align = cmp::max(unit_align, bitfield_width);
+            // alignment. this is only available in bitfields start of structure


Is this a libclang limitation?

emilio · 2025-07-11T19:41:52Z

bindgen/ir/comp.rs

+            if bitfields_bit_begin_off == 0 {
+                unit_align = cmp::max(unit_align, bitfield_width);
+            } else {
+                // if bitfields is not start of structure, due to the presence


Something like:

If the bitfield is not the first member of the struct, we can't use max alignment. Default to u8.

What is backward embedding? Never heard that term. You mean something like the optimization where you put fields in the padding of the base class or something?

emilio · 2025-07-11T19:43:40Z

bindgen/ir/comp.rs

+            .offset()
+            .map_or(unit_size_in_bits, |off| off - bitfields_bit_begin_off)
+    } else if let Some(parent_layout_) = parent_layout {
+        // if the bitfield is an end of structure, use bitfields start to the structure end


Hmm, is this special-case necessary? Seems a bit unfortunate...

emilio · 2025-07-11T19:45:44Z

Also could you squash before I hit the merge button? A lot of the commits don't have particularly descriptive commit messages (like cargo fmt)

emilio · 2025-07-11T19:53:08Z

bindgen-tests/tests/headers/bitfield-linux-32.hpp

@@ -1,4 +1,4 @@
-// bindgen-flags: -- --target=i586-unknown-linux
+// bindgen-flags: --no-layout-tests -- --target=i586-unknown-linux


Actually, I don't understand why this is needed. Aren't we regressing this test-case? Why is that fine? Can we file a follow-up issue to address it at least?

Or is the issue I guess that the layout doesn't match on Linux64 which is where we build the code?

emilio · 2025-07-11T20:00:28Z

bindgen/ir/comp.rs

        let bitfield_align = bitfield_layout.align;
+        // bitfield offset of struct/union
+        let bitfield_offset = bitfield.offset().unwrap();


Actually I'm not sure we can guarantee we have this, right? This panics with this patch:

template <typename T> struct foo { T member; bool b : 8; };

emilio · 2025-07-11T20:01:02Z

bindgen/ir/comp.rs

+        next_field
+            .offset()
+            .map_or(unit_size_in_bits, |off| off - bitfields_bit_begin_off)
+    } else if let Some(parent_layout_) = parent_layout {


Trying to remove parent_layout doesn't cause any layout test failures at a glance. So I think it's fine to remove.

emilio · 2025-07-11T21:07:21Z

Ok so I dug into what's going on. The key difference between how bindgen tracks bitfields vs. LLVM is that we track the offset within the bitfield, but llvm tracks the global offset within the struct.

#3247 fixes that, which fixes all the regressions, and it is significantly simpler. @qinghon lmk if you have any feedback on that. I kept credit since the test-cases are yours and you spent a lot of effort on this.

Thanks!

emilio · 2025-07-11T22:36:12Z

Superseded by #3247. But feel free to comment there or send more patches if something is not addressed. Thanks!

qinghon added 3 commits June 1, 2025 00:44

fix some bitfield size calculate bug

298a282

Now, we can perceive non-starting bitfields and non-ending bitfields

code format

8617950

update test case

7a49275

qinghon added 4 commits June 1, 2025 01:15

fix warn for clippy

7aa8995

fix test case issue-743

7ac0392

fix some sub overflow

e1a4d63

fix format and clippy

5492b58

emilio requested changes Jun 8, 2025

View reviewed changes

clean add some comment

abd4ada

qinghon requested a review from emilio June 9, 2025 11:09

qinghon added 3 commits June 10, 2025 10:33

Remove conflicting layout tests

36443da

For 32bit structures compiled across platforms, align_of is not accurate whether it is set to 8 or 4.

fix some bitfield offset not true

1dc9fb5

fix union_bitfield.h case bug

95c7d8b

add bitfield pack offset bug

qinghon added 5 commits June 10, 2025 23:59

cargo fmt

c21dddc

cargo clippy

7a25aa5

Merge remote-tracking branch 'origin/main'

da1f143

cargo fmt

a5450da

Merge branch 'main' into main

513823c

qinghon mentioned this pull request Jun 25, 2025

Incorrect bitstream length #3238

Open

emilio approved these changes Jul 11, 2025

View reviewed changes

emilio requested changes Jul 11, 2025

View reviewed changes

emilio closed this Jul 11, 2025

		}
		// depended clang report field offset, manual calculates sometime not true

		@@ -1,4 +1,4 @@
		// bindgen-flags: -- --target=i586-unknown-linux
		// bindgen-flags: --no-layout-tests -- --target=i586-unknown-linux

fix some bitfield size calculate bug #3215

fix some bitfield size calculate bug #3215

Uh oh!

Conversation

qinghon commented May 31, 2025

Uh oh!

qinghon commented May 31, 2025

Uh oh!

qinghon commented Jun 3, 2025

Uh oh!

rustbot commented Jun 3, 2025

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qinghon commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qinghon commented Jun 20, 2025

Uh oh!

wdouglass commented Jun 25, 2025

Uh oh!

nikvoid commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qinghon commented Jul 3, 2025

Uh oh!

nikvoid commented Jul 4, 2025

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

emilio Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

emilio Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

emilio Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

emilio Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

emilio commented Jul 11, 2025

Uh oh!

emilio Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

emilio Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

emilio Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

emilio commented Jul 11, 2025

Uh oh!

emilio commented Jul 11, 2025

Uh oh!

Uh oh!

qinghon commented Jun 10, 2025 •

edited

Loading

nikvoid commented Jul 3, 2025 •

edited

Loading