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

[lld][ELF][AVR] Add range check for R_AVR_13_PCREL #67636

Merged
merged 1 commit into from
Oct 6, 2023
Merged

[lld][ELF][AVR] Add range check for R_AVR_13_PCREL #67636

merged 1 commit into from
Oct 6, 2023

Conversation

benshi001
Copy link
Member

Some large AVR programs (for devices without long jump) may exceed 128KiB, and lld should give explicit errors other than generate wrong executables silently.

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 28, 2023

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Changes

Some large AVR programs (for devices without long jump) may exceed 128KiB, and lld should give explicit errors other than generate wrong executables silently.


Full diff: https://github.com/llvm/llvm-project/pull/67636.diff

2 Files Affected:

  • (modified) lld/ELF/Arch/AVR.cpp (+1)
  • (added) lld/test/ELF/avr-errors.s (+27)
diff --git a/lld/ELF/Arch/AVR.cpp b/lld/ELF/Arch/AVR.cpp
index 4905d61796fa98f..d5cba4c70e8564b 100644
--- a/lld/ELF/Arch/AVR.cpp
+++ b/lld/ELF/Arch/AVR.cpp
@@ -238,6 +238,7 @@ void AVR::relocate(uint8_t *loc, const Relocation &rel, uint64_t val) const {
     break;
   }
   case R_AVR_13_PCREL: {
+    checkInt(loc, val, 13, rel);
     checkAlignment(loc, val, 2, rel);
     const uint16_t target = (val - 2) >> 1;
     write16le(loc, (read16le(loc) & 0xf000) | (target & 0xfff));
diff --git a/lld/test/ELF/avr-errors.s b/lld/test/ELF/avr-errors.s
new file mode 100644
index 000000000000000..bbf88f887a4466b
--- /dev/null
+++ b/lld/test/ELF/avr-errors.s
@@ -0,0 +1,27 @@
+; REQUIRES: avr
+; RUN: llvm-mc -filetype=obj -triple=avr -mcpu=atmega328 %s -o %t.o
+; RUN: not ld.lld %t.o -o %t 2>&1 -Ttext=0x1000 --defsym=callee0=0xf00 --defsym=callee1=0x2048 --defsym=callee2=0x100f | FileCheck %s
+
+.section .LDI,"ax",@progbits
+
+.globl __init
+__init:
+
+; CHECK:     relocation R_AVR_7_PCREL out of range: {{.*}} 'callee0'
+; CHECK-NOT: R_AVR_13_PCREL {{.*}} 'callee0'
+; CHECK-NOT: R_AVR_CALL {{.*}} 'callee0'
+brne  callee0
+rjmp  callee0
+jmp   callee0
+
+; CHECK:      relocation R_AVR_7_PCREL out of range: {{.*}} 'callee1'
+; CHECK:      relocation R_AVR_13_PCREL out of range: {{.*}} 'callee1'
+; CHECK-NOT:  R_AVR_CALL {{.*}} 'callee1'
+breq  callee1
+rcall callee1
+call  callee1
+
+; CHECK: improper alignment for relocation R_AVR_7_PCREL: {{.*}} not aligned to 2 bytes
+; CHECK: improper alignment for relocation R_AVR_13_PCREL: {{.*}} not aligned to 2 bytes
+brlt  callee2
+rcall callee2

@benshi001
Copy link
Member Author

benshi001 commented Sep 28, 2023

Here is a real world program that gnu-ld reports an error, but lld silently generate an incorrect executable:
#67042

lld/test/ELF/avr-errors.s Outdated Show resolved Hide resolved
lld/test/ELF/avr-errors.s Outdated Show resolved Hide resolved
lld/test/ELF/avr-errors.s Outdated Show resolved Hide resolved
lld/test/ELF/avr-errors.s Outdated Show resolved Hide resolved
lld/test/ELF/avr-errors.s Outdated Show resolved Hide resolved
lld should report explicit range and alignment errors for some relocations
other than silently generate incorrect ELF outputs.
@benshi001 benshi001 merged commit 488a62f into llvm:main Oct 6, 2023
3 checks passed
@benshi001 benshi001 deleted the avr-lld branch October 6, 2023 10:20
@aykevl
Copy link
Contributor

aykevl commented May 18, 2024

The change to R_AVR_7_PCREL broke linking when updating to LLVM 18:

ld.lld-18: error: /tmp/tinygo1961037610/main.lto.main.o:(function runtime.run$1$gowrapper: .text.runtime.run$1$gowrapper+0x4c0): relocation R_AVR_7_PCREL out of range: -66 is not in [-64, 63]; references section '.text.runtime.run$1$gowrapper'

When I undo that one line change using the following patch, all AVR tests pass:

   case R_AVR_7_PCREL: {
-    checkInt(loc, val - 2, 7, rel);
+    checkInt(loc, val, 7, rel);
     checkAlignment(loc, val, 2, rel);
     const uint16_t target = (val - 2) >> 1;
     write16le(loc, (read16le(loc) & 0xfc07) | ((target & 0x7f) << 3));
     break;
   }

@aykevl
Copy link
Contributor

aykevl commented May 19, 2024

A correct fix is the following:

--- a/lld/ELF/Arch/AVR.cpp
+++ b/lld/ELF/Arch/AVR.cpp
@@ -231,7 +231,7 @@ void AVR::relocate(uint8_t *loc, const Relocation &rel, uint64_t val) const {
 
   // Since every jump destination is word aligned we gain an extra bit
   case R_AVR_7_PCREL: {
-    checkInt(loc, val - 2, 7, rel);
+    checkInt(loc, val - 2, 8, rel);
     checkAlignment(loc, val, 2, rel);
     const uint16_t target = (val - 2) >> 1;
     write16le(loc, (read16le(loc) & 0xfc07) | ((target & 0x7f) << 3));

I've verified the behavior against avr-ld.
It's 8 (and not 7) because while the instruction has 7 bits for the location, it jumps in word sizes so has effectively 8 bits of range (in bytes).

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

Successfully merging this pull request may close these issues.

None yet

4 participants