[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/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

if (Id == getCIEId(IsDWARF64, IsEH)) {
         if (!AugmentationString.empty()) {
           // Parse the augmentation length and data for this FDE.
           uint64_t AugmentationLength = Data.getULEB128(&Offset);
 
           uint64_t EndAugmentationOffset = Offset + AugmentationLength;
 
           // 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);
+
+            auto IsPCRelativeFn = [&]() {
+              return (0x70 & Cie->getLSDAPointerEncoding()) ==
+                     dwarf::DW_EH_PE_pcrel;
+            };
+            if (LSDAAddress && *LSDAAddress != 0 && IsPCRelativeFn())
+              LSDAAddress = *LSDAAddress + CurPC;
           }
 
           if (Offset != EndAugmentationOffset)
             return createStringError(errc::invalid_argument,
                                      "parsing augmentation data at 0x%" PRIx64
                                      " failed",
                                      StartOffset);
         }