[ELF] Fix handling of FDE negative relative PC addr

[andrewng]

Summary

Signed values for the FDE PC addr were not correctly handled in
readFdeAddr(). If the value is negative and the type of the value is
smaller than 64 bits, the FDE PC addr overflow error would be
incorrectly triggered.

Fixed readFdeAddr() to properly handle signed values by sign extending
where appropriate.

[ruiu]

Thanks! How did yoy find this?

[grimar]

Nice catch! Few comments/suggestions are below.

test/ELF/eh-frame-negative-relative-pcaddr.s

1 ↗

(On Diff #156307)

Should this test case be in a linkerscript sub-folder? Since it relies on LS.

4 ↗

(On Diff #156307)

Maybe it worth to check the .eh_frame_hdr section content like eh-frame-value-format8.s/eh-frame-value-format7.s
tests do? To demonstrate that we encoded the address correctly and were able to build a valid eh_frame_hdr entry.

10 ↗

(On Diff #156307)

I think this can be a single line:

SECTIONS { .text : { *(.text) } .eh_frame : { *(.eh_frame) } }

[grimar]

Also, what about adding 3 test cases? A one for each new type you added (DW_EH_PE_sdata2, DW_EH_PE_sdata4, DW_EH_PE_sdata8).

[andrewng]

In D49557#1168549, @ruiu wrote:

Thanks! How did yoy find this?

Our LLD port uses linker scripts where .eh_frame is placed after .text, so a number of our test cases started failing with the "PC address is too large" error from commit r337382. That led me to finding this issue.

In D49557#1169389, @grimar wrote:

Also, what about adding 3 test cases? A one for each new type you added (DW_EH_PE_sdata2, DW_EH_PE_sdata4, DW_EH_PE_sdata8).

Yes, that's a good idea, although having had a quick look, I'm not too sure we have test coverage for all the other types either. Also I think some of the .eh_frame value format tests are using invalid augmentation strings, but these issues aren't related to this particular patch.

[andrewng]

Updated testing as per review comments.

[andrewng]

Update to the eh-frame-negative-pcrel-sdata8.s test.

[ruiu]

LGTM

ELF/SyntheticSections.cpp

524–525 ↗

(On Diff #156531)

nit: move this above udata4 so that they are sorted by size.

lld/trunk/ELF/SyntheticSections.cpp

std::vector<EhFrameSection::FdeData> EhFrameSection::getFdeData() const {
 
   return Ret;
 }
 
 static uint64_t readFdeAddr(uint8_t *Buf, int Size) {
   switch (Size) {
   case DW_EH_PE_udata2:
     return read16(Buf);
+  case DW_EH_PE_sdata2:
+    return (int16_t)read16(Buf);
   case DW_EH_PE_udata4:
     return read32(Buf);
+  case DW_EH_PE_sdata4:
+    return (int32_t)read32(Buf);
   case DW_EH_PE_udata8:
+  case DW_EH_PE_sdata8:
     return read64(Buf);
   case DW_EH_PE_absptr:
     return readUint(Buf);
   }
   fatal("unknown FDE size encoding");
 }
 
 // Returns the VA to which a given FDE (on a mmap'ed buffer) is applied to.
 // We need it to create .eh_frame_hdr section.
 uint64_t EhFrameSection::getFdePc(uint8_t *Buf, size_t FdeOff,
                                   uint8_t Enc) const {
   // The starting address to which this FDE applies is
   // stored at FDE + 8 byte.
   size_t Off = FdeOff + 8;
-  uint64_t Addr = readFdeAddr(Buf + Off, Enc & 0x7);
+  uint64_t Addr = readFdeAddr(Buf + Off, Enc & 0xf);
   if ((Enc & 0x70) == DW_EH_PE_absptr)
     return Addr;
   if ((Enc & 0x70) == DW_EH_PE_pcrel)
     return Addr + getParent()->Addr + Off;
   fatal("unknown FDE size relative encoding");
 }
 
 void EhFrameSection::writeTo(uint8_t *Buf) {

lld/trunk/test/ELF/eh-frame-negative-pcrel-sdata2.s

+# REQUIRES: x86
+
+# Test handling of FDE pc negative relative addressing with DW_EH_PE_sdata2.
+# This situation can arise when .eh_frame is placed after .text.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: echo "SECTIONS { .text : { *(.text) } .eh_frame : { *(.eh_frame) } }" > %t.script
+# RUN: ld.lld --eh-frame-hdr --script %t.script --section-start .text=0x1000 %t.o -o %t
+# RUN: llvm-readobj -s -section-data %t | FileCheck %s
+
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .eh_frame
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address: 0x1001
+# CHECK-NEXT:   Offset: 0x1001
+# CHECK-NEXT:   Size:
+# CHECK-NEXT:   Link:
+# CHECK-NEXT:   Info:
+# CHECK-NEXT:   AddressAlignment:
+# CHECK-NEXT:   EntrySize:
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 14000000 00000000 017A5200 01010101
+# CHECK-NEXT:     0010: 1A000000 00000000 0C000000 1C000000
+# CHECK-NEXT:     0020: DFFFFFFF
+#                       ^
+#   DFFFFFFF = _start(0x1000) - PC(.eh_frame(0x1001) + 0x20)
+
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .eh_frame_hdr
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address: 0x1030
+# CHECK-NEXT:   Offset: 0x1030
+# CHECK-NEXT:   Size: 20
+# CHECK-NEXT:   Link: 0
+# CHECK-NEXT:   Info: 0
+# CHECK-NEXT:   AddressAlignment: 4
+# CHECK-NEXT:   EntrySize: 0
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 011B033B CDFFFFFF 01000000 D0FFFFFF
+# CHECK-NEXT:     0010: E9FFFFFF
+#   Header (always 4 bytes): 011B033B
+#   CDFFFFFF = .eh_frame(0x1001) - .eh_frame_hdr(0x1030) - 4
+#   01000000 = 1 = the number of FDE pointers in the table.
+#   D0FFFFFF = _start(0x1000) - .eh_frame_hdr(0x1030)
+#   E9FFFFFF = FDE(.eh_frame(0x1001) + 0x18) - .eh_frame_hdr(0x1030)
+
+.text
+.global _start
+_start:
+ nop
+
+.section .eh_frame, "a"
+  .long 16   # Size
+  .long 0x00 # ID
+  .byte 0x01 # Version.
+
+  .byte 0x7A # Augmentation string: "zR"
+  .byte 0x52
+  .byte 0x00
+
+  .byte 0x01
+
+  .byte 0x01 # LEB128
+  .byte 0x01 # LEB128
+
+  .byte 0x01 # LEB128
+  .byte 0x1A # DW_EH_PE_pcrel | DW_EH_PE_sdata2
+
+  .byte 0x00
+  .byte 0x00
+  .byte 0x00
+
+  .long 10   # Size
+  .long 24   # ID
+fde:
+  .long _start - fde
+  .word 0

lld/trunk/test/ELF/eh-frame-negative-pcrel-sdata4.s

+# REQUIRES: x86
+
+# Test handling of FDE pc negative relative addressing with DW_EH_PE_sdata4.
+# This situation can arise when .eh_frame is placed after .text.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: echo "SECTIONS { .text : { *(.text) } .eh_frame : { *(.eh_frame) } }" > %t.script
+# RUN: ld.lld --eh-frame-hdr --script %t.script --section-start .text=0x1000 %t.o -o %t
+# RUN: llvm-readobj -s -section-data %t | FileCheck %s
+
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .eh_frame
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address: 0x1001
+# CHECK-NEXT:   Offset: 0x1001
+# CHECK-NEXT:   Size:
+# CHECK-NEXT:   Link:
+# CHECK-NEXT:   Info:
+# CHECK-NEXT:   AddressAlignment:
+# CHECK-NEXT:   EntrySize:
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 14000000 00000000 017A5200 01010101
+# CHECK-NEXT:     0010: 1B000000 00000000 0C000000 1C000000
+# CHECK-NEXT:     0020: DFFFFFFF
+#                       ^
+#   DFFFFFFF = _start(0x1000) - PC(.eh_frame(0x1001) + 0x20)
+
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .eh_frame_hdr
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address: 0x1030
+# CHECK-NEXT:   Offset: 0x1030
+# CHECK-NEXT:   Size: 20
+# CHECK-NEXT:   Link: 0
+# CHECK-NEXT:   Info: 0
+# CHECK-NEXT:   AddressAlignment: 4
+# CHECK-NEXT:   EntrySize: 0
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 011B033B CDFFFFFF 01000000 D0FFFFFF
+# CHECK-NEXT:     0010: E9FFFFFF
+#   Header (always 4 bytes): 011B033B
+#   CDFFFFFF = .eh_frame(0x1001) - .eh_frame_hdr(0x1030) - 4
+#   01000000 = 1 = the number of FDE pointers in the table.
+#   D0FFFFFF = _start(0x1000) - .eh_frame_hdr(0x1030)
+#   E9FFFFFF = FDE(.eh_frame(0x1001) + 0x18) - .eh_frame_hdr(0x1030)
+
+.text
+.global _start
+_start:
+ nop
+
+.section .eh_frame, "a"
+  .long 16   # Size
+  .long 0x00 # ID
+  .byte 0x01 # Version.
+
+  .byte 0x7A # Augmentation string: "zR"
+  .byte 0x52
+  .byte 0x00
+
+  .byte 0x01
+
+  .byte 0x01 # LEB128
+  .byte 0x01 # LEB128
+
+  .byte 0x01 # LEB128
+  .byte 0x1B # DW_EH_PE_pcrel | DW_EH_PE_sdata4
+
+  .byte 0x00
+  .byte 0x00
+  .byte 0x00
+
+  .long 12   # Size
+  .long 24   # ID
+fde:
+  .long _start - fde
+  .long 0

lld/trunk/test/ELF/eh-frame-negative-pcrel-sdata8.s

+# REQUIRES: x86
+
+# Test handling of FDE pc negative relative addressing with DW_EH_PE_sdata8.
+# This situation can arise when .eh_frame is placed after .text.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: echo "SECTIONS { .text : { *(.text) } .eh_frame : { *(.eh_frame) } }" > %t.script
+# RUN: ld.lld --eh-frame-hdr --script %t.script --section-start .text=0x1000 %t.o -o %t
+# RUN: llvm-readobj -s -section-data %t | FileCheck %s
+
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .eh_frame
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address: 0x1001
+# CHECK-NEXT:   Offset: 0x1001
+# CHECK-NEXT:   Size:
+# CHECK-NEXT:   Link:
+# CHECK-NEXT:   Info:
+# CHECK-NEXT:   AddressAlignment:
+# CHECK-NEXT:   EntrySize:
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 14000000 00000000 017A5200 01010101
+# CHECK-NEXT:     0010: 1C000000 00000000 14000000 1C000000
+# CHECK-NEXT:     0020: DFFFFFFF FFFFFFFF
+#                       ^
+#   DFFFFFFF FFFFFFFF = _start(0x1000) - PC(.eh_frame(0x1001) + 0x20)
+
+# CHECK:      Section {
+# CHECK:        Index:
+# CHECK:        Name: .eh_frame_hdr
+# CHECK-NEXT:   Type: SHT_PROGBITS
+# CHECK-NEXT:   Flags [
+# CHECK-NEXT:     SHF_ALLOC
+# CHECK-NEXT:   ]
+# CHECK-NEXT:   Address: 0x1038
+# CHECK-NEXT:   Offset: 0x1038
+# CHECK-NEXT:   Size: 20
+# CHECK-NEXT:   Link: 0
+# CHECK-NEXT:   Info: 0
+# CHECK-NEXT:   AddressAlignment: 4
+# CHECK-NEXT:   EntrySize: 0
+# CHECK-NEXT:   SectionData (
+# CHECK-NEXT:     0000: 011B033B C5FFFFFF 01000000 C8FFFFFF
+# CHECK-NEXT:     0010: E1FFFFFF
+#   Header (always 4 bytes): 011B033B
+#   C5FFFFFF = .eh_frame(0x1001) - .eh_frame_hdr(0x1038) - 4
+#   01000000 = 1 = the number of FDE pointers in the table.
+#   C8FFFFFF = _start(0x1000) - .eh_frame_hdr(0x1038)
+#   E1FFFFFF = FDE(.eh_frame(0x1001) + 0x18) - .eh_frame_hdr(0x1038)
+
+.text
+.global _start
+_start:
+ nop
+
+.section .eh_frame, "a"
+  .long 16   # Size
+  .long 0x00 # ID
+  .byte 0x01 # Version.
+
+  .byte 0x7A # Augmentation string: "zR"
+  .byte 0x52
+  .byte 0x00
+
+  .byte 0x01
+
+  .byte 0x01 # LEB128
+  .byte 0x01 # LEB128
+
+  .byte 0x01 # LEB128
+  .byte 0x1C # DW_EH_PE_pcrel | DW_EH_PE_sdata8
+
+  .byte 0x00
+  .byte 0x00
+  .byte 0x00
+
+  .long 16   # Size
+  .long 24   # ID
+fde:
+  .quad _start - fde
+  .long 0