Page MenuHomePhabricator

Allow RegisterContext to track if behaves-like-frame-0, allow LanguageRuntime for above frame 0
ClosedPublic

Authored by jasonmolenda on Feb 28 2021, 2:48 PM.

Details

Summary

This patch allows a RegisterContext to record if it behaves like a 0th frame -- where its pc value should not be decremented before symbolication. It adds a call out to the LanguageRuntime to get a special UnwindPlan when unwinding from the 0th (first) frame, not just further unwinds up the stack. The majority of the patch is the mechanical addition of the new ivar/method to expose this, and the LanguageRuntime hook in InitializeZerothFrame(). This is a follow-on patch to the previous https://reviews.llvm.org/D96839

For the Swift async unwinds, the saved pc values we have are not normal ABI style calls, and backing up the resume pc results in bad symbolication. I've been meaning to add a capability like this to RegisterContext for a while for DWARFExpression::Evaluate where we get a frame's pc value to look up a location list entry too (and do not decr the pc resulting in bogus values being displayed for noreturn calls) - I'll fix that in a separate patch.

I have API tests when the Swift language runtime is present, but in generic lldb we don't have an environment quite like this. In the followup patch to DWARFExpression::Evaluate I'll be able to test RegisterContext::BehavesLikeZerothFrame so there will be some test coverage then.

rdar://problem/70398009

Diff Detail

Unit TestsFailed

TimeTest
90 msx64 windows > LLVM.Instrumentation/InstrProfiling::profiling.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\Instrumentation\InstrProfiling\profiling.ll -mtriple=x86_64 -passes=instrprof -S | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\Instrumentation\InstrProfiling\profiling.ll --check-prefixes=CHECK,ELF,ELF_GENERIC

Event Timeline

jasonmolenda created this revision.Feb 28 2021, 2:48 PM
jasonmolenda requested review of this revision.Feb 28 2021, 2:48 PM

Another alternative to RegisterContext::BehavesLikeZerothFrame that I've thought of is RegisterContext::GetPCForSymbolication, similar to GetPC() today. I think everyone who is decrementing $pc is doing it for symbolication, so having this hint and then leaving it to everyone to decrement-or-not may not be a great choice. It may be easier for higher-levels if RegisterContext can provide an Address suitable for symbolication directly.

Another alternative to RegisterContext::BehavesLikeZerothFrame that I've thought of is RegisterContext::GetPCForSymbolication, similar to GetPC() today. I think everyone who is decrementing $pc is doing it for symbolication, so having this hint and then leaving it to everyone to decrement-or-not may not be a great choice. It may be easier for higher-levels if RegisterContext can provide an Address suitable for symbolication directly.

If "behaves_like_zeroth_frame" is in the register context for the purpose of backing up the address, then I vote for RegisterContext::GetPCForSymbolication(). We can actually backup the PC by the min instruction size if we really want to get fancy. It would be really nice to have this centralized in one location so we don't have a bunch of code locations backing up the PC.

For the Swift async unwinds, the saved pc values we have are not normal ABI style calls, and backing up the resume pc results in bad symbolication. I've been meaning to add a capability like this to RegisterContext for a while for DWARFExpression::Evaluate where we get a frame's pc value to look up a location list entry too (and do not decr the pc resulting in bogus values being displayed for noreturn calls) - I'll fix that in a separate patch.

What does this mean for ReportCrash and other tools that like to just walk the frame pointer chain? Does that still produce valid stack trace, just with some Swift async frames missing?

Rewrite patch to expose "get pc for symbolication" APIs instead of "behaves like zeroth frame" APIs so that we don't have different parts of lldb decrementing the pc for symbolication. Doing a final round of tests on different targets etc but I believe this is done.

clayborg requested changes to this revision.Mar 3 2021, 3:01 PM
clayborg added inline comments.
lldb/source/Target/RegisterContext.cpp
162

Can we decrement the PC by the minimum instruction size? If we can get an ArchSpec we can call:

uint32_t ArchSpec::GetMinimumOpcodeByteSize() const;
lldb/source/Target/StackFrame.cpp
217–226

Should we be using the "bool RegisterContext::GetPCForSymbolication(Address &address)" function here to avoid having more than one place that is subtracting from the PC value?

This revision now requires changes to proceed.Mar 3 2021, 3:01 PM
jasonmolenda added inline comments.Mar 3 2021, 3:47 PM
lldb/source/Target/RegisterContext.cpp
162

I don't know if it's worth it. For fixed-width architectures, we'll be symbolicating an instruction boundary, but for others, we're just decrementing within the bounds of the instruction. We should not ever show this address anywhere, it's only used for setting context and we need to be within the bounds of the instruction. pc-1 seems fine to me. I don't feel very strongly about this.

lldb/source/Target/StackFrame.cpp
217–226

Yeah, I thought about that as I was doing it. I'll unify these.

clayborg added inline comments.Mar 3 2021, 4:00 PM
lldb/source/Target/RegisterContext.cpp
162

My only reason to vote for doing this is to avoid creating a lookup address that looks like a thumb address on 32 bit ARM if we subtract 1 from it and the code is available above. That being said, I don't feel strongly about this either.

jasonmolenda added inline comments.Mar 3 2021, 6:45 PM
lldb/source/Target/StackFrame.cpp
217–226

Ah, I see why they can't be unified. A StackFrame may be set up with a pc value different than the register context for the concrete frame here, e.g. for a tail-call artifical stack frame. It took me a little while to figure out why deferring to the StackFrame's RegisterContext was causing failures in those tests. I added a comment saying why this code needs to decr-pc too, so someone doesn't try to clean these up in the future.

This revision was not accepted when it landed; it landed in state Needs Revision.Mar 3 2021, 7:29 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.