rustc_trans doesn't emit alignment information causing llvm to assume abi alignment

[binarycrusader]

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:

Thread 4 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 2 (LWP 2)]
0xffffffff50037b34 in rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup (self=0xffffffff7b861df0, bytes=..., def_index=...)
    at /builds/rust.git/src/librustc_metadata/index.rs:73
73              let position = u32::from_le(words[index].get());
(gdb) print words
$37 = &[rustc_metadata::index::Unaligned<u32>] {data_ptr: 0xffffffff793ae6b6, length: 6908}
(gdb) print index
$38 = 5283
(gdb) print words[index]
$39 = rustc_metadata::index::Unaligned<u32> (4092795392)

A disassembly of the call site shows this:

(gdb) disassemble 0xffffffff50037b34-16,+32
Dump of assembler code from 0xffffffff50037b24 to 0xffffffff50037b44:
   0xffffffff50037b24 <rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup+864>:      nop 
   0xffffffff50037b28 <rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup+868>:      ldx  [ %fp + 0x627 ], %i0
   0xffffffff50037b2c <rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup+872>:      ldx  [ %fp + 0x537 ], %i1
   0xffffffff50037b30 <rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup+876>:      sllx  %i1, 2, %i2
=> 0xffffffff50037b34 <rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup+880>:      ld  [ %i0 + %i2 ], %i0
   0xffffffff50037b38 <rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup+884>:      st  %i0, [ %fp + 0x6e7 ]
   0xffffffff50037b3c <rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup+888>:      st  %i0, [ %fp + 0x7d7 ]
   0xffffffff50037b40 <rustc_metadata::schema::LazySeq<rustc_metadata::index::Index>::lookup+892>:      call  0xffffffff50038054 <rustc_metadata::index::Unaligned<u32>::get<u32>>
(gdb) print/a $i0    
$40 = 0xffffffff793ae6b6
(gdb) print/a $i2
$41 = 0x528c
(gdb) print/a ($i0+$i2)
$42 = 0xffffffff793b3942
(gdb) print/x *($i0+$i2)
$44 = 0xf3f31a00

Now, if I'm not mistaken it looks like it's trying to perform an ld (aka lduw; 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.

[nagisa]

That commit changed bytes_to_words to return &[Unaligned], but made that type #[repr(packed)].

What #[repr(packed)] does is making compiler to consider alignment of Unaligned<u32> to be 1. If the generated code is still trying to use aligned-word-load for this structure, then the LLVM/Rust are most likely miscompiling stuff in this case.

[binarycrusader]

CC @dhduvall

[binarycrusader]

What #[repr(packed)] does is making compiler to consider alignment of Unaligned to be 1. If the generated code is still trying to use aligned-word-load for this structure, then the LLVM/Rust are most likely miscompiling stuff in this case.

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:

1. Unaligned would actually be aligned according to size of u32
2. Code generation would have generated four unsigned byte loads (ldub)

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?

[arielb1]

From the LLVM docs:

A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target.

And we don't emit alignment for loads from packed structs. Oops.

[binarycrusader]

@arielb1 can you point me at where I can confirm this in the rust source code?

[arielb1]

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:

define i32 @foo(%Bar* noalias readonly dereferenceable(5)) unnamed_addr #0 !dbg !4 {
entry-block:
  %arg0 = alloca %Bar*
  %b = alloca %Bar*
  store %Bar* %0, %Bar** %arg0
  call void @llvm.dbg.declare(metadata %Bar** %arg0, metadata !20, metadata !21), !dbg !22
  call void @llvm.dbg.declare(metadata %Bar** %b, metadata !23, metadata !21), !dbg !25
  br label %start

start:                                            ; preds = %entry-block
  %1 = load %Bar*, %Bar** %arg0, !dbg !26, !nonnull !2
  store %Bar* %1, %Bar** %b, !dbg !26
  %2 = load %Bar*, %Bar** %b, !dbg !27, !nonnull !2
  %3 = getelementptr inbounds %Bar, %Bar* %2, i32 0, i32 1, !dbg !27
  %4 = getelementptr inbounds %Foo, %Foo* %3, i32 0, i32 0, !dbg !27
  %5 = load i32, i32* %4, !dbg !27 ; you would expect an "align 1" here
  ret i32 %5, !dbg !28
}

[arielb1]

BTW, which architecture are you using?

[binarycrusader]

@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 if !ret.packed { block.

[arielb1]

I can confirm that the above code is miscompiled on sparc, and that adding align 1 fixes the problem.

--- orig.s      2017-02-03 00:31:43.635590663 +0100
+++ fixed.s     2017-02-03 00:32:14.347723354 +0100
@@ -23,7 +23,16 @@
        st %i0, [%fp+-8]
 .Ltmp4:
        .loc    1 8 0                   ! <anon>:8:0
-       ld [%i0+1], %i0
+       ldub [%i0+3], %i1
+       ldub [%i0+4], %i2
+       ldub [%i0+1], %i3
+       ldub [%i0+2], %i0
+       sll %i1, 8, %i1
+       or %i1, %i2, %i1
+       sll %i3, 8, %i2
+       or %i2, %i0, %i0
+       sll %i0, 16, %i0
+       or %i0, %i1, %i0
        .loc    1 9 0                   ! <anon>:9:0
        ret
        restore

[binarycrusader]

My best guess at the moment is that fn load in src/librustc_trans/builder.rs either needs an alignment parameter for callers to specify, or at a minimum, needs to do something like this instead (untested, wild guess):

    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 
        }    
    }  

[binarycrusader]

Here's a minimal example to reproduce the issue that attempts to closely mimic what is done in src/librustc_metadata/index.rs:

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());
}