[PATCH] D56587: Introduce DW_OP_LLVM_convert

[Markus Lavin via Phabricator via llvm-commits]

markus marked 3 inline comments 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);
----------------
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.)


================
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:403
+        // later.
+        unsigned I = 0, E = CU.ExprRefedBaseTypes.size();
+        for (; I != E; ++I)
----------------
aprantl wrote:
> Why not use a SmallDenseSet for CU.ExprRefedBaseTypes?
I don't think that would work as we rely on being able to index into it.


================
Comment at: test/DebugInfo/Generic/convert-inlined.ll:32
+; DW5-CHECK-NEXT:                 DW_AT_decl_line	(1)
+; DW5-CHECK-NEXT:                 DW_AT_location	(DW_OP_addrx 0x0, DW_OP_deref, DW_OP_convert 0x1e, DW_OP_convert 0x22, DW_OP_stack_value)
+; DW5-CHECK-EMPTY:
----------------
aprantl wrote:
> Sorry about the extra work this causes, but it would really pay off to also have a separate peraratory commit that adds dwarfdump support for DW_OP_convert so this is printed as `DW_OP_convert (0x000022 "DW_ATE_signed_32")` as well as dwarfdump --verify support that ensures that the offset actually points to a type DIE.
I could look into that but before I spend too much time on dwarfdump I would like to get confirmation that would be the last remaining issue.


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

https://reviews.llvm.org/D56587