Pretend NRVO variables are references so they can be found by debug info

[akhuang]

Summary

Previously variables optimized by NRVO showed up as "optimized away" in CodeView and Dwarf debug info because their debug location was the return register.
This change stores a reference to the NRVO variable and uses that for the debug value.

Related to https://bugs.chromium.org/p/chromium/issues/detail?id=860398#c16

[rnk]

+@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.

clang/lib/CodeGen/CGDebugInfo.cpp

3940–3941

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.".

clang/lib/CodeGen/CGDecl.cpp

1566–1576

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

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1145–1150

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.

[akhuang]

• fix test case

[aprantl]

clang/lib/CodeGen/CGDecl.cpp

1572

We can't change IR generation based on whether debug info is enabled or not. The alloca should be emitted unconditionally.

It's a strong goal of LLVM that enabling/disabling debug info should have no effect on the contents of the .text section.

[akhuang]

Now creates a pointer to the return location in the function prolog, whenever sret is being used.

Also addressed some other comments.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

1145–1150

That should work

[akhuang]

• add semicolon

[rnk]

This needs two unit tests:

• A clang test at clang/test/CodeGenCXX/debug-info-nrvo.cpp similar to other debug-info-* tests there. This test should have a second RUN line and extra checks for -fno-elide-constructors.
• An LLVM test at llvm/test/DebugInfo/COFF/nrvo.ll to show that we handle dbg.declare + deref

Truly, we should also test the inalloca case.

clang/lib/CodeGen/CGDebugInfo.cpp

3943

I think we should check for getLangOpts().ElideConstructors here, and check that the debug info is correct (no deref) in that mode.

clang/lib/CodeGen/CodeGenFunction.cpp

908 ↗

