Page MenuHomePhabricator

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

Authored by dongAxis1944 on Apr 27 2021, 4:18 AM.

Details

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

Diff Detail

Event Timeline

dongAxis1944 created this revision.Apr 27 2021, 4:18 AM
dongAxis1944 requested review of this revision.Apr 27 2021, 4:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 4:18 AM

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.

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.

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.

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.

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.

Address the comments and add the unit test

dongAxis1944 marked 2 inline comments as done.May 7 2021, 7:39 PM