[DebugInfo] do not add pc value if lsda value in fde is 0

[dongAxis1944]

Summary

Summary
The code might occur error in the following code:
This CIE shows the FDE related to the CIE has LSDA and the encoding of LSDA is DW_EH_PE_pcrel.
But the LDSA value is zero in FDE, in this case llvm should set lsda value to 0 even if LSDA is relative to pc.

Test Plan:

check-llvm

[dongAxis1944]

@rafaelauler. this case is related to the issue https://github.com/facebookincubator/BOLT/issues/141 in Bolt project.

[probinson]

A test plan of check-llvm is not sufficient. It shows that you didn't break anything that we have an existing test for, but it would also suggest that there is no test for this case, and you want that so someone else doesn't break your change in the future.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

1140

For a bool computation that is used only once and not in a loop, a local variable rather than a lambda seems simpler and more efficient.

1142

It seems that DW_EH_PE_aligned is overloading the DW_EH_PE_pcrel flag, and that's why you're doing the test this way? I see DWARFDataExtractor::getEncodedPointer is doing something similar.
Please add an enum constant to Dwarf.h for the 0x70 mask, and use that.

[rafauler]

Hi @dongAxis1944, thanks for submitting this!

But the LDSA value is zero in FDE, in this case llvm should set lsda value to 0 even if LSDA is relative to pc.

I don't understand why is zero special? We know it looks wrong because no LSDA would ever be encoded with a zero offset from the current address. But in that logic, 1 or 2 is also wrong, and so on. This looks like a special case to account for a bug in the compiler, is that right? What is the compiler that is generating that and why? It would be good to get to the bottom of that before special casing the parser library.

I'm saying this because if the violating FDE entry with LSDA set to 0 is being generated by source code written in assembly language that just happens to be coded by someone writing incorrect FDE metadata, it seems wrong to adapt the parser library to swallow that mistake -- and the source code should be fixed instead.

[dongAxis1944]

In D101353#2719773, @probinson wrote:

A test plan of check-llvm is not sufficient. It shows that you didn't break anything that we have an existing test for, but it would also suggest that there is no test for this case, and you want that so someone else doesn't break your change in the future.

@probinson thanks for reviewing!
Since the current LLVM code does not handle DW_EH_PE_pcrel well, because the EHFrameAddress is always 0.
But considering the code from the bolt, EHFrameAddress is not 0.

I will upload the more ut for this case.

[dongAxis1944]

In D101353#2720517, @rafauler wrote:

Hi @dongAxis1944, thanks for submitting this!

But the LDSA value is zero in FDE, in this case llvm should set lsda value to 0 even if LSDA is relative to pc.

I don't understand why is zero special? We know it looks wrong because no LSDA would ever be encoded with a zero offset from the current address. But in that logic, 1 or 2 is also wrong, and so on. This looks like a special case to account for a bug in the compiler, is that right? What is the compiler that is generating that and why? It would be good to get to the bottom of that before special casing the parser library.

I'm saying this because if the violating FDE entry with LSDA set to 0 is being generated by source code written in assembly language that just happens to be coded by someone writing incorrect FDE metadata, it seems wrong to adapt the parser library to swallow that mistake -- and the source code should be fixed instead.

Thanks for reviewing, I think zero is very special, it means although cie shows fde should have lsda, but it should not have.

In D101353#2720517, @rafauler wrote:

Hi @dongAxis1944, thanks for submitting this!

But the LDSA value is zero in FDE, in this case llvm should set lsda value to 0 even if LSDA is relative to pc.

I don't understand why is zero special? We know it looks wrong because no LSDA would ever be encoded with a zero offset from the current address. But in that logic, 1 or 2 is also wrong, and so on. This looks like a special case to account for a bug in the compiler, is that right? What is the compiler that is generating that and why? It would be good to get to the bottom of that before special casing the parser library.

I'm saying this because if the violating FDE entry with LSDA set to 0 is being generated by source code written in assembly language that just happens to be coded by someone writing incorrect FDE metadata, it seems wrong to adapt the parser library to swallow that mistake -- and the source code should be fixed instead.

