[ELF] Fix reading of PC values of FDEs

[atanasyan]

Summary

The patch fixes two related problems:

• If CIE augmentation string has 'L' token the CIE contains a byte defines LSDA encoding. We should skip this byte in getFdeEncoding routine. Before this fix we do not skip it and if the next token is 'R' treat this byte as FDE encoding.
• FDE encoding format has separate flags e.g. DW_EH_PE_pcrel for definition of relative pointers. We should add .eh_frame address to the PC value iif the DW_EH_PE_pcrel is specified.

http://www.airs.com/blog/archives/460

There is one more not fixed problem in this code. If PC value is encoded using signed relative format e.g. DW_EH_PE_sdata4 | DW_EH_PE_pcrel we should sign extend result of read32 to perform calculation correctly. I am going to fix that in a separate patch.

[• rafael]

ELF/OutputSections.cpp

675 ↗

(On Diff #49411)

This might be shorter as a lambda so that you can just return PC.

test/ELF/eh-frame-hdr-abs-fde.s

21 ↗

(On Diff #49411)

Can you use objdump to get a parsed content?

[atanasyan]

ELF/OutputSections.cpp

675 ↗

(On Diff #49411)

Do you mean something like that?

auto Adjust = [=](uint64_t PC) {
  return IsRelative ? PC + EhVA + F.Off + 8 : PC;
};
...
case DW_EH_PE_sdata4:
  return Adjust(read32<E>(F.PCRel));

test/ELF/eh-frame-hdr-abs-fde.s

21 ↗

(On Diff #49411)

It looks like I need to fix the llvm-objdump first. Now it shows Augmentation data as a regular string but it might contain non-printable characters. GNU objdump shows it as hex numbers.

[atanasyan]

• Reduce size of code using lambda function
• Use llvm-objdump -dwarf=frames to make checking in the test more clear

[• rafael]

LGTM

[ruiu]

ELF/OutputSections.cpp

669–670 ↗

(On Diff #49537)

Do you have to use lambda? It seems you can write without it in a more regular way.

const endianness E = ELFT::TargetEndianness;

int Size = F.Enc & 0x7;
if (Size == DW_EH_PE_absptr)
  Size = sizeof(uintX_t) == 8 ? DW_EH_PE_udata8 : DW_EH_PE_udata4;

uintX_t V;
switch (Size) {
case DW_EH_PE_udata2:
  V = read16<E>(F.PCRel);
case DW_EH_PE_udata4:
  V = read32<E>(F.PCRel);
case DW_EH_PE_udata8:
  V = read64<E>(F.PCRel);
default:
  fatal("unknown FDE size encoding");
}

switch (F.Enc & 0x70) {
case DW_EH_PE_absptr:
  return V;
case DW_EH_PE_pcrel:
  return V + EhVA + F.Off + 8;
default:
  fatal("unknown FDE size relative encoding");
}

[atanasyan]

ELF/OutputSections.cpp

669–670 ↗

(On Diff #49537)

Do you have to use lambda? It seems you can write without it in a more regular way.

The lambda is not mandatory. But without it the code is a bit longer because we have to add break to each case of the switch (Size).

[• rafael]

Small preference for a lambda, but LGTM without it too if Rui prefers
it that way.

Cheers,
Rafael

[atanasyan]

Thanks for review. I have committed the code without lambda function.

lld/trunk/ELF/OutputSections.cpp

 // which are target specific, so we run this code after performing
 // all relocations. We read the values from ouput buffer according to the
 // encoding given for FDEs. Return value is an offset to the initial PC value
 // for the FDE.
 template <class ELFT>
 typename EhFrameHeader<ELFT>::uintX_t
 EhFrameHeader<ELFT>::getFdePc(uintX_t EhVA, const FdeData &F) {
   const endianness E = ELFT::TargetEndianness;
-  assert((F.Enc & 0xF0) != DW_EH_PE_datarel);
-
-  uintX_t FdeOff = EhVA + F.Off + 8;
-  switch (F.Enc & 0xF) {
+  uint8_t Size = F.Enc & 0x7;
+  if (Size == DW_EH_PE_absptr)
+    Size = sizeof(uintX_t) == 8 ? DW_EH_PE_udata8 : DW_EH_PE_udata4;
+  uint64_t PC;
+  switch (Size) {
   case DW_EH_PE_udata2:
-  case DW_EH_PE_sdata2:
-    return FdeOff + read16<E>(F.PCRel);
+    PC = read16<E>(F.PCRel);
+    break;
   case DW_EH_PE_udata4:
-  case DW_EH_PE_sdata4:
-    return FdeOff + read32<E>(F.PCRel);
+    PC = read32<E>(F.PCRel);
+    break;
   case DW_EH_PE_udata8:
-  case DW_EH_PE_sdata8:
-    return FdeOff + read64<E>(F.PCRel);
+    PC = read64<E>(F.PCRel);
+    break;
+  default:
+    fatal("unknown FDE size encoding");
+  }
+  switch (F.Enc & 0x70) {
   case DW_EH_PE_absptr:
-    if (sizeof(uintX_t) == 8)
-      return FdeOff + read64<E>(F.PCRel);
-    return FdeOff + read32<E>(F.PCRel);
+    return PC;
+  case DW_EH_PE_pcrel:
+    return PC + EhVA + F.Off + 8;
+  default:
+    fatal("unknown FDE size relative encoding");
   }
-  fatal("unknown FDE size encoding");
 }
 
 template <class ELFT> void EhFrameHeader<ELFT>::writeTo(uint8_t *Buf) {
   const endianness E = ELFT::TargetEndianness;
 
   const uint8_t Header[] = {1, DW_EH_PE_pcrel | DW_EH_PE_sdata4,
                             DW_EH_PE_udata4,
                             DW_EH_PE_datarel | DW_EH_PE_sdata4};
for (char C : Aug) {
     if (C == 'z') {
       skipLeb128(D);
       continue;
     }
     if (C == 'P') {
       skipAugP<ELFT>(D);
       continue;
     }
-    if (C == 'L')
+    if (C == 'L') {
+      readByte(D);
       continue;
+    }
     fatal("unknown .eh_frame augmentation string: " + Aug);
   }
   return DW_EH_PE_absptr;
 }
 
 template <class ELFT>
 static typename ELFFile<ELFT>::uintX_t readEntryLength(ArrayRef<uint8_t> D) {
   const endianness E = ELFT::TargetEndianness;

lld/trunk/test/ELF/eh-frame-hdr-abs-fde.s

+# Check reading PC values of FDEs and writing lookup table in the .eh_frame_hdr
+# if CIE augmentation string has 'L' token and PC values are encoded using
+# absolute (not relative) format.
+
+# RUN: llvm-mc -filetype=obj -triple=mips-unknown-linux %s -o %t.o
+# RUN: ld.lld --eh-frame-hdr %t.o -o %t
+# RUN: llvm-objdump -s -dwarf=frames %t | FileCheck %s
+
+# REQUIRES: mips
+
+# CHECK:      Contents of section .eh_frame_hdr:
+# CHECK-NEXT:  10158 011b033b ffffffcc 00000001 0000fea8
+#                                               ^-- 0x20000 - 0x10158
+#                                                   .text   - .eh_frame_hdr
+# CHECK-NEXT:  10168 ffffffe8
+# CHECK-NEXT: Contents of section .text:
+# CHECK-NEXT:  20000 00000000
+
+# CHECK: Augmentation:          "zLR"
+# CHECK: Augmentation data:     00 0B
+#                                  ^-- DW_EH_PE_udata4 | DW_EH_PE_signed
+
+  .text
+  .globl __start
+__start:
+  .cfi_startproc
+  .cfi_lsda 0, _ex
+  nop
+  .cfi_endproc
+
+  .data
+_ex:
+  .word 0