New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid machine code generated for R_AVR_16 fixups #39

Closed
gergoerdi opened this Issue Apr 30, 2017 · 13 comments

Comments

Projects
None yet
3 participants
@gergoerdi
Collaborator

gergoerdi commented Apr 30, 2017

GIven the following main function:

static mut PIXELS: &'static mut [u8] = &mut [0b_1010_0110; 10];

#[no_mangle]
pub extern fn main() {
    unsafe {
        spi::setup();

        for &pixel in PIXELS.iter() {
            spi::sync(pixel);
        }

        loop {}
    }
}

the generated .elf file seems to contain invalid machine code at several places, e.g.

000000b2 <LBB1_5>:
  b2:   8d b5           in      r24, 0x2d       ; 45
  b4:   88 23           and     r24, r24
  b6:   ea f7           brpl    .-6             ; 0xb2 <LBB1_5>
  b8:   8e b5           in      r24, 0x2e       ; 46
  ba:   83 92           .word   0x9283  ; ????
  bc:   00 00           nop
  be:   8e bd           out     0x2e, r24       ; 46

If needed, I can try attaching a full project, but it is basically using a trimmed-down version of libcore. Full .ll listing:

; ModuleID = 'hello_avr.cgu-0.rs'
source_filename = "hello_avr.cgu-0.rs"
target datalayout = "e-p:16:16:16-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-n8"
target triple = "avr-atmel-none"

@ref_mut.0 = internal unnamed_addr global [10 x i8] c"\A6\A6\A6\A6\A6\A6\A6\A6\A6\A6", align 1

; Function Attrs: norecurse nounwind readnone uwtable
define void @rust_eh_personality({}* nocapture, {}* nocapture) unnamed_addr #0 {
start:
  ret void
}


; Function Attrs: noreturn nounwind uwtable
define void @main() unnamed_addr #1 {
start:
  %0 = load volatile i8, i8* inttoptr (i16 37 to i8*), align 1
  %1 = or i8 %0, 4
  store volatile i8 %1, i8* inttoptr (i16 37 to i8*), align 1
  %2 = load volatile i8, i8* inttoptr (i16 36 to i8*), align 4
  %3 = or i8 %2, 44
  store volatile i8 %3, i8* inttoptr (i16 36 to i8*), align 4
  %4 = load volatile i8, i8* inttoptr (i16 36 to i8*), align 4
  %5 = and i8 %4, -17
  store volatile i8 %5, i8* inttoptr (i16 36 to i8*), align 4
  %6 = load volatile i8, i8* inttoptr (i16 76 to i8*), align 4
  %7 = or i8 %6, 80
  store volatile i8 %7, i8* inttoptr (i16 76 to i8*), align 4
  %8 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 0), align 1
  store volatile i8 %8, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i

bb2.i:                                            ; preds = %bb2.i, %start
  %9 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %10 = icmp sgt i8 %9, -1
  br i1 %10, label %bb2.i, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit:  ; preds = %bb2.i
  %11 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %12 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 1), align 1
  store volatile i8 %12, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.1

bb9:                                              ; preds = %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.9, %bb9
  br label %bb9

bb2.i.1:                                          ; preds = %bb2.i.1, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit
  %13 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %14 = icmp sgt i8 %13, -1
  br i1 %14, label %bb2.i.1, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.1

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.1: ; preds = %bb2.i.1
  %15 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %16 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 2), align 1
  store volatile i8 %16, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.2

bb2.i.2:                                          ; preds = %bb2.i.2, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.1
  %17 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %18 = icmp sgt i8 %17, -1
  br i1 %18, label %bb2.i.2, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.2

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.2: ; preds = %bb2.i.2
  %19 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %20 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 3), align 1
  store volatile i8 %20, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.3

bb2.i.3:                                          ; preds = %bb2.i.3, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.2
  %21 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %22 = icmp sgt i8 %21, -1
  br i1 %22, label %bb2.i.3, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.3

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.3: ; preds = %bb2.i.3
  %23 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %24 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 4), align 1
  store volatile i8 %24, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.4

bb2.i.4:                                          ; preds = %bb2.i.4, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.3
  %25 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %26 = icmp sgt i8 %25, -1
  br i1 %26, label %bb2.i.4, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.4

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.4: ; preds = %bb2.i.4
  %27 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %28 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 5), align 1
  store volatile i8 %28, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.5

bb2.i.5:                                          ; preds = %bb2.i.5, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.4
  %29 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %30 = icmp sgt i8 %29, -1
  br i1 %30, label %bb2.i.5, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.5

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.5: ; preds = %bb2.i.5
  %31 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %32 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 6), align 1
  store volatile i8 %32, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.6

bb2.i.6:                                          ; preds = %bb2.i.6, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.5
  %33 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %34 = icmp sgt i8 %33, -1
  br i1 %34, label %bb2.i.6, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.6

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.6: ; preds = %bb2.i.6
  %35 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %36 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 7), align 1
  store volatile i8 %36, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.7

bb2.i.7:                                          ; preds = %bb2.i.7, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.6
  %37 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %38 = icmp sgt i8 %37, -1
  br i1 %38, label %bb2.i.7, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.7

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.7: ; preds = %bb2.i.7
  %39 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %40 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 8), align 1
  store volatile i8 %40, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.8

bb2.i.8:                                          ; preds = %bb2.i.8, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.7
  %41 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %42 = icmp sgt i8 %41, -1
  br i1 %42, label %bb2.i.8, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.8

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.8: ; preds = %bb2.i.8
  %43 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  %44 = load i8, i8* getelementptr inbounds ([10 x i8], [10 x i8]* @ref_mut.0, i16 0, i16 9), align 1
  store volatile i8 %44, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb2.i.9

bb2.i.9:                                          ; preds = %bb2.i.9, %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.8
  %45 = load volatile i8, i8* inttoptr (i16 77 to i8*), align 1
  %46 = icmp sgt i8 %45, -1
  br i1 %46, label %bb2.i.9, label %_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.9

_ZN9hello_avr3spi4sync17he0f643349c96805cE.exit.9: ; preds = %bb2.i.9
  %47 = load volatile i8, i8* inttoptr (i16 78 to i8*), align 2
  br label %bb9
}

attributes #0 = { norecurse nounwind readnone uwtable }
attributes #1 = { noreturn nounwind uwtable }

!llvm.module.flags = !{!0}

!0 = !{i32 1, !"PIE Level", i32 2}
@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay Apr 30, 2017

Member

I'm pretty surprised by this to be honest because we have machine code tests for every single instruction in the ISA here.

Member

dylanmckay commented Apr 30, 2017

I'm pretty surprised by this to be honest because we have machine code tests for every single instruction in the ISA here.

@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay Apr 30, 2017

Member

If I compile the .ll file and objdump it, the machine code is valid.

mc.s

0000007a <LBB1_15>:                                                                                                                                                                    
    7a:   8d b5           in      r24, 0x2d       ; 45                                                                                                                                   
    7c:   88 23           and     r24, r24                                                                                                                                               
    7e:   02 f4           brpl    .+0             ; 0x80 <LBB1_15+0x6>                                                                                                                   
    80:   8e b5           in      r24, 0x2e       ; 46                           
    ; here is the broken instruction                                                                                                        
    82:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <LBB1_21+0x7fff62>                                                                                                          
    86:   8e bd           out     0x2e, r24       ; 46

It looks like it is only during linking when the instruction is corrupted.

mc.elf

000000fa <LBB1_15>:
  fa:   8d b5           in      r24, 0x2d       ; 45
  fc:   88 23           and     r24, r24
  fe:   ea f7           brpl    .-6             ; 0xfa <LBB1_15>
 100:   8e b5           in      r24, 0x2e       ; 46
 102:   88 92           .word   0x9288  ; ????
 104:   00 00           nop
 106:   8e bd           out     0x2e, r24       ; 46

Here is the object file displayed with relocations inline

0000007a <LBB1_15>:
  7a:   8d b5           in      r24, 0x2d       ; 45
  7c:   88 23           and     r24, r24
  7e:   02 f4           brpl    .+0             ; 0x80 <LBB1_15+0x6>
                        7e: R_AVR_7_PCREL       .text+0x7a
  80:   8e b5           in      r24, 0x2e       ; 46
  82:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <LBB1_21+0x7fff62>
                        82: R_AVR_16    .data+0x8
  86:   8e bd           out     0x2e, r24       ; 46
