Fix R_AVR_7_PCREL and R_AVR_13_PCREL range checking #92695

aykevl · 2024-05-19T13:00:06Z

See the commit messages for detail. The first one fixes a bug I found while updating to LLVM 18 (see #67636 (comment)), the second one fixes a bug I found while fixing the first.

I'm not sure whether sending two commits in a single PR like this is allowed, but since they're related (but fix different bugs) it made sense to me to do it in one PR.

R_AVR_7_PCREL has 7 bits available, but because it works in instruction words the actual range is 256 bytes (-128..126 bytes). This fixes a bug introduced in llvm#67636 that caused code generated by the compiler to be rejected by the linker. There is still a bug in the compiler: it can use one more bit for R_AVR_7_PCREL than it currently does. But that should be fixed separately.

llvmbot · 2024-05-19T13:00:40Z

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Ayke (aykevl)

Changes

See the commit messages for detail. The first one fixes a bug I found while updating to LLVM 18 (see #67636 (comment)), the second one fixes a bug I found while fixing the first.

I'm not sure whether sending two commits in a single PR like this is allowed, but since they're related (but fix different bugs) it made sense to me to do it in one PR.

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

3 Files Affected:

(modified) lld/ELF/Arch/AVR.cpp (+1-2)
(modified) lld/test/ELF/avr-reloc-error.s (+2-2)
(modified) lld/test/ELF/avr-reloc.s (+12)

diff --git a/lld/ELF/Arch/AVR.cpp b/lld/ELF/Arch/AVR.cpp
index 9211eabc9669a..2275f86942871 100644
--- a/lld/ELF/Arch/AVR.cpp
+++ b/lld/ELF/Arch/AVR.cpp
@@ -231,14 +231,13 @@ 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));
     break;
   }
   case R_AVR_13_PCREL: {
-    checkInt(loc, val - 2, 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-reloc-error.s b/lld/test/ELF/avr-reloc-error.s
index 0a30f68d168e2..4e49adfd78b7e 100644
--- a/lld/test/ELF/avr-reloc-error.s
+++ b/lld/test/ELF/avr-reloc-error.s
@@ -3,7 +3,7 @@
 # RUN: rm -rf %t && split-file %s %t && cd %t
 
 # RUN: llvm-mc -filetype=obj -triple=avr -mcpu=atmega328 avr-pcrel-7.s -o avr-pcrel-7.o
-# RUN: not ld.lld avr-pcrel-7.o -o /dev/null -Ttext=0x1000 --defsym=callee0=0x1040 --defsym=callee1=0x1044 --defsym=callee2=0x100f 2>&1 | \
+# RUN: not ld.lld avr-pcrel-7.o -o /dev/null -Ttext=0x1000 --defsym=callee0=0x1040 --defsym=callee1=0x1084 --defsym=callee2=0x100f 2>&1 | \
 # RUN:     FileCheck %s --check-prefix=PCREL7
 # RUN: llvm-mc -filetype=obj -triple=avr -mcpu=atmega328 avr-pcrel-13.s -o avr-pcrel-13.o
 # RUN: not ld.lld avr-pcrel-13.o -o /dev/null -Ttext=0x1000 --defsym=callee0=0x2000 --defsym=callee1=0x2004 --defsym=callee2=0x100f 2>&1 | \
@@ -20,7 +20,7 @@
 __start:
 
 # PCREL7-NOT: callee0
-# PCREL7:     error: {{.*}} relocation R_AVR_7_PCREL out of range: {{.*}} is not in [-64, 63]; references 'callee1'
+# PCREL7:     error: {{.*}} relocation R_AVR_7_PCREL out of range: {{.*}} is not in [-128, 127]; references 'callee1'
 # PCREL7:     error: {{.*}} improper alignment for relocation R_AVR_7_PCREL: {{.*}} is not aligned to 2 bytes
 brne callee0
 breq callee1
diff --git a/lld/test/ELF/avr-reloc.s b/lld/test/ELF/avr-reloc.s
index 172c0e03ba74b..ec088eaa149d0 100644
--- a/lld/test/ELF/avr-reloc.s
+++ b/lld/test/ELF/avr-reloc.s
@@ -82,6 +82,12 @@ sbic  b, 1    ; R_AVR_PORT5
 ; CHECK-NEXT:  rjmp .-36
 ; CHECK-NEXT:  breq .+26
 ; CHECK-NEXT:  breq .-40
+; CHECK-NEXT:  rjmp .-4096
+; CHECK-NEXT:  rjmp .+4094
+; CHECK-NEXT:  rjmp .+4094
+; CHECK-NEXT:  rjmp .-4096
+; CHECK-NEXT:  breq .-128
+; CHECK-NEXT:  breq .+126
 ; HEX-LABEL:   section .PCREL:
 ; HEX-NEXT:    0fc0eecf 69f061f3
 foo:
@@ -89,6 +95,12 @@ rjmp foo + 32  ; R_AVR_13_PCREL
 rjmp foo - 32  ; R_AVR_13_PCREL
 breq foo + 32  ; R_AVR_7_PCREL
 breq foo - 32  ; R_AVR_7_PCREL
+rjmp 1f - 4096  $ 1:  ; R_AVR_13_PCREL
+rjmp 1f + 4094  $ 1:  ; R_AVR_13_PCREL
+rjmp 1f - 4098  $ 1:  ; R_AVR_13_PCREL (overflow)
+rjmp 1f + 4096  $ 1:  ; R_AVR_13_PCREL (overflow)
+breq 1f - 128   $ 1:  ; R_AVR_7_PCREL
+breq 1f + 126   $ 1:  ; R_AVR_7_PCREL
 
 .section .LDSSTS,"ax",@progbits
 ; CHECK-LABEL: section .LDSSTS:

aykevl · 2024-05-19T13:03:49Z

lld/test/ELF/avr-reloc.s

+rjmp 1f - 4096  $ 1:  ; R_AVR_13_PCREL
+rjmp 1f + 4094  $ 1:  ; R_AVR_13_PCREL
+rjmp 1f - 4098  $ 1:  ; R_AVR_13_PCREL (overflow)
+rjmp 1f + 4096  $ 1:  ; R_AVR_13_PCREL (overflow)
+breq 1f - 128   $ 1:  ; R_AVR_7_PCREL
+breq 1f + 126   $ 1:  ; R_AVR_7_PCREL


For those not familiar with this syntax, the $ symbol works as a newline.
So this code:

rjmp 1f - 4096 $ 1:

Is equivalent to the following:

rjmp 1f - 4096 1:

(where 1f refers to the next anonymous label numbered "1")

This matches the behavior of avr-ld, and is needed for devices like the ATtiny85 which only have rjmp/rcall (no jmp/call) but have 8kB of flash memory. Without this change, jumps over 4kB would not be possible on the ATtiny85.

benshi001 · 2024-05-20T01:35:52Z

I agree with the change to R_AVR_PCREL_7, there are 7 bits in the RJMP/RCALL instruction encoding which actually represents [-128, 126].

aykevl · 2024-05-20T12:15:20Z

Thanks for the review!
@benshi001 do you also agree with the R_AVR_13_PCREL change?

benshi001 · 2024-05-20T12:44:05Z

Thanks for the review! @benshi001 do you also agree with the R_AVR_13_PCREL change?

I would like to keep the R_AVR_13_PCREL part unchanged, could you please give me an example that this check is harm ? Even on attiny85 you mentioned, the range of RJMP is still [-4096, 4094], so I think the check is necessary.

benshi001 · 2024-05-20T12:46:22Z

I wonder how RJMP can jumps over 4kB on the ATtiny85 device. Is RJMP has a different encoding against other devices ?

If avr-ld allows an overflow, I do not think we should follow avr-ld's behavior, let lld give an error is better than silently generate incorrect code.

aykevl · 2024-05-21T16:22:01Z

I wonder how RJMP can jumps over 4kB on the ATtiny85 device. Is RJMP has a different encoding against other devices ?

I can't quickly find it in the instruction set manual, but I did find this:

For AVR microcontrollers with Program memory not exceeding 4K words (8KB) this instruction can address the entire memory from every address location.

I assume it wraps around, because that's the only way I see that would make it possible to address the full 8kB of memory.
So for example if there's a rjmp .+4094 at address 6114, it will jump to (6114 + 4094 + 2) = 10210, which wraps around to address 2018. (The +2 is the size of the instruction itself).

I also found this in the attiny85 datasheet:

The ATtiny25/45/85 Program Counter (PC) is 10/11/12 bits wide, thus addressing the 1024/2048/4096 Program memory locations

This means that the attiny85 program counter is 12 bits, which means that it will indeed wrap around (12 bits is 4096 words, is 8192 bytes).

For avr-ld, there's an option to enable/disable wraparound (but it appears that wraparound is enabled by default):
https://sourceware.org/binutils/docs-2.30/as/AVR-Options.html. See -mno-wrap. I don't know whether we can add a flag like that to lld, I'd much prefer if we didn't need a special chip-specific flag for this (in my opinion a better solution would be two different relocations: one with wraparound and one without - but that would break compatibility with avr-ld).

benshi001 · 2024-05-22T06:28:23Z

I wonder how RJMP can jumps over 4kB on the ATtiny85 device. Is RJMP has a different encoding against other devices ?

I can't quickly find it in the instruction set manual, but I did find this:

For AVR microcontrollers with Program memory not exceeding 4K words (8KB) this instruction can address the entire memory from every address location.

I assume it wraps around, because that's the only way I see that would make it possible to address the full 8kB of memory. So for example if there's a rjmp .+4094 at address 6114, it will jump to (6114 + 4094 + 2) = 10210, which wraps around to address 2018. (The +2 is the size of the instruction itself).

I also found this in the attiny85 datasheet:

The ATtiny25/45/85 Program Counter (PC) is 10/11/12 bits wide, thus addressing the 1024/2048/4096 Program memory locations

This means that the attiny85 program counter is 12 bits, which means that it will indeed wrap around (12 bits is 4096 words, is 8192 bytes).

For avr-ld, there's an option to enable/disable wraparound (but it appears that wraparound is enabled by default): https://sourceware.org/binutils/docs-2.30/as/AVR-Options.html. See -mno-wrap. I don't know whether we can add a flag like that to lld, I'd much prefer if we didn't need a special chip-specific flag for this (in my opinion a better solution would be two different relocations: one with wraparound and one without - but that would break compatibility with avr-ld).

LGTM!

benshi001 · 2024-05-20T01:34:02Z

lld/ELF/Arch/AVR.cpp

@@ -238,7 +238,6 @@ void AVR::relocate(uint8_t *loc, const Relocation &rel, uint64_t val) const {
    break;
  }
  case R_AVR_13_PCREL: {
-    checkInt(loc, val - 2, 13, rel);


I did not catch your point at here, indeed RJMP/RCALL have a range limit of [-4096, 4094], why we can not check it for attiny85 ? And How can it jump over 4KB with RJMP on attiny85?

Is there an example, that RJMP can exceeds 4094 bytes on attiny85, which is allowed by avr-ld ?

Looks like these were older comments?

aykevl · 2024-05-22T10:56:27Z

Thank you both for the review!

Can someone merge this PR? (I don't have commit access anymore). @MaskRay?

Fix incorrect range check of R_AVR_7_PCREL. R_AVR_7_PCREL has 7 bits available, but it works in instruction words and the actual range is [-128, 126]. Disable range check of R_AVR_13_PCREL. This matches the behavior of avr-ld, and is needed for devices like the ATtiny85 which only have rjmp/rcall but have 8KiB flash memory.

aykevl requested review from MaskRay and benshi001 May 19, 2024 13:00

llvmbot added lld lld:ELF labels May 19, 2024

aykevl commented May 19, 2024

View reviewed changes

[lld][AVR] Allow R_AVR_13_PCREL to overflow

7a64998

This matches the behavior of avr-ld, and is needed for devices like the ATtiny85 which only have rjmp/rcall (no jmp/call) but have 8kB of flash memory. Without this change, jumps over 4kB would not be possible on the ATtiny85.

aykevl force-pushed the avr-lld-pcrel7 branch from d863f64 to 7a64998 Compare May 19, 2024 13:08

MaskRay approved these changes May 19, 2024

View reviewed changes

aykevl mentioned this pull request May 22, 2024

LLVM 18 support tinygo-org/tinygo#4271

Merged

benshi001 approved these changes May 22, 2024

View reviewed changes

benshi001 merged commit 5ae8567 into llvm:main May 23, 2024
4 checks passed

aykevl deleted the avr-lld-pcrel7 branch May 23, 2024 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix R_AVR_7_PCREL and R_AVR_13_PCREL range checking #92695

Fix R_AVR_7_PCREL and R_AVR_13_PCREL range checking #92695

aykevl commented May 19, 2024

llvmbot commented May 19, 2024 •

edited

aykevl May 19, 2024

benshi001 commented May 20, 2024 •

edited

aykevl commented May 20, 2024

benshi001 commented May 20, 2024

benshi001 commented May 20, 2024 •

edited

aykevl commented May 21, 2024

benshi001 commented May 22, 2024

benshi001 May 20, 2024

benshi001 May 20, 2024

aykevl May 22, 2024

aykevl commented May 22, 2024

Fix R_AVR_7_PCREL and R_AVR_13_PCREL range checking #92695

Fix R_AVR_7_PCREL and R_AVR_13_PCREL range checking #92695

Conversation

aykevl commented May 19, 2024

llvmbot commented May 19, 2024 • edited

aykevl May 19, 2024

Choose a reason for hiding this comment

benshi001 commented May 20, 2024 • edited

aykevl commented May 20, 2024

benshi001 commented May 20, 2024

benshi001 commented May 20, 2024 • edited

aykevl commented May 21, 2024

benshi001 commented May 22, 2024

benshi001 May 20, 2024

Choose a reason for hiding this comment

benshi001 May 20, 2024

Choose a reason for hiding this comment

aykevl May 22, 2024

Choose a reason for hiding this comment

aykevl commented May 22, 2024

llvmbot commented May 19, 2024 •

edited

benshi001 commented May 20, 2024 •

edited

benshi001 commented May 20, 2024 •

edited