[PATCH] D90916: [DebugInfo] Emit locations for large constant integers

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In D90916#2378622 <https://reviews.llvm.org/D90916#2378622>, @SouraVX wrote:

> Thanks for the patch! Overall this looks nice. We can also represent `long double`(128 bits) using `DW_OP_implicit_value`, any plans for supporting that too ? `long double` is non-trivial like it has different  representation on Power. It's pending on me since a while :(

Um... maybe? What's the deal with long double and PPC ? Does bitcastToAPInt return a different representation than what's actually used on the machine?



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2477-2480
+  if (Value.isConstantInt()) {
+    DwarfExpr.addUnsignedConstant(Value.getConstantInt()->getValue(), AP);
+    return;
+  }
----------------
dblaikie wrote:
> What's this change doing? It looks new & not sure what the old code would've been doing previously under this condition (isConstantInt) 
This is the part which actually implements this feature. :)
I don't understand why, but integers <= 64 bit are stored differently than integers with larger width. So, smaller integers would be caught by the `isInt` check on line 2451, but the code wouldn't do anything with larger integers (AFAICT).


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:202
 
-void DwarfExpression::addUnsignedConstant(const APInt &Value) {
+void DwarfExpression::addUnsignedConstant(APInt Value, const AsmPrinter &AP) {
   assert(isImplicitLocation() || isUnknownLocation());
----------------
dblaikie wrote:
> This change looks like it'd have two effects:
> 
> 1) existing callers of addUnsignedConstant now might get DW_OP_implicit_values where they didn't before
> 2) the new call from addConstantFP might lead to that use case getting DW_OP_const values it didn't get before
> 
> Could those two situations be separated into distinct patches with distinct tests? (I mean, could be the same test file, but making it clear how adding this new functionality to addUnsignedConstant impacts existing callers, then separately how changing addConstantFP to use addUnsignedConstant changes that)
I elaborate on (1) below. You're right about (2). I did check that gdb&lldb can actually handle the expanded usage of DW_OP_const, but I was too lazy to turn it into a test. I'll try to split that off into a separate patch.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:206
 
+  if (AP.getDwarfVersion() >= 4 && !AP.getDwarfDebug()->tuneForSCE()) {
+    int NumBytes = Value.getBitWidth() / 8;
----------------
SouraVX wrote:
> GDB also supports this. When I introduced this representation, I kept it exclusively GDB. Looking at DwarfDebug.cpp:2477 looks like someone changed it along the way.
> Do you mind adding `GDB` also ?
This already covers gdb (note the negation). The condition was changed from `GDB` to `not SCE` when lldb support for this was added.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:233
     addUnsignedConstant(*Data++);
+    addStackValue();
     if (Offset == 0 && Size <= 64)
----------------
dblaikie wrote:
> Is this a separate bugfix? Could go in a separate patch/test/etc?
I'm not sure I would call it a bugfix, as the function had only one caller (and a pretty strange one at that) and was only being called for size <= 64. However, it is required to reflect the new realities of how this function is used now.

Not emitting DW_OP_stack_value here was kind of correct, as its only caller (DwarfDebug.cpp:2484) was falling through (after ensuring that the size is <= 64) to `DwarfExpr.addExpression(std::move(ExprCursor));`, which would end up adding DW_OP_stack_value.

I'm not really sure what addExpression does (I am quite new to this area), but in the examples I've tried, it only appends DW_OP_stack_value. I suppose it might be possible to modify the value of the float constant via some dwarf expressions, but that would be fairly tricky as the dwarf operands have integral semantics.

In the new setup, I don't even bother calling addExpression, as hooking that up would be tricky (and of dubious value), as that function could be called on single-piece non-implicit_value expressions. That means this function always needs to emit DW_OP_stack_value itself.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:245
   int NumBytes = API.getBitWidth() / 8;
-  if (NumBytes == 4 /*float*/ || NumBytes == 8 /*double*/) {
+  if (NumBytes <= 8 /*double*/) {
     // FIXME: Add support for `long double`.
----------------
SouraVX wrote:
> NIT: Trival braces ?
> https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
In my mind, the FIXME made the body non-trivial. But I guess it is possible to move that outside the body...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90916