[PATCH] D63361: Pretend NRVO variables are references so they can be found by debug info

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

+ at aprantl @dblaikie

The basic idea of this change is that clang should store implicit sret pointer arguments into a static alloca, and the dbg.declare should reference the static alloca instead of referring to the LLVM argument directly. The idea is that that will be more resilient. The current consensus seems to be that that's the best way to ensure we have maximally correct debug info at -O0. Let us know if you have any feedback on how to do this.



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3940-3941
 
+  // Add DW_OP_deref to NRVO variables because we set their debug values to be
+  // references.
+  if (VD->isNRVOVariable())
----------------
I wouldn't mention references here since those are codeview specific. This should instead say something like: "Clang stores the sret pointer provided by the caller in a static alloca. Use DW_OP_deref to tell the debugger to load that pointer and treat it as the address of the variable.".


================
Comment at: clang/lib/CodeGen/CGDecl.cpp:1566-1576
+    // NRVO variables are stored in the return register and are not always
+    // readable by the debugger. Get around this by storing a reference to
+    // the variable.
+    if (NRVO) {
+      llvm::Type *RefTy =
+          ConvertTypeForMem(getContext().getLValueReferenceType(Ty));
+      DebugAddr = CreateTempAlloca(RefTy, getPointerAlign(), "nrvo_ref");
----------------
I think this alloca is something that we should set up in the prologue. Take a look at CodeGenFunction::StartFunction for where the ReturnValue member variable is initialized. You can create the alloca there, save it, and reuse it here for every NRVO variable, since there could be more than one, and we they can all use the same static alloca.

Initially I thought perhaps we could change the meaning of `ReturnValue` to simply *be* this alloca, but I don't think it will be so easy. I'd recommend adding a second member next to it that's null unless sret is in use.

Finally, I notice we need to do something if inalloca is involved. (Yuck.) You can see how its handled in StartFunction. You could use the result of the Builder.CreateStructGEP call here as the value to save to pass to dbg.declare when NRVO fires:
https://github.com/llvm/llvm-project/blob/14059d2a13667409ccf2860ff7d1c56f6c8c70a6/clang/lib/CodeGen/CodeGenFunction.cpp#L904


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1145-1149
+    bool Deref = false;
+    if (VI.Expr) {
+      if (!VI.Expr->extractIfOffset(ExprOffset, Deref))
         continue;
+    }
----------------
I see. I thought this code was using the stuff I added in `DbgVariableLocation::extractFromMachineInstruction`. Hm.

In practice, do you actually see DIExpression offsets here? I think maybe we should just special case a single DW_OP_deref expression, since that's what we get at O0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63361