Member

dylanmckay commented Apr 30, 2017

If I compile the .ll file and objdump it, the machine code is valid.

mc.s

0000007a <LBB1_15>:                                                                                                                                                                    
    7a:   8d b5           in      r24, 0x2d       ; 45                                                                                                                                   
    7c:   88 23           and     r24, r24                                                                                                                                               
    7e:   02 f4           brpl    .+0             ; 0x80 <LBB1_15+0x6>                                                                                                                   
    80:   8e b5           in      r24, 0x2e       ; 46                           
    ; here is the broken instruction                                                                                                        
    82:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <LBB1_21+0x7fff62>                                                                                                          
    86:   8e bd           out     0x2e, r24       ; 46

It looks like it is only during linking when the instruction is corrupted.

mc.elf

000000fa <LBB1_15>:
  fa:   8d b5           in      r24, 0x2d       ; 45
  fc:   88 23           and     r24, r24
  fe:   ea f7           brpl    .-6             ; 0xfa <LBB1_15>
 100:   8e b5           in      r24, 0x2e       ; 46
 102:   88 92           .word   0x9288  ; ????
 104:   00 00           nop
 106:   8e bd           out     0x2e, r24       ; 46

Here is the object file displayed with relocations inline

0000007a <LBB1_15>:
  7a:   8d b5           in      r24, 0x2d       ; 45
  7c:   88 23           and     r24, r24
  7e:   02 f4           brpl    .+0             ; 0x80 <LBB1_15+0x6>
                        7e: R_AVR_7_PCREL       .text+0x7a
  80:   8e b5           in      r24, 0x2e       ; 46
  82:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <LBB1_21+0x7fff62>
                        82: R_AVR_16    .data+0x8
  86:   8e bd           out     0x2e, r24       ; 46
@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay Apr 30, 2017

Member

The R_AVR_16 relocation writes up to 16-bits. I've checked with the instruction manual and the lds instruction does indeed have 16-bits dedicated to the pointer, and so we're not just emitting a pointer so large it flipped bits in rest of the instruction.

When I attempt to assemble the same instruction with AVR-GCC:

➜  avr-llvm avr-as
 lds     r24, 0x0000
➜  avr-llvm avr-objdump -S a.out

a.out:     file format elf32-avr

Disassembly of section .text:

00000000 <.text>:
   0:   80 91 00 00     lds     r24, 0x0000     ;  0x800000

It looks like the linking step is mutating the second byte of the LDS instruction, which it shouldn't. This makes avr-objdump interpret the first byte as junk, and the second byte as a NOP.

Interestingly, if I use llc to generate assembly from the .ll and then use avr-as to assemble and avr-gcc to link, the executable turns out fine.

Member

dylanmckay commented Apr 30, 2017

The R_AVR_16 relocation writes up to 16-bits. I've checked with the instruction manual and the lds instruction does indeed have 16-bits dedicated to the pointer, and so we're not just emitting a pointer so large it flipped bits in rest of the instruction.

When I attempt to assemble the same instruction with AVR-GCC:

➜  avr-llvm avr-as
 lds     r24, 0x0000
➜  avr-llvm avr-objdump -S a.out

a.out:     file format elf32-avr

Disassembly of section .text:

00000000 <.text>:
   0:   80 91 00 00     lds     r24, 0x0000     ;  0x800000

It looks like the linking step is mutating the second byte of the LDS instruction, which it shouldn't. This makes avr-objdump interpret the first byte as junk, and the second byte as a NOP.

Interestingly, if I use llc to generate assembly from the .ll and then use avr-as to assemble and avr-gcc to link, the executable turns out fine.

@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay Apr 30, 2017

Member

Here is the relocation generated by LLVM

  82:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <LBB1_21+0x7fff62>
                        82: R_AVR_16    .data+0x8

Here is the relocation generated by GCC

  82:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <LBB1_21+0x7fff62>
                        84: R_AVR_16    .data+0x8
Member

dylanmckay commented Apr 30, 2017

Here is the relocation generated by LLVM

  82:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <LBB1_21+0x7fff62>
                        82: R_AVR_16    .data+0x8

Here is the relocation generated by GCC

  82:   80 91 00 00     lds     r24, 0x0000     ; 0x800000 <LBB1_21+0x7fff62>
                        84: R_AVR_16    .data+0x8
@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay Apr 30, 2017

Member

Here are the two relocations side-by-side

00000082 R_AVR_16          .data+0x00000008 # LLVM
00000084 R_AVR_16          .data+0x00000008 # GCC

It looks like the targets of the LLVM relocation is off by two bytes. This makes sense because the LLVM relocation was changing bytes it shouldn't have.

This is probably caused by us assuming that an instruction is 2 bytes somewhere in LLVM.

Member

dylanmckay commented Apr 30, 2017

Here are the two relocations side-by-side

00000082 R_AVR_16          .data+0x00000008 # LLVM
00000084 R_AVR_16          .data+0x00000008 # GCC

It looks like the targets of the LLVM relocation is off by two bytes. This makes sense because the LLVM relocation was changing bytes it shouldn't have.

This is probably caused by us assuming that an instruction is 2 bytes somewhere in LLVM.

@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay Apr 30, 2017

Member

It looks like both our fixup description table AND the fixup creation code doesn't take into account the offset into the instruction.

Member

dylanmckay commented Apr 30, 2017

It looks like both our fixup description table AND the fixup creation code doesn't take into account the offset into the instruction.

@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay Apr 30, 2017

Member

I've got a fix locally which fixes it

GCC

mc.gcc.elf

 102:   80 91 08 01     lds     r24, 0x0108     ; 0x800108 <ref_mut.0+0x8>

LLVM

mc.llvm.elf

 102:   80 91 08 01     lds     r24, 0x0108     ; 0x800108 <ref_mut.0+0x8>
Member

dylanmckay commented Apr 30, 2017

I've got a fix locally which fixes it

GCC

mc.gcc.elf

 102:   80 91 08 01     lds     r24, 0x0108     ; 0x800108 <ref_mut.0+0x8>

LLVM

mc.llvm.elf

 102:   80 91 08 01     lds     r24, 0x0108     ; 0x800108 <ref_mut.0+0x8>
@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay Apr 30, 2017

Member

I've submitted a fix for this upstream in r301782.

Member

dylanmckay commented Apr 30, 2017

I've submitted a fix for this upstream in r301782.

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster May 1, 2017

Collaborator

Cherry-picked llvm-mirror/llvm@900da36 into our holding LLVM fork

Collaborator

shepmaster commented May 1, 2017

Cherry-picked llvm-mirror/llvm@900da36 into our holding LLVM fork

@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay May 1, 2017

Member

Do we want to keep this open?

Shall we cherry-pick all has-llvm_commit patches into our fork?

Seems like we may as well cherry-pick and close.

Member

dylanmckay commented May 1, 2017

Do we want to keep this open?

Shall we cherry-pick all has-llvm_commit patches into our fork?

Seems like we may as well cherry-pick and close.

@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay May 1, 2017

Member

Looks like you just did :)

Member

dylanmckay commented May 1, 2017

Looks like you just did :)

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster May 1, 2017

Collaborator

Closing the issue I can see going one way or the other... I'm cool with closing it; @gergoerdi can always reopen or open a new issue if the codegen isn't better.

Collaborator

shepmaster commented May 1, 2017

Closing the issue I can see going one way or the other... I'm cool with closing it; @gergoerdi can always reopen or open a new issue if the codegen isn't better.

@shepmaster shepmaster changed the title from Invalid machine code in .elf file to Invalid machine code generated for R_AVR_16 fixups May 1, 2017

@dylanmckay

This comment has been minimized.

Show comment
Hide comment
@dylanmckay

dylanmckay May 1, 2017

Member

@gergoerdi: Thanks for the bug report(s)! There should no longer be a corrupted instruction if you pull master and rebuild.

Feel free to reopen if it's still broken

Member

dylanmckay commented May 1, 2017

@gergoerdi: Thanks for the bug report(s)! There should no longer be a corrupted instruction if you pull master and rebuild.

Feel free to reopen if it's still broken

@dylanmckay dylanmckay closed this May 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment