[PATCH] D56587: Introduce DW_OP_LLVM_convert

[Markus Lavin via Phabricator via llvm-commits]

markus marked 6 inline comments as done.
markus added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1938
+    bool EmitByte = true;
+    // XXX: This is admittedly pretty stupid but sadly appears to be the
+    // easiest way to pass custom values as the expressions are inserted into a
----------------
aprantl wrote:
> You might want to reword this comment to be more assertive :-)
> 
> Is my understanding correct that we only need to do this here because the inline DW_AT_location (DW_FORM_block, ..) are emitted ahead of time and thus have the correct offsets injected from the get go?
> 
> Could you please move this out into a `fixupLocEntryDIEReferences()` (or something) function?
> Is my understanding correct that we only need to do this here because the inline DW_AT_location (DW_FORM_block, ..) are emitted ahead of time and thus have the correct offsets injected from the get go?

Sort of. When these are inserted in `DwarfExpression::addExpression` (for both location-lists and inlined) the offset of the base_type DIE is not known so we need to insert a placeholder. For the the location-list case the data structure is unfortunately a plain byte stream so we need this elaborate state machine to extract the placeholder here.

> Could you please move this out into a fixupLocEntryDIEReferences() (or something) function?
Since this is a state machine and hence keeps state I think that putting it in a separate function would only make it messier.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1978
+      case dwarf::DW_OP_bregx:
+        NumSkipLEB128s = 2;
+        State = SkipLEB128;
----------------
aprantl wrote:
> Didn't your dwarfdump patch have this info in an enum or am I confusing things?
` DWARFExpression` does contain similar information but it is private there. Not sure if refactoring would be worthwhile.


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

https://reviews.llvm.org/D56587