Make i128 aligned to 16 bytes on all 64bit plats #38871
Conversation
The alignment expected by the sysv ABI for i128 (16 bytes) does not match the alignment expected by rustc (8 bytes). Anything containing `i128`, then, when passed across the ABI boundary may result in stack/memory being trashed by the callee.
Not sure if its minimal required alignment on non-x86_64 targets but extra alignment cannot influence correctness, so its fine.
|
r? @eddyb |
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
|
Why all architectures? I only heard something about x64 SysV. Do you have links to more information? |
| @@ -343,6 +343,7 @@ pub fn compute_abi_info(ccx: &CrateContext, fty: &mut FnType) { | |||
| arg.make_indirect(ccx); | |||
| if let Some(attr) = ind_attr { | |||
| arg.attrs.set(attr); | |||
| arg.attrs.set_align(ty_align(arg.ty, 0) as u64); | |||
There was a problem hiding this comment.
Isn't this a noop? (same for all the other cabi_* changes)
Only 64 bit ones¹. Because I have no idea what expected alignment on those arches are for i128. Having larger alignment is not incorrect, only potentially wasteful, whereas alignment being 8 instead of expected 16 might end up in similar hard-to-debug issues again. I’d rather have all 64 bit arches have i128 aligned to 16 bytes and reduce the alignment later on, rather than go around looking for some weird “codegen bug” where something somewhere gets overwritten on stack on some very obscure platform that nobody really has access to. ¹: 32 bit arches do not matter because C compilers do not support int128 on 32 bit machines. |
... except when using it inside a If you found clang code giving custom alignments to |
That’s https://github.com/llvm-mirror/clang/blob/ec16cb9b5a481d62a73ad47fa59034ced4d62022/lib/AST/ASTContext.cpp#L416, also linked in the referenced issue. |
Oops, had not read that in detail. |
|
clang has |
|
☔ The latest upstream changes (presumably #39095) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Better solved in LLVM. |
Fixes #38762
This is based on #38870.