Page MenuHomePhabricator

[CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)
AcceptedPublic

Authored by vsk on Thu, Nov 7, 2:23 PM.

Details

Summary

Currently, clang emits subprograms for declared functions when the
target debugger or DWARF standard is known to support entry values
(DW_OP_entry_value & the GNU equivalent).

Treat DW_AT_tail_call the same way to allow debuggers to follow cross-TU
tail calls.

Pre-patch debug session with a cross-TU tail call:

* frame #0: 0x0000000100000fa4 main`target at b.c:4:3 [opt]
  frame #1: 0x0000000100000f99 main`main at a.c:8:10 [opt]

Post-patch (note that the tail-calling frame, "helper", is visible):

* frame #0: 0x0000000100000fa4 main`target at b.c:4:3 [opt]
  frame #1: 0x0000000100000f80 main`helper [opt] [artificial]
  frame #2: 0x0000000100000f99 main`main at a.c:8:10 [opt]

This was reverted in 5b9a072c because it attached declaration
subprograms to inlinable builtin calls, which interacted badly with the
MergeICmps pass. The fix is to not attach declarations to builtins.

rdar://46577651

Previous review: https://reviews.llvm.org/D69743

Diff Detail

Event Timeline

vsk created this revision.Thu, Nov 7, 2:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 7, 2:23 PM
aprantl accepted this revision.Thu, Nov 7, 2:38 PM
This revision is now accepted and ready to land.Thu, Nov 7, 2:38 PM

The failure I am investigating from the original commit of this at Google probably isn't related to the assertion failure that caused the revert of this patch/being addressed by this recommit. So if you could hold off a bit while I try to help provide a reproduction or enough detail for you to investigate this other internal failure?

At the moment the details I have is that the resulting assembly has an unused label/basic block boundary that's resulting in a location 0 (an un-located instruction placed at the beginning of a basic block uses location zero so it doesn't flow from the previous BB). Hmm, maybe that's unrelated, though. I haven't quite nailed it down yet.

vsk added a comment.Thu, Nov 7, 2:52 PM

The failure I am investigating from the original commit of this at Google probably isn't related to the assertion failure that caused the revert of this patch/being addressed by this recommit. So if you could hold off a bit while I try to help provide a reproduction or enough detail for you to investigate this other internal failure?

At the moment the details I have is that the resulting assembly has an unused label/basic block boundary that's resulting in a location 0 (an un-located instruction placed at the beginning of a basic block uses location zero so it doesn't flow from the previous BB). Hmm, maybe that's unrelated, though. I haven't quite nailed it down yet.

Thanks for the heads-up. I'm happy to hold off for now. I've also kicked off a stage2 -O3 -gline-tables-only build to be on the safe side.

vsk added a comment.Tue, Nov 12, 10:06 AM

@dblaikie do you need more time to investigate? fwiw I didn't uncover any new failures in a stage2 build with this patch applied.

In D69970#1742575, @vsk wrote:

@dblaikie do you need more time to investigate? fwiw I didn't uncover any new failures in a stage2 build with this patch applied.

I haven't figured this out yet, sorry - bit inundated with a few things, but I'm coming back to it. Apologies for the delay.

I can confirm we still have the same internal failure with this revised/updated patch.

Trying to reduce/reproduce it.

So I /think/ what's happening here is the extra addresses needed for the call_site low_pcs are creating new basic blocks (or whatever the logic is in the backend that says "use line zero at the start of each basic block-like thing" - possible that logic is overly conservative even though we just labeled something without ever needing to jump to it - guess we could test that with plain scopes).

Now, the test we have internally might be brittle & depending on an accidental source location - and/or this has shown an upstream bug where we drop a location that we should be preserving somewhere along the way.

OK - I believe the issue I'm seeing is an internal issue, a fragile/buggy test case. Please go ahead with this change if (by the sounds of it) all other concerns are addressed.

For more rambling context: A test case would call absl::GetStackTrace to get essentially the return address of the GetStackTrace call. It was doing this in an inlined function and expecting that the symbolized return address would reflect that. But the instruction immediately following the call to GetStackTrace (ie: the return address on the stack, the address that was being symbolized) was a spill due to something in the outer function.

This change caused that address (of the spill) to be labeled, to reference as the low_pc of a call_site.

Labeling the instruction caused a change in the line table due to use-unknown-locations (see r289468/ac7fe5e0c47a093b0fd8fd4972734ce143475c0e/D24180), giving the instruction a line zero location (since it was now at the start of a labeled region) making the stack trace not contain what was desired.

The test should probably be symbolizing 1 before the return address to get a more stable/intended location, and I'm working on an internal fix to that end.

/maybe/ the labels introduced by the call_site attribute (or any other label that is never used as a jump target) shouldn't trigger the line zero behavior from use-unknown-locations (@probinson in case he's got thoughts on that, though Sony might be using the "Always" option here, in which case this wouldn't've changed behavior for them)?

And/or maybe there are open questions about what location a spill should have? Whether we should back/forward propagate locations for those, since they can happen all over the place & maybe describing them as line zero isn't helpful? Not sure there.

(@echristo - just in case he's curious about what's going on here, though I'll write up more about the specific test on the internal bug)

vsk added a comment.Wed, Nov 13, 5:30 PM

@dblaikie thanks for chasing that down & the detailed explanation.

It's unfortunate that introducing call site info affects the line table. The impact of the change seems somewhat neutral (the additional line 0 locations aren't wrong, exactly), so I'm not sure it's worth keeping track of all the labels just referenced by call site tags and teaching DwarfDebug to tiptoe around those. As you point out, perhaps the divergence could be reduced by some general (hypothetical) improvement w.r.t line 0 locations for spill code. That might be the cheaper option (IIRC there's a predicate available on MI.getPseudoValue() to check for a spilled value).

I'm going to hold off on landing this, though. We've just gotten an internal report about some KAsan code that trips:

inlinable function call in a function with debug info must have a !dbg location
  %134 = call i8* @__asan_memcpy(i8* %agg.tmp2.sroa.0.0..sroa_cast10.i, i8* %agg.tmp.sroa.0.i.0..sroa_cast, i64 32)

Apparently there's a #define memcpy __asan_memcpy in some system-internal KAsan header, and some pass (likely ASan) introduces __asan_memcpy calls without attaching debug info. So we'll have to fix that first.

vsk updated this revision to Diff 229215.Wed, Nov 13, 6:27 PM

Don't emit declaration subprograms for functions with reserved names. There may not be much benefit from referencing these functions from call site tags (e.g. instead of relying on call site info to work out the arguments passed to __asan_memcpy, the asan runtime could log those arguments instead).

This fixes a verifier failure:

inlinable function call in a function with debug info must have a !dbg location
  %134 = call i8* @__asan_memcpy(i8* %agg.tmp2.sroa.0.0..sroa_cast10.i, i8* %agg.tmp.sroa.0.i.0..sroa_cast, i64 32)

@aprantl / @djtodoro, mind taking another look?

I think that it sounds reasonable to avoid declaration subprograms for functions with reserved names, so that part looks good to me.

In D69970#1745038, @vsk wrote:

Don't emit declaration subprograms for functions with reserved names. There may not be much benefit from referencing these functions from call site tags (e.g. instead of relying on call site info to work out the arguments passed to __asan_memcpy, the asan runtime could log those arguments instead).

This fixes a verifier failure:

inlinable function call in a function with debug info must have a !dbg location
  %134 = call i8* @__asan_memcpy(i8* %agg.tmp2.sroa.0.0..sroa_cast10.i, i8* %agg.tmp.sroa.0.i.0..sroa_cast, i64 32)

@aprantl / @djtodoro, mind taking another look?

(my very vague take on this, admittedly having not followed this work in detail, is that this sounds pretty heuristical & not like it's a principled/general solution? But I don't really know much about all this & will leave actual approvals/disapprovals to the other folks who've been reviewing this stuff (& admittedly, as the person who introduced the "inlinable call with no debug location in debug-having function" assertion/failure, it is a bit contextual as to whether a certain call is inlinable, etc))

vsk added a comment.Thu, Nov 14, 10:50 AM
In D69970#1745038, @vsk wrote:

Don't emit declaration subprograms for functions with reserved names. There may not be much benefit from referencing these functions from call site tags (e.g. instead of relying on call site info to work out the arguments passed to __asan_memcpy, the asan runtime could log those arguments instead).

This fixes a verifier failure:

inlinable function call in a function with debug info must have a !dbg location
  %134 = call i8* @__asan_memcpy(i8* %agg.tmp2.sroa.0.0..sroa_cast10.i, i8* %agg.tmp.sroa.0.i.0..sroa_cast, i64 32)

@aprantl / @djtodoro, mind taking another look?

(my very vague take on this, admittedly having not followed this work in detail, is that this sounds pretty heuristical & not like it's a principled/general solution? But I don't really know much about all this & will leave actual approvals/disapprovals to the other folks who've been reviewing this stuff (& admittedly, as the person who introduced the "inlinable call with no debug location in debug-having function" assertion/failure, it is a bit contextual as to whether a certain call is inlinable, etc))

*nod*, yeah I agree, but haven't come up with a better alternative. One option I considered was to play whac-a-mole and just teach ExpandICmps/ASan/others to emit builtin calls with line 0 locations attached. But I'm not sure that the extra line 0 locations would really help (maybe they'd actually get in the way of some reasonable location fallthrough), so the cleaner option seemed to be to not emit the declaration subprograms in the first place.