dblaikie added subscribers: probinson, echristo.
dblaikie added a comment.

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 
<https://reviews.llvm.org/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)


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

https://reviews.llvm.org/D69970



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to