hi @rafauler, thanks for reviewing.
I think 0 is a special case, I think it means although the CIE should have lsda and related to PC, but if the value of lsda is 0, it should not add pc value.

But in that logic, 1 or 2 is also wrong, and so on.

Yes, but I do not have the address and size of gcc_exception_table, so I can not judge whether it is right or not if it is not zero.

[dongAxis1944]

Address the comments and add the unit test

llvm/include/llvm/BinaryFormat/Dwarf.h

   DW_EH_PE_indirect = 0x80
 };
 
+constexpr uint32_t DW_EH_ENCODING_TYPE = 0x0f;
+constexpr uint32_t DW_EH_REL_TYPE = 0x70;
+
 /// Constants for the DW_APPLE_PROPERTY_attributes attribute.
 /// Keep this list in sync with clang's DeclObjCCommon.h
 /// ObjCPropertyAttribute::Kind!

llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp

   uint64_t Result = 0;
   uint64_t OldOffset = *Offset;
   // First get value
-  switch (Encoding & 0x0F) {
+  switch (Encoding & dwarf::DW_EH_ENCODING_TYPE) {
   case dwarf::DW_EH_PE_absptr:
     switch (getAddressSize()) {
     case 2:

     return None;
   }
   // Then add relative offset, if required
-  switch (Encoding & 0x70) {
+  switch (Encoding & dwarf::DW_EH_REL_TYPE) {
   case dwarf::DW_EH_PE_absptr:
     // do nothing
     break;

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

 
           // Decode the LSDA if the CIE augmentation string said we should.
           if (Cie->getLSDAPointerEncoding() != DW_EH_PE_omit) {
+            auto CurPC = Offset + EHFrameAddress;
             LSDAAddress = Data.getEncodedPointer(
-                &Offset, Cie->getLSDAPointerEncoding(),
-                EHFrameAddress ? Offset + EHFrameAddress : 0);
+                &Offset, Cie->getLSDAPointerEncoding(), 0);
+            bool IsPCRelative =
+                (dwarf::DW_EH_REL_TYPE & Cie->getLSDAPointerEncoding()) ==
+                dwarf::DW_EH_PE_pcrel;
+            if (LSDAAddress && *LSDAAddress != 0 && IsPCRelative)
+              LSDAAddress = *LSDAAddress + CurPC;
           }
 
           if (Offset != EndAugmentationOffset)

llvm/test/DebugInfo/X86/Inputs/eh_frame.yaml

+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+  Entry:           0x00000000004004FA
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x00000000004003E0
+    AddressAlign:    0x0000000000000010
+    Content:         31ED4989D15E4889E24883E4F0505449C7C07005400048C7C10005400048C7C7FA044000E8B7FFFFFFF4660F1F440000B82F10600055482D281060004883F80E4889E577025DC3B8000000004885C074F45DBF28106000FFE00F1F8000000000B82810600055482D2810600048C1F8034889E54889C248C1EA3F4801D048D1F875025DC3BA000000004885D274F45D4889C6BF28106000FFE20F1F8000000000803D9D0B2000007511554889E5E87EFFFFFF5DC6058A0B200001F3C30F1F400048833D7809200000741EB8000000004885C0741455BF200E60004889E5FFD05DE97BFFFFFF0F1F00E973FFFFFF648B0425B0FCFFFF39C70F850C0000004839160F850400000048890EC3B8FFFFFFFFC34839FE0F84F0FFFFFFC34831C0C3669041574189FF41564989F641554989D541544C8D25F808200055488D2DF8082000534C29E531DB48C1FD034883EC08E85DFEFFFF4885ED741E0F1F8400000000004C89EA4C89F64489FF41FF14DC4883C3014839EB75EA4883C4085B5D415C415D415E415FC390662E0F1F840000000000F3C3
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Flags:           [ SHF_INFO_LINK ]
+    Link:            .symtab
+    AddressAlign:    0x0000000000000008
+    Info:            .text
+    Relocations:
+  - Name:            .rodata
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000400580
+    AddressAlign:    0x0000000000000008
+    Content:         '01000200000000000000000000000000'
+  - Name:            .dynamic
+    Type:            SHT_DYNAMIC
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    Address:         0x0000000000600E28
+    Link:            .dynstr
+    AddressAlign:    0x0000000000000008
+    Content:         010000000000000001000000000000000C0000000000000090034000000000000D0000000000000074054000000000001900000000000000100E6000000000001B0000000000000008000000000000001A00000000000000180E6000000000001C000000000000000800000000000000F5FEFF6F000000009802400000000000050000000000000000034000000000000600000000000000B8024000000000000A0000000000000038000000000000000B0000000000000018000000000000001500000000000000000000000000000003000000000000000010600000000000020000000000000018000000000000001400000000000000070000000000000017000000000000007803400000000000070000000000000060034000000000000800000000000000180000000000000009000000000000001800000000000000FEFFFF6F000000004003400000000000FFFFFF6F000000000100000000000000F0FFFF6F000000003803400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
+  - Name:            .eh_frame
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Address:         0x0000000000400700
+    AddressAlign:    0x0000000000000008
+
+# eh_frame dump info
+#   .section .eh_frame,"a",@unwind
+#
+#         .long .LCIEEnd - .LCIEStart    # Length
+# .LCIEStart:
+#         .long 0xffffffff        # CIE pointer
+#         .byte 5                 # version
+#         .byte 0x7A              # Augmentation string: "zL"
+#         .byte 0x4C
+#         .byte 0x00
+#         .byte 0x01              # Code alignment factor, ULEB128
+#         .byte 0x01              # Data alignment factor, ULEB128
+#         .byte 0xF0              # Return address register, ubyte for version 1.
+#         .byte 0x01              # LEB128
+#         .byte 0x1A              # DW_EH_PE_pcrel | DW_EH_PE_sdata2
+#         .byte 0x00
+#         .byte 0x00
+#         .byte 0x00
+# .LCIEEnd
+#         .long 10                # Size
+#         .long 0                 # ID
+#         .long .Lend - .LCIEptr  # Length
+# .LCIEptr:
+#         .long 0xffffffff        # CIE pointer
+#         .quad 0x1111abcd        # Initial location
+#         .quad 0x00010000        # Address range
+#         .quad 0x00000000
+#         .quad 0x00000000
+# .Lend:
+
+    Content:         '1200000000000000057a4c000800010101011a0000001d0000001a000000cdab1111000000000000010000000000020000000000000000'
+Symbols:
+    - Name:            FUNC
+      Type:            STT_FUNC
+      Section:         .text
+      Value:           0x00000000004004F0
+      Size:            0x000000000000000A
+      Binding:         STB_GLOBAL
+    - Name:            main
+      Type:            STT_FUNC
+      Section:         .text
+      Value:           0x00000000004004FA
+      Size:            0x0000000000000004
+      Binding:         STB_GLOBAL
+    - Name:            XYZ
+      Type:            STT_FUNC
+      Section:         .text
+      Value:           0x00000000004004CD
+      Size:            0x0000000000000023
+      Binding:         STB_GLOBAL
+DynamicSymbols:
+    - Name:            mydata
+      Section:         .rodata
+      Value:           0x0000000000400100
+      Binding:         STB_GLOBAL
+ProgramHeaders:
+  - Type: PT_PHDR
+    Flags: [ PF_X, PF_R ]
+    VAddr: 0x004003E0
+    PAddr: 0x004003E0
+    FirstSec: .text
+    LastSec: .text
+  - Type: PT_LOAD
+    Flags: [ PF_X, PF_R ]
+    VAddr: 0x004003E0
+    PAddr: 0x004003E0
+    FirstSec: .text
+    LastSec: .text
+  - Type: PT_DYNAMIC
+    Flags: [ PF_X, PF_R ]
+    VAddr: 0x00600E28
+    PAddr: 0x00600E28
+    FirstSec: .dynamic
+    LastSec: .dynamic
 No newline at end of file

llvm/test/DebugInfo/X86/lsda_issue.test

+# This reproduces LSDA
+
+# RUN: yaml2obj %p/Inputs/eh_frame.yaml &> %t.exe
+# RUN: llvm-dwarfdump -debug-frame %t.exe | FileCheck %s
+
+CHECK: LSDA Address: 0000000000000000
 No newline at end of file