Page MenuHomePhabricator

Reinitialize UnwindTable when the SymbolFile changes
ClosedPublic

Authored by labath on Feb 18 2019, 5:25 AM.

Details

Summary

This is a preparatory step to enable adding of unwind plans by symbol
file plugins.

Although at the surface it seems that currently symbol files have
nothing to do with unwinding, this isn't entirely correct even now. The
mere act of adding a symbol file can have the effect of making more
sections (typically .debug_frame) available to the unwinding machinery,
so that it can have more unwind strategies to choose from.

Up until now, we've had a bug, which went largely unnoticed, where
unwind info in the manually added symbols files (target symbols add) was
being ignored during unwinding. Reinitializing the UnwindTable fixes
that bug too.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Feb 18 2019, 5:25 AM
clayborg added inline comments.Feb 19 2019, 9:15 AM
source/Core/Module.cpp
1451–1454 ↗(On Diff #187231)

Are we sure no one is holding onto info that was given out from an existing UnwindTable? What if we do a backtrace, then load symbol files and then do another backtrace? Seems like we will want to keep our existing info and just call m_unwind_table->Clear()?

jasonmolenda marked an inline comment as done.Feb 19 2019, 12:39 PM
jasonmolenda added inline comments.
source/Core/Module.cpp
1451–1454 ↗(On Diff #187231)

I THINK it will be OK. RegisterContextLLDB holds shared pointers to the UnwindPlans that it got from the UnwindTable, so those will stay alive. An UnwindPlan doesn't refer back to the UnwindTable it came from.

labath marked 2 inline comments as done.Feb 20 2019, 1:44 AM
labath added inline comments.
source/Core/Module.cpp
1451–1454 ↗(On Diff #187231)

I agree with Jason. Nobody should store any pointers to the unwind table, and the FuncUnwinders and UnwindPlan objects are already handed out as shared pointers (but those shouldn't persist for long either, or otherwise our new unwind info wouldn't be effective). If we find a good reason to store the old unwind table, we can always do the thing like we do for SymbolFiles (where the Module keeps a list of all SymbolFile objects it ever had), but I hope that will not be necessary.

Greg, what do you make of this patch? Do you still want me to clear the unwind table instead of resetting it?

Greg, what do you make of this patch? Do you still want me to clear the unwind table instead of resetting it?

No, Jason seems ok with it so I am good.

Since unwinding is Jason's thing, he should give the ok for this patch.

jasonmolenda accepted this revision.Mar 11 2019, 4:55 PM

Yeah, I'm ok with this, thanks.

This revision is now accepted and ready to land.Mar 11 2019, 4:55 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2019, 3:44 AM