(On Diff #205385)

What I had in mind was to use this GEP as the ReturnValuePointer here. The inalloca parameter is also a pointer to stack memory, and a GEP is an offset, so it should end up being handled like a static alloca.

[aprantl]

Thanks, this addresses my concern!

[akhuang]

clang/lib/CodeGen/CGDebugInfo.cpp

3943

Makes sense, though now I realized there is also an issue because CGDecl checks whether ReturnValuePointer is valid before changing the debug value, but CGDebugInfo doesn't check this when adding DW_OP_deref.. so maybe it makes sense to pass something through to EmitDeclare?

[rnk]

clang/lib/CodeGen/CGDebugInfo.cpp

3943

I guess adding a boolean to EmitDeclareOfAutoVariable would be the way to go, and then to thread that through here.

[akhuang]

• Add clang and llvm tests, and windows debuginfo test
• Use GEP as returnValuePointer in inalloca case
• Add bool parameter for EmitDeclare when pointer is being used

[akhuang]

clang/lib/CodeGen/CodeGenFunction.cpp

908 ↗

(On Diff #205385)

Oh, ok. I changed it, but not sure how to test debug info for the inalloca case

[rnk]

I see we don't have any tests for inalloca to model this on, so I think we should skip that for this change. I'll add one later that handles arguments as well, since those are interesting.

clang/lib/CodeGen/CGDecl.cpp

1564

Nit: Since the boolean is really the condition of the if, I'd suggest writing it like this:

bool UsePointerValue = NRVO && ReturnValuePointer.isValid();
if (UsePointerValue)
  DebugAddr = ReturnValuePointer;
...

It's more functional, less stateful, and is one less line in the end.

clang/lib/CodeGen/CodeGenFunction.cpp

908 ↗

(On Diff #205385)

I don't think this is the correct alignment. The sret slot in the inalloca has a pointer type, so I think you want pointer alignment here. I think you can substitute Int8PtrTy for RetTy here.

[akhuang]

• fix alignment of pointer in inalloca case
• make existing tests stop failing by changing some and adding a check for existing return value alloca (I think?) before adding the ReturnValuePointer alloca

[akhuang]

clang/lib/CodeGen/CodeGenFunction.cpp

908 ↗

(On Diff #205385)

I suppose getPointerAlign() should work?

[rnk]

lgtm, thanks!

clang/lib/CodeGen/CodeGenFunction.cpp

908 ↗

(On Diff #205385)

Looks right to me. :)

clang/lib/CodeGen/CGDebugInfo.cpp

if (RD->isUnion() && RD->isAnonymousStructOrUnion()) {
         DBuilder.insertDeclare(
             Storage, D, DBuilder.createExpression(Expr),
             llvm::DebugLoc::get(Line, Column, Scope, CurInlinedAt),
             Builder.GetInsertBlock());
       }
     }
   }
 
+  // Add DW_OP_deref to NRVO variables because we set their debug values to be
+  // references.
+  if (VD->isNRVOVariable())
+    Expr.push_back(llvm::dwarf::DW_OP_deref);
+
   // Create the descriptor for the variable.
   auto *D = ArgNo ? DBuilder.createParameterVariable(
                         Scope, Name, *ArgNo, Unit, Line, Ty,
                         CGM.getLangOpts().Optimize, Flags)
                   : DBuilder.createAutoVariable(Scope, Name, Unit, Line, Ty,
                                                 CGM.getLangOpts().Optimize,
                                                 Flags, Align);
 

clang/lib/CodeGen/CGDecl.cpp

bool EmitDebugInfo = DI && CGM.getCodeGenOpts().getDebugInfo() >=
                                  codegenoptions::LimitedDebugInfo;
 
   Address address = Address::invalid();
   Address AllocaAddr = Address::invalid();
   Address OpenMPLocalAddr =
       getLangOpts().OpenMP
           ? CGM.getOpenMPRuntime().getAddressOfLocalVariable(*this, &D)
           : Address::invalid();
+  bool NRVO = getLangOpts().ElideConstructors && D.isNRVOVariable();
+
   if (getLangOpts().OpenMP && OpenMPLocalAddr.isValid()) {
     address = OpenMPLocalAddr;
   } else if (Ty->isConstantSizeType()) {
-    bool NRVO = getLangOpts().ElideConstructors &&
-      D.isNRVOVariable();
-
     // If this value is an array or struct with a statically determinable
     // constant initializer, there are optimizations we can do.
     //
     // TODO: We should constant-evaluate the initializer of any variable,
     // as long as it is initialized by a constant expression. Currently,
     // isConstantInitializer produces wrong answers for structs with
     // reference or bitfield members, and a few other cases, and checking
     // for POD-ness protects us from some of these.
CodeGenFunction::EmitAutoVarAlloca(const VarDecl &D) {
   }
 
   setAddrOfLocalVar(&D, address);
   emission.Addr = address;
   emission.AllocaAddr = AllocaAddr;
 
   // Emit debug info for local var declaration.
   if (EmitDebugInfo && HaveInsertPoint()) {
+    Address DebugAddr = address;
     DI->setLocation(D.getLocation());
-    (void)DI->EmitDeclareOfAutoVariable(&D, address.getPointer(), Builder);
+
+    // 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");
+      Builder.CreateStore(ReturnValue.getPointer(), DebugAddr);
+    }
+
+    (void)DI->EmitDeclareOfAutoVariable(&D, DebugAddr.getPointer(), Builder);
   }
 
   if (D.hasAttr<AnnotateAttr>() && HaveInsertPoint())
     EmitVarAnnotations(&D, address.getPointer());
 
   // Make sure we call @llvm.lifetime.end.
   if (emission.useLifetimeMarkers())
     EHStack.pushCleanup<CallLifetimeEnd>(NormalEHLifetimeMarker,

debuginfo-tests/nrvo-string.cpp

struct string {
   string() {}
   string(int i) : i(i) {}
   ~string() {}
   int i = 0;
 };
 string get_string() {
   string unused;
   string result = 3;
   // DEBUGGER: break 21
   return result;
 }
-int main() { get_string(); }
+void some_function(int) {}
+struct string2 {
+  string2() = default;
+  string2(string2 && other) { i = other.i; }
+  int i;
+}
+string2 get_string2() {
+  string2 result;
+  result.i = 5;
+  some_function(result.i);
+  // Test that the debugger can get the value of result after another
+  // function is called.
+  // DEBUGGER: break 35
+  return result;
+}
+int main() {
+  get_string();
+  get_string2();
+}
 
 // DEBUGGER: r
 // DEBUGGER: print result.i
 // CHECK:  = 3
+// DEBUGGER: c
+// DEBUGGER: print result.i
+// CHECK:  = 5

llvm/include/llvm/IR/DebugInfoMetadata.h

public:
 
   /// Return whether this is an implicit location description.
   bool isImplicit() const;
 
   /// Append \p Ops with operations to apply the \p Offset.
   static void appendOffset(SmallVectorImpl<uint64_t> &Ops, int64_t Offset);
 
   /// If this is a constant offset, extract it. If there is no expression,
-  /// return true with an offset of zero.
-  bool extractIfOffset(int64_t &Offset) const;
+  /// or if there is one expression of DW_OP_deref, return true with an offset
+  /// of zero. Also set Deref.
+  bool extractIfOffset(int64_t &Offset, bool &Deref) const;
 
   /// Checks if the last 4 elements of the expression are DW_OP_constu <DWARF
   /// Address Space> DW_OP_swap DW_OP_xderef and extracts the <DWARF Address
   /// Space>.
   static const DIExpression *extractAddressClass(const DIExpression *Expr,
                                                  unsigned &AddrClass);
 
   /// Used for DIExpression::prepend.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp

for (const MachineFunction::VariableDbgInfo &VI : MF.getVariableDbgInfo()) {
 
     // If variable scope is not found then skip this variable.
     if (!Scope)
       continue;
 
     // If the variable has an attached offset expression, extract it.
     // FIXME: Try to handle DW_OP_deref as well.
     int64_t ExprOffset = 0;
-    if (VI.Expr)
-      if (!VI.Expr->extractIfOffset(ExprOffset))
+    bool Deref = false;
+    if (VI.Expr) {
+      if (!VI.Expr->extractIfOffset(ExprOffset, Deref))
         continue;
+    }
 
     // Get the frame register used and the offset.
     unsigned FrameReg = 0;
     int FrameOffset = TFI->getFrameIndexReference(*Asm->MF, VI.Slot, FrameReg);
     uint16_t CVReg = TRI->getCodeViewRegNum(FrameReg);
 
     // Calculate the label ranges.
     LocalVarDefRange DefRange =
         createDefRangeMem(CVReg, FrameOffset + ExprOffset);
+
     for (const InsnRange &Range : Scope->getRanges()) {
       const MCSymbol *Begin = getLabelBeforeInsn(Range.first);
       const MCSymbol *End = getLabelAfterInsn(Range.second);
       End = End ? End : Asm->getFunctionEnd();
       DefRange.Ranges.emplace_back(Begin, End);
     }
 
     LocalVariable Var;
     Var.DIVar = VI.Var;
     Var.DefRanges.emplace_back(std::move(DefRange));
+    if (Deref)
+      Var.UseReferenceType = true;
+
     recordLocalVariable(std::move(Var), Scope);
   }
 }
 
 static bool canUseReferenceType(const DbgVariableLocation &Loc) {
   return !Loc.LoadChain.empty() && Loc.LoadChain.back() == 0;
 }
 

llvm/lib/IR/DebugInfoMetadata.cpp

if (Offset > 0) {
     Ops.push_back(Offset);
   } else if (Offset < 0) {
     Ops.push_back(dwarf::DW_OP_constu);
     Ops.push_back(-Offset);
     Ops.push_back(dwarf::DW_OP_minus);
   }
 }
 
-bool DIExpression::extractIfOffset(int64_t &Offset) const {
+bool DIExpression::extractIfOffset(int64_t &Offset, bool &Deref) const {
+  Deref = false;
+
   if (getNumElements() == 0) {
     Offset = 0;
     return true;
   }
 
+  if (getNumElements() == 1 && Elements[0] == dwarf::DW_OP_deref) {
+    Offset = 0;
+    Deref = true;
+    return true;
+  }
+
   if (getNumElements() == 2 && Elements[0] == dwarf::DW_OP_plus_uconst) {
     Offset = Elements[1];
     return true;
   }
 
   if (getNumElements() == 3 && Elements[0] == dwarf::DW_OP_constu) {
     if (Elements[2] == dwarf::DW_OP_plus) {
       Offset = Elements[1];