[DebugInfo] Refine the condition to detect CIEs.

[ikudrin]

Summary

The condition was not accurate enough and could interpret some FDEs in .eh_frame or 64-bit DWARF .debug_frame sections as CIEs. Even though such FDEs are unlikely in a normal situation, the wrong interpretation could hide an issue in a buggy generator.

[aprantl]

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

386

Is there any way to break this up into subexpressions and helper variables that would make this condition more easy to follow?

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

386

What about this?

bool IsCIE = (!IsEH && !IsDWARF64 && Id == DW_CIE_ID) ||
             (!IsEH && IsDWARF64 && Id == DW64_CIE_ID) ||
             (IsEH && !Id);

Or we may define a helper method, CIE::getCIEId(), like in D73887.

[ikudrin]

• Simplified the condition by using a function that returns an appropriate CIE id.

[ikudrin]

Ping.

[probinson]

The two tests look like they have an FDE with no preceding CIE? Isn't that invalid?

Well, in one test that's on purpose, but the other one looks like it's being handled as if the input were actually valid.

[ikudrin]

In D73886#1891998, @probinson wrote:

The two tests look like they have an FDE with no preceding CIE? Isn't that invalid?

The inputs are for sure invalid. The idea was to check that our consumer does not misinterpret them as valid CIEs as it did before the patch.

Well, in one test that's on purpose, but the other one looks like it's being handled as if the input were actually valid.

We allow FDEs without CIEs in .debug_frame, for good or bad. However, that is out of the scope of this patch.

[probinson]

Hmmm. I guess the next question is, do we have to bail out when something doesn't look right? Or can we keep going?
If we see an FDE with no preceding CIE, yes that's something we should report, but the not --crash tells me we are abandoning attempts to continue dumping the section. Does one of your other patches address this?

[ikudrin]

In D73886#1893606, @probinson wrote:

Hmmm. I guess the next question is, do we have to bail out when something doesn't look right? Or can we keep going?
If we see an FDE with no preceding CIE, yes that's something we should report, but the not --crash tells me we are abandoning attempts to continue dumping the section. Does one of your other patches address this?

DWARFDebugFrame::parse() tends to call report_fatal_error() in case of any trouble; that was initially added in https://reviews.llvm.org/rG705085da37eed9557202ff1e86f9462842ef7af9 and than extended in D15535 (https://reviews.llvm.org/rG03a670c0ec1d38de8c206de1426340041f10f8ff).

While I agree that the parser should not crash the program on invalid input, resolving that lays a bit far away from my current intentions, which are just to add support for 64-bit values here.

[ikudrin]

• Fixed section flags in the test.

[ikudrin]

Ping.

@probinson, do you suggest to change the intended behavior to crash on incorrect input before fixing this issue?

[probinson]

In D73886#1904675, @ikudrin wrote:

Ping.

@probinson, do you suggest to change the intended behavior to crash on incorrect input before fixing this issue?

There is a longer-term effort to make parsing more tolerant, but I see that this is existing behavior in handling .debug_frame and you should not be required to fix it before committing this patch. However, I would appreciate it if you would file a bug about this, mentioning the eh-frame-cie-id.s test, so that the people trying to clean up the parsing behavior can be aware of it.

File that bug and LGTM.

[ikudrin]

Thanks!

Here is the bug report.

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

while (Data.isValidOffset(Offset)) {
     // Length is the structure size excluding itself. Compute an offset one
     // past the end of the structure (needed to know how many instructions to
     // read).
     uint64_t StartStructureOffset = Offset;
     uint64_t EndStructureOffset = Offset + Length;
 
     // The Id field's size depends on the DWARF format
     Id = Data.getUnsigned(&Offset, (IsDWARF64 && !IsEH) ? 8 : 4);
-    bool IsCIE =
-        ((IsDWARF64 && Id == DW64_CIE_ID) || Id == DW_CIE_ID || (IsEH && !Id));
+    bool IsCIE = (!IsEH && ((!IsDWARF64 && Id == DW_CIE_ID) ||
+                            (IsDWARF64 && Id == DW64_CIE_ID))) ||
+                 (IsEH && !Id);
 
     if (IsCIE) {
       uint8_t Version = Data.getU8(&Offset);
       const char *Augmentation = Data.getCStr(&Offset);
       StringRef AugmentationString(Augmentation ? Augmentation : "");
       uint8_t AddressSize = Version < 4 ? Data.getAddressSize() :
                                           Data.getU8(&Offset);
       Data.setAddressSize(AddressSize);

llvm/test/DebugInfo/X86/debug-frame-cie-id-dwarf64.s

+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   llvm-dwarfdump -debug-frame - | \
+# RUN:   FileCheck %s
+
+# CHECK: 00000000 {{.*}} FDE
+
+        .section .debug_frame,"",@progbits
+## This FDE was formerly wrongly interpreted as a CIE because its CIE pointer
+## is similar to DWARF32 CIE id.
+        .long 0xffffffff        # DWARF64 mark
+        .quad .Lend - .LCIEptr  # Length
+.LCIEptr:
+        .quad 0xffffffff        # CIE pointer
+        .quad 0x1111abcd        # Initial location
+        .quad 0x00010000        # Address range
+.Lend:

llvm/test/DebugInfo/X86/eh-frame-cie-id.s

+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:   not llvm-dwarfdump -debug-frame - 2>&1 | \
+# RUN:   FileCheck %s
+
+# CHECK: Parsing FDE data at 0 failed due to missing CIE
+
+        .section .eh_frame,"",@progbits
+## This FDE was formerly wrongly interpreted as a CIE because its CIE pointer
+## is similar to CIE id of a .debug_frame FDE.
+        .long .Lend - .LCIEptr  # Length
+.LCIEptr:
+        .long 0xffffffff        # CIE pointer
+        .quad 0x1111abcd        # Initial location
+        .quad 0x00010000        # Address range
+.Lend: