Skip to content

codegen: Fix struct padding alignment regression - #3449

Merged
emilio merged 4 commits into
mainfrom
fix-3406
Sep 6, 2026
Merged

codegen: Fix struct padding alignment regression#3449
emilio merged 4 commits into
mainfrom
fix-3406

Conversation

@emilio

@emilio emilio commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

See #3406.

@ojeda

ojeda commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Thanks for looking at this so quickly.

What about aligned typedefs:

typedef int AlignedInt __attribute__((aligned(16)));

struct Outer {
    long long before;
    AlignedInt inner;
};

? 0.72.1 and 0.73.1 would give a different result (inner's offset (16 vs. 8) and Outer's size (32 vs. 16)) than 445dee5.

@rustbot

rustbot commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@emilio

emilio commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@ojeda Hmm, fun one... This patch doesn't fix it, but that never worked quite right, tho, did it? 0.72.1 generates broken code for this:

typedef int AlignedInt __attribute__((aligned(16)));

struct Outer {
    int before;
    AlignedInt one;
    AlignedInt two;
};

Or even something simpler like:

typedef int AlignedInt __attribute__((aligned(8)));

struct Outer {
    int before;
    AlignedInt inner;
};

I guess the crux of the issue here is that Rust doesn't really have a way of provide the alignment on a typedef. For the struct member case, manually injecting the padding works, I suppose, but it won't work for things like arguments and such.

So to make this work properly I think we'd need to generate something like a wrapped type (but that is also not quite right, because you do want #[repr(transparent)].

So I think I'd consider that a technically pre-existing, separate bug. I can fix the struct layout by manually padding, though it's a bit ugly.

It's not necessary now that we support repr(align) properly.

There's probably some edge cases where we still get the padding wrong
tho.

Fixes #3406.
Since we know precisely the amount of bytes we need.
@ojeda

ojeda commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Yeah, if those were broken, agreed, of course.

Just to clarify: the one I mentioned was "working" before, no? i.e. it is still a change (which perhaps doesn't break anybody, but it may be surprising).

Thanks!

@emilio

emilio commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Just to clarify: the one I mentioned was "working" before, no? i.e. it is still a change (which perhaps doesn't break anybody, but it may be surprising).

Yeah, it was working, but just by chance, because we were deciding to manually pad...

I'll look at fixing those in a follow-up, I think it might not be too annoying, at least from the struct layout perspective...

@emilio
emilio enabled auto-merge September 6, 2026 18:56
@emilio
emilio added this pull request to the merge queue Sep 6, 2026
Merged via the queue into main with commit 910679f Sep 6, 2026
51 checks passed
emilio added a commit that referenced this pull request Sep 6, 2026
This fixes struct layout issues with over-aligned typedefs, which
rust can't represent, see #3449 for context and some other discussion.

This is kinda ugly tho, but as far as I can tell there's no good way of
getting the right ABI and struct layout at the same time...
pull Bot pushed a commit to xtqqczze/rust-lang-rust-bindgen that referenced this pull request Sep 6, 2026
This fixes struct layout issues with over-aligned typedefs, which
rust can't represent, see rust-lang#3449 for context and some other discussion.

This is kinda ugly tho, but as far as I can tell there's no good way of
getting the right ABI and struct layout at the same time...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants