rustc_trans doesn't emit alignment information causing llvm to assume abi alignment #39376
Comments
binarycrusader
changed the title
What |
|
CC @dhduvall |
Right, I'm aware of what packed does. On some architectures, there is no such thing as an unaligned load. All accesses must be aligned according to the size of the access. So in this particular case, I suspect you're right that the code generation has gone wrong. In this particular case, I would have expected one of two things to happen:
Generally, implementation choice in this scenario is to simply ignore the allocation overhead and allocate the data aligned to avoid the extra code. Any thoughts? As for the code generation, are there any ideas as to where I should look? |
|
From the LLVM docs:
And we don't emit alignment for loads from packed structs. Oops. |
|
@arielb1 can you point me at where I can confirm this in the rust source code? |
|
For example, this: #[repr(packed)]
pub struct Foo(u32);
pub struct Bar(u8, Foo);
#[no_mangle]
pub fn foo(b: &Bar) -> u32 {
b.1 .0
}Translates into the following IR: |
|
BTW, which architecture are you using? |
|
@arielb1 the one I'm testing on at the moment is sparc; cross-compiled programs generally work fine (cargo, etc.), but a cross-compiled build of rustc itself tends to run into issues like this one I'm guessing the logic you're referring to is in src/librustc/ty/layout.rs inside the |
|
I can confirm that the above code is miscompiled on sparc, and that adding |
|
My best guess at the moment is that pub fn load(&self, ptr: ValueRef) -> ValueRef {
self.count_insn("load");
unsafe {
let load = llvm::LLVMBuildLoad(self.llbuilder, ptr, noname())
let ty = Type::from_ref(llvm::LLVMTypeOf(ptr));
if ty.is_packed() {
llvm::LLVMSetAlignment(load, 1 as c_uint);
}
load
}
} |
|
Here's a minimal example to reproduce the issue that attempts to closely mimic what is done in use std::slice;
use std::u32;
#[repr(packed)]
#[derive(Copy, Clone)]
struct Unaligned<T>(T);
impl<T> Unaligned<T> {
fn get(self) -> T { self.0 }
}
fn bytes_to_words(b: &[u8]) -> &[Unaligned<u32>] {
unsafe { slice::from_raw_parts(b.as_ptr() as *const Unaligned<u32>, b.len() / 4) }
}
fn main() {
let values = String::from("fedcba98765432100123456789abcdef");
let bytes = values.as_bytes();
let mut words = &bytes_to_words(&bytes[1..]);
let index = 1;
let position = u32::from_le(words[index].get());
} |
According to the LLVM reference: > A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target. So loads/stores of fields of packed structs need to have their align set to 1. Implement that by tracking the alignment of `LvalueRef`s. Fixes rust-lang#39376.
emit "align 1" metadata on loads/stores of packed structs According to the LLVM reference: > A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target. So loads/stores of fields of packed structs need to have their align set to 1. Implement that by tracking the alignment of `LvalueRef`s. Fixes rust-lang#39376. r? @eddyb
binarycrusader
changed the title
|
I see that @arielb1 already has a more comprehensive and probably far more correct fix, but purely as a point of reference, this is the hackery that worked for me: diff --git a/src/librustc_trans/base.rs b/src/librustc_trans/base.rs
index 4cdde24..6c657ef 100644
--- a/src/librustc_trans/base.rs
+++ b/src/librustc_trans/base.rs
@@ -455,6 +455,15 @@ pub fn load_fat_ptr<'a, 'tcx>(
b.load(ptr)
};
+ let ty = type_of::fat_ptr_base_ty(b.ccx, t).element_type();
+ if ty.kind() == llvm::TypeKind::Struct {
+ if ty.is_packed() {
+ unsafe {
+ llvm::LLVMSetAlignment(ptr, 1);
+ }
+ }
+ }
+
// FIXME: emit metadata on `meta`.
let meta = b.load(get_meta(b, src));
diff --git a/src/librustc_trans/builder.rs b/src/librustc_trans/builder.rs
index cf7f3e9..f7f89b8 100644
--- a/src/librustc_trans/builder.rs
+++ b/src/librustc_trans/builder.rs
@@ -514,7 +514,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
pub fn load(&self, ptr: ValueRef) -> ValueRef {
self.count_insn("load");
unsafe {
- llvm::LLVMBuildLoad(self.llbuilder, ptr, noname())
+ let load = llvm::LLVMBuildLoad(self.llbuilder, ptr, noname());
+
+ let mut ty = val_ty(ptr);
+ // Strip off pointers
+ while ty.kind() == llvm::TypeKind::Pointer {
+ ty = ty.element_type();
+ }
+
+ if ty.kind() == llvm::TypeKind::Struct {
+ if ty.is_packed() {
+ llvm::LLVMSetAlignment(load, 1);
+ }
+ }
+ load
}
}
@@ -577,6 +590,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let store = llvm::LLVMBuildStore(self.llbuilder, val, ptr);
if let Some(align) = align {
llvm::LLVMSetAlignment(store, align as c_uint);
+ } else {
+ let mut ty = val_ty(ptr);
+ // Strip off pointers
+ while ty.kind() == llvm::TypeKind::Pointer {
+ ty = ty.element_type();
+ }
+
+ if ty.kind() == llvm::TypeKind::Struct {
+ if ty.is_packed() {
+ llvm::LLVMSetAlignment(store, 1);
+ }
+ }
}
store
}I will try out @arielb1 's fix soon and see if it resolves the original crashes I was seeing, hopefully with results that are as good as the hacky patch I applied above. |
According to the LLVM reference: > A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target. So loads/stores of fields of packed structs need to have their align set to 1. Implement that by tracking the alignment of `LvalueRef`s. Fixes rust-lang#39376.
emit "align 1" metadata on loads/stores of packed structs According to the LLVM reference: > A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target. So loads/stores of fields of packed structs need to have their align set to 1. Implement that by tracking the alignment of `LvalueRef`s. Fixes #39376. r? @eddyb
|
@binarycrusader: can you verify rustc works on your sparc? |
|
@arielb1 it will take me some time as I'm actively working on a port to the platform I'm using and it doesn't build using rustbuild at the moment, which gate tip requires. I had applied your patch to a version of rustc from early January when I tested it. |
|
That version requires a different fix. Apply my patch to some 1.16 beta and test. |
|
@arielb1 I can confirm that with your fix applied to the latest beta, that particular alignment issue has been resolved, and I can now successfully run rustc on sparc. While I don't see "align 1" emitted in all of the cases where a load is done from a packed strcut, it does appear to be emitting in all the cases where it's actually required, so it's probably correct. If I find any new cases, I will open new bugs for them. I consider this bug fixed by #39586 (at least for nightly). |
According to the LLVM reference: > A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target. So loads/stores of fields of packed structs need to have their align set to 1. Implement that by tracking the alignment of `LvalueRef`s. Fixes rust-lang#39376.
commit 7bc1054 attempted to fix unaligned loads, but appears to have only resolved part of the problem and may have unintentionally left things a little broken on architectures with strict alignment requirements.
That commit changed bytes_to_words to return
&[Unaligned<u32>], but made that type#[repr(packed)].I'm currently working with a fellow colleague on a port of rust to another architecture and we hit this:
A disassembly of the call site shows this:
Now, if I'm not mistaken it looks like it's trying to perform an
ld(akalduw; load unsigned word) and a word is 4 bytes, but the offset of the memory address it's trying to read from is byte-aligned, not word-aligned, due to the#[repr(packed)], which I believe is causing the fault since what seems like a valid value appears to be there.I suspect this is another case of issue #27060, but while we're waiting for that to be fixed, should the packed directive be removed instead?
I plan to try that fix myself soon, but wanted confirmation that was a reasonable change before attempting it.
The text was updated successfully, but these errors were encountered: