Page MenuHomePhabricator

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

Authored by akhuang on Jun 14 2019, 2:48 PM.



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

Diff Detail


Event Timeline

akhuang created this revision.Jun 14 2019, 2:48 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 14 2019, 2:48 PM
akhuang retitled this revision from Pretend NRVO variables are references so that CodeView can get their value to Pretend NRVO variables are references so they can be found by debug info.Jun 14 2019, 3:08 PM
akhuang edited the summary of this revision. (Show Details)
akhuang added a reviewer: rnk.
rnk added a comment.Jun 14 2019, 4:13 PM

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

3940–3941 ↗(On Diff #204854)

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

1566–1576 ↗(On Diff #204854)

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:

1145–1149 ↗(On Diff #204854)

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 updated this revision to Diff 204878.Jun 14 2019, 4:19 PM
  • fix test case
aprantl requested changes to this revision.Jun 14 2019, 4:24 PM
aprantl added inline comments.
1572 ↗(On Diff #204854)

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.

This revision now requires changes to proceed.Jun 14 2019, 4:24 PM
akhuang updated this revision to Diff 205382.Jun 18 2019, 9:54 AM
akhuang marked 2 inline comments as done.

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

Also addressed some other comments.

1145–1149 ↗(On Diff #204854)

That should work

akhuang updated this revision to Diff 205385.Jun 18 2019, 9:58 AM
  • add semicolon
rnk added a comment.Jun 18 2019, 10:16 AM

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.

3946 ↗(On Diff #205385)

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

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.

Thanks, this addresses my concern!

akhuang marked an inline comment as done.Jun 19 2019, 9:44 AM
akhuang added inline comments.
3946 ↗(On Diff #205385)

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 added inline comments.Jun 19 2019, 9:56 AM
3946 ↗(On Diff #205385)

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

akhuang updated this revision to Diff 205649.Jun 19 2019, 11:25 AM
  • 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 marked an inline comment as done.Jun 19 2019, 11:25 AM
akhuang added inline comments.
908 ↗(On Diff #205385)

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

rnk added a comment.Jun 19 2019, 11:54 AM

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.

1564 ↗(On Diff #205649)

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.

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 updated this revision to Diff 205712.Jun 19 2019, 4:51 PM
akhuang marked 2 inline comments as done.
  • 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 added inline comments.Jun 19 2019, 4:54 PM
908 ↗(On Diff #205385)

I suppose getPointerAlign() should work?

rnk accepted this revision.Jun 20 2019, 9:00 AM

lgtm, thanks!

908 ↗(On Diff #205385)

Looks right to me. :)

This revision was not accepted when it landed; it landed in state Needs Review.Jun 20 2019, 10:13 AM
This revision was automatically updated to reflect the committed changes.