[PATCH] D132484: [BOLT] Preserve original LSDA type encoding

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Interesting.. Thanks for presenting this testcase and providing a fix!

@maksfb Do you recall why were we hardcoding the encoding of types?

I'm taking a guess that we wanted to restrict the encoding to the ones we know how to emit in BinaryEmitter, but we do support emitting both pic/non-pic - meaning, indirect + pcrel, as well as absolute.  Taking a look at BinaryEmitter.cpp:1024, which is responsible for actually encoding the type tables, what if we don't support outputting the format seen in the input? Will we crash with "unsupported TTypeEncoding"? Or are we covering every possible input type encoding?



================
Comment at: bolt/include/bolt/Core/BinaryFunction.h:194
   bool IsInjected = false;
-
+  unsigned LSDATypeEncoding = dwarf::DW_EH_PE_omit;
   using LSDATypeTableTy = SmallVector<uint64_t, 0>;
----------------
I think it makes more sense to move this line closer to "LSDAAddress" declaration in this same class, and define a getter member functions, since this will be used outside of this class (in binary emitter)


================
Comment at: bolt/lib/Core/BinaryEmitter.cpp:980-981
 
   // Account for any extra padding that will be added to the call site table
   // length.
+  if (TTypeEncoding != dwarf::DW_EH_PE_omit)
----------------
This comment should be moved inside the if


================
Comment at: bolt/lib/Core/Exceptions.cpp:123
 
-  const uint8_t TTypeEncoding = Data.getU8(&Offset);
+  LSDATypeEncoding = Data.getU8(&Offset);
   size_t TTypeEncodingSize = 0;
----------------
The changes to this function will generate this->LSDATypeEncoding access all over the parser (accessing a heap object).

I guess we can keep the original code, which is a const variable, and just add a new line here setting LSDATypeEncoding to TTypeEncoding.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132484/new/

https://reviews.llvm.org/D132484