[PATCH] D91058: [DebugInfo] Refactor code for emitting DWARF expressions for FP constants

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:234-236
+    addStackValue();
     if (Offset == 0 && Size <= 64)
       break;
----------------
dblaikie wrote:
> This looks like a change in behavior - is it? Should it be tested?
It shouldn't normally be a change because this adjustment in DW_OP_stack_value emission is matched by a change in the callers (previously it was emitted through the `DwarfExpr.addExpression(std::move(ExprCursor))` call, now that call is skipped).

It could theoretically make a difference if we had a floating point constant accompanied by a non-empty dwarf expression -- previously that expression would be emitted, but now it's skipped. However, such a combination wouldn't be very useful (the expression would operate on the constant as an integer), and judging by the lack of failing tests, we don't actually make use of it. (Though my knowledge of this part of the code is fairly limited).


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:247
+  // FIXME: Add support for `long double`.
+  if (NumBytes <= 8 /*double*/)
+    addUnsignedConstant(API, AP);
----------------
dblaikie wrote:
> This looks like a change in behavior (now other <= 8 floating point sizes can be emitted - is it just that other such sizes are impossible/never come up? In which case maybe an assertion would be suitable?) & might possibly benefit from testing.
Actually, it was already possible to emit the DW_OP_stack_value flavour of _Float16 (for example). This added the ability to handle the DW_OP_implicit_value form as well, because the new code layout made it harder to split the two cases. I didn't want to add a test without checking that this actually works (though I had no reason to suspect it wouldn't), but I have now verified that both gdb and lldb handle _Float16 (on arm), so I am adding a test for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91058