Skip to content
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

AVR asm doesn't validate ldd operands #62012

Closed
workingjubilee opened this issue Apr 8, 2023 · 3 comments
Closed

AVR asm doesn't validate ldd operands #62012

workingjubilee opened this issue Apr 8, 2023 · 3 comments
Assignees

Comments

@workingjubilee
Copy link

workingjubilee commented Apr 8, 2023

Consider the following LLVM IR program, derived from a Rust program featured in rust-lang/rust#109360 that misuses the ldd instruction on AVR:

; ModuleID = 'avr_exampl.3c3314d5-cgu.0'
source_filename = "avr_exampl.3c3314d5-cgu.0"
target datalayout = "e-P1-p:16:8-i8:8-i16:8-i32:8-i64:8-f32:8-f64:8-n8-a:8"
target triple = "avr-unknown-unknown"

@alloc3 = private unnamed_addr constant <{ [2 x i8] }> <{ [2 x i8] c"\BB\AA" }>, align 1

; Function Attrs: nounwind
define dso_local void @main() unnamed_addr addrspace(1) #0 {
start:
    %0 = tail call addrspace(0) i8
        asm sideeffect alignstack
        "\0A    ldd ${0}, X+0    \0A    ldd ${0}, X+1   \0A",
        "=&r,{r27r26},~{sreg},~{memory}"
        (ptr nonnull @alloc3) #2,
        !srcloc !0
    ret void
}

attributes #0 = { nounwind "target-cpu"="atmega328" }
attributes #1 = { nofree norecurse noreturn nosync nounwind readnone "target-cpu"="atmega328" }
attributes #2 = { nounwind }

!0 = !{i32 144, i32 161, i32 194, i32 223}

As I understand it, ldd only takes the Y or Z registers as its second operand (plus an offset). But this assembly is accepted without complaint, resulting in ill-formed assembly and peculiar binaries. This is what llc thinks the assembly is:

main:                                   ; @main
        ldi     r26, lo8(alloc3)
        ldi     r27, hi8(alloc3)

        ldd     r24, X+0
        ldd     r24, X+1

        ret
alloc3:
        .ascii  "\273\252"

But this is what comes out the other side of objdump:

000000a6 <main>:
      a6: a0 e0        	ldi	r26, 0
      a8: b1 e0        	ldi	r27, 1
      aa: 88 81        	ldd	r24, Y+0
      ac: 89 81        	ldd	r24, Y+1
      ae: 08 95        	ret

The address is loaded into the r26 and r27 register (which are concatenated together as the "X" register), and then reads are done from Y (a valid target of ldd, at least), which does not contain the address that was loaded and thus the programmer's intent is lost.

Strictly speaking, there is no problem here. The assembly is incorrect, so feeding it into an assembler is arguably UB. However, observing that it is incorrect is also trivial, thus an assembler may wish to reject it. I don't know LLVM's policy here, but I notice llvm-project/llvm/test/CodeGen/AVR/inline-asm/inline-asm-invalid.ll does exist, so there's that.

@benshi001
Copy link
Member

fixed by https://reviews.llvm.org/D147877

However you need to use -march=avr -mcpu=avr6 -filetype=obj xxx.ll to trigger the error report.

@workingjubilee
Copy link
Author

workingjubilee commented Apr 9, 2023

Thank you! If we need to wrangle LLVM a bit to get full error messaging, that's okay, it's nice enough to not need to compile LLVM with full assertions to do so.

@benshi001
Copy link
Member

fixed in 6e57f68, please try to build llvm with the newest main branch.

gysit pushed a commit to nextsilicon/llvm-project that referenced this issue Apr 27, 2023
We should reject "ldd Rn, X" with explicit error message
rather than "llvm_unreachable" in llvm's release build.

Fixes llvm#62012

Reviewed By: Miss_Grape

Differential Revision: https://reviews.llvm.org/D147877
flemairen6 pushed a commit to Xilinx/llvm-project that referenced this issue May 10, 2023
We should reject "ldd Rn, X" with explicit error message
rather than "llvm_unreachable" in llvm's release build.

Fixes llvm#62012

Reviewed By: Miss_Grape

Differential Revision: https://reviews.llvm.org/D147877
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants