[PATCH] D56587: Introduce DW_OP_LLVM_convert

[Markus Lavin via Phabricator via llvm-commits]

markus marked an inline comment as done.
markus added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DIE.cpp:516
+  uint64_t Offset = CU->ExprRefedBaseTypes[Index].Die->getOffset();
+  assert(Offset < (1ULL << (ULEB128PadSize * 7)) && "Offset wont fit");
+  AP->EmitULEB128(Offset, nullptr, ULEB128PadSize);
----------------
markus wrote:
> aprantl wrote:
> > what happens when this assertion isn't met in a release compiler? Is that a purely hypothetical scenario?
> > what happens when this assertion isn't met in a release compiler? 
> The debug info would be corrupted.
> 
> > Is that a purely hypothetical scenario?
> With `ULEB128PadSize = 4` we are fine as long as the types are placed within 256MB from the start of the `.debug_info` section. Since we take care to insert the types immediately after the CU this will not be an issue for the case of a single CU. However by using llvm-link it is possible to put several CUs in the same module and that could push us closer to the 256MB limit.
> 
> If we set `ULEB128PadSize = 5` then the limit becomes 32GB and that should put us on the safe side for a considerable future (keeping in mind that this limit is for object files and not the final linked executable).
> 
> (One could argue that symbols and label differences, effectively pushing the problem to the assembler, are the right way to go here and I would tend to agree with that but unfortunately that causes havoc in the existing DIE framework as it relies on being able to pre-compute sizes.)
> However by using llvm-link it is possible to put several CUs in the same module and that could push us closer to the 256MB limit.
Actually forget that part. I see now that is irrelevant since the offset is within CU and not from start of .debug_info. I got confused looking at the output of readelf as that tool prints .debug_info address even though it is encoded as a CU offset.

So given this I think the 256MB limit is perfectly reasonable. 


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

https://reviews.llvm.org/D56587