[bolt] Fix typo in BinaryFunction::parseLSDA

[RIscRIpt]

Summary

According to EHStreamer::emitExceptionTable LLVM emits absptr
address in non-PIC case. Otherwise, LLVM emits pcrel offset.

[RIscRIpt]

I think it went unnoticed, because AFAICS lpStartEncoding is omit most of the times, which resulted MaybeLPStart == std::nullopt, and LPStart = 0.

LPStart = 0
LPOffset = LPStart + LandingPad;
LPAddress = Address + LPOffset; // == Address + 0 + LandingPad == Address + LandingPad
LPAddress < Address == false // OK, fragment is not skipped

However if lpStartEncoding was not omit, this should have resulted in skipping the fragment:

LPStart = *MaybeLPStart - Address
LPOffset = LPStart + LandingPad;
LPAddress = Address + LPOffset; // == Address + *MaybeLPStart - Address + LandingPad == *MaybeLPStart + LandingPad
LPAddress < Address == true // iff *MaybeLPStart is not absolute (i.e. *MaybeLPStart < Address)

This had to work correctly in case of non-PIC binaries (with absolute addresses in *MaybeLPStart), and it had to result in a bug for PIC binaries.

I came across this typo while studying relevant code; my thinking may be wrong.

[rafauler]

Apparently this code was introduced at https://reviews.llvm.org/D128561 but no test case was provided showing when LPStart would be present.

Honestly I do not understand why would we subtract Address when LPStart is encoded with absptr but not for other encodings. Of course, talking about the unknown case where LPStart is actually present and not omitted - I still need to find a case where this is actually used. I don't have any test case where LPStart encoding is not set to omit. @Amir any idea on which binary would have that? It would be better to have a testcase where LPStart is actually used, so we can better understand what we are fixing.

I'm inclined towards always decrementing this Address instead of having the if(absptr). Another thing that is wrong is that LPStart can potentially be negative after subtracting Address from it, but the variable itself is declared as unsigned. Confusing. I think it needs to be clear that:

• LPStart is ALWAYS a full pointer (when present). Therefore, if we want to extract an offset out of it, we ought to subtract Address for all cases.
• LandingPad is ALWAYS an offset.

[Amir]

In D150229#4333580, @rafauler wrote:

Apparently this code was introduced at https://reviews.llvm.org/D128561 but no test case was provided showing when LPStart would be present.

Honestly I do not understand why would we subtract Address when LPStart is encoded with absptr but not for other encodings. Of course, talking about the unknown case where LPStart is actually present and not omitted - I still need to find a case where this is actually used. I don't have any test case where LPStart encoding is not set to omit. @Amir any idea on which binary would have that? It would be better to have a testcase where LPStart is actually used, so we can better understand what we are fixing.

I'm inclined towards always decrementing this Address instead of having the if(absptr). Another thing that is wrong is that LPStart can potentially be negative after subtracting Address from it, but the variable itself is declared as unsigned. Confusing. I think it needs to be clear that:

• LPStart is ALWAYS a full pointer (when present). Therefore, if we want to extract an offset out of it, we ought to subtract Address for all cases.
• LandingPad is ALWAYS an offset.

Sorry about that omission in test. The binary (HHVM) was compiled with Clang with -fsplit-machine-functions. Let me see if I can repro it and come up with a reduced test case.

[RIscRIpt]

Bump. Tbh, I am not interested in diving into Bolt project, so you might do whatever you want with this patch.

[maksfb]

@RIscRIpt - thanks, I ran into the same issue. Fixed in https://github.com/llvm/llvm-project/commit/84602066a6be175513880283394268b0f29eb296

[RIscRIpt]

Thanks, closing. As per your changes, looks like my fix was not correct after all.

bolt/lib/Core/Exceptions.cpp

DWARFDataExtractor Data(
       StringRef(reinterpret_cast<const char *>(LSDASectionData.data()),
                 LSDASectionData.size()),
       BC.DwCtx->getDWARFObj().isLittleEndian(), 8);
   uint64_t Offset = getLSDAAddress() - LSDASectionAddress;
   assert(Data.isValidOffset(Offset) && "wrong LSDA address");
 
   uint8_t LPStartEncoding = Data.getU8(&Offset);
   uint64_t LPStart = 0;
-  // Convert to offset if LPStartEncoding is typed absptr DW_EH_PE_absptr
+  // Convert to offset if LPStartEncoding is absptr (non-PIC case)
   if (std::optional<uint64_t> MaybeLPStart = Data.getEncodedPointer(
           &Offset, LPStartEncoding, Offset + LSDASectionAddress))
-    LPStart = (LPStartEncoding && 0xFF == 0) ? *MaybeLPStart
-                                             : *MaybeLPStart - Address;
+    LPStart = LPStartEncoding == DW_EH_PE_absptr ? *MaybeLPStart - Address
+                                                 : *MaybeLPStart;
 
   const uint8_t TTypeEncoding = Data.getU8(&Offset);
   LSDATypeEncoding = TTypeEncoding;
   size_t TTypeEncodingSize = 0;
   uintptr_t TTypeEnd = 0;
   if (TTypeEncoding != DW_EH_PE_omit) {
     TTypeEnd = Data.getULEB128(&Offset);
     TTypeEncodingSize = BC.getDWARFEncodingSize(TTypeEncoding);