This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Decouple IsMasterPlan and OkayToDiscard (NFC)
Needs ReviewPublic

Authored by kastiglione on Feb 15 2021, 8:06 AM.

Details

Reviewers
jingham
Summary

While learning the semantics of IsMasterPlan and OkayToDiscard, and guided by the
FIXME comment in ThreadPlan.h, this change decouples the two functions.

These two functions have been called in pairs, for example:

SetIsMasterPlan(true);
SetOkayToDiscard(false);

With this change, almost all cases turn into a single SetIsMasterPlan(true) call. Note
that m_okay_to_discard now defaults to false.

Most of the decoupling happens in ThreadPlanStack::DiscardConsultingMasterPlans, which
handles OkayToDiscard and IsMasterPlan individually. This function could be renamed,
but I haven't thought of a good alternative.

Other notes:

  • OkayToDiscard is no longer virtual
  • ThreadPlanPython was incorrectly calling SetOkayToDiscard(true)
  • Reworded some comments

Diff Detail

Event Timeline

kastiglione requested review of this revision.Feb 15 2021, 8:06 AM
kastiglione created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2021, 8:06 AM

Restore a comment.

kastiglione added inline comments.Feb 15 2021, 9:28 AM
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
72

This is the only remaining SetOkayToDiscard(true) call.

I don't understand how DiscardPlansConsultingMasterPlan in this patch works.

With your changes, every plan but the ObjC trampoline finder returns false from OkayToDiscard. So if you are trying to discard plans up to the innermost master plan, you start from the youngest plan asking it if is "OkayToDiscard". Since no plans return false, the iteration will go all the way to the Base thread plan regardless of whether there were intervening master plans, which isn't right. Why didn't you ask IsMasterPlan in that loop rather than OkayToDiscard?

Was there a problem with just saying "Everything but master plans can be discarded when we are discarding plans." I'm not sure why the ObjC Trampoline step through thread plan needs to be a Master Plan. It seems to me it's just another subsidiary plan to a step in plan or something like. Maybe it would be better to eliminate the OkayToDiscard altogether, and figure out a better way to handle the ObjC trampoline, if that actually needs doing.