[Lldb-commits] [PATCH] D54221: Add setting to require hardware breakpoints.
Jim Ingham via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Wed Nov 7 16:44:30 PST 2018
jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
This is pretty good, but in all the places where some plan tries to find a sub-plan to do its job, you are losing the text of the error when that job fails. So the controlling plan can't present a good error message. You need to hold onto the Status object and return that, either as the error from ValidatePlan if it happens when the plan is getting created, or in the plan's stop description so that StopInfoThreadPlan::GetDescription can print it properly.
================
Comment at: include/lldb/Target/Thread.h:643
//------------------------------------------------------------------
- virtual lldb::ThreadPlanSP QueueFundamentalPlan(bool abort_other_plans);
+ virtual lldb::ThreadPlanSP QueueFundamentalPlan(Status &status,
+ bool abort_other_plans);
----------------
You need to change the HeaderDoc to describe the status parameter. Looks like you have to do this for all the QueueThreadPlan... functions.
================
Comment at: include/lldb/Target/ThreadPlanBase.h:55
friend lldb::ThreadPlanSP
- Thread::QueueFundamentalPlan(bool abort_other_plans);
+ Thread::QueueFundamentalPlan(Status &status, bool abort_other_plans);
----------------
I don't think it is useful to pass an error in this case. The "Fundamental plan" just fields unhandled responses from other plans, and queuing it can never fail.
================
Comment at: include/lldb/Target/ThreadPlanRunToAddress.h:66
// using to stop us at m_address.
+ bool m_could_not_resolve_hw_bp;
----------------
Looks like you are adding this to most of the plans. Would it make sense to add it to the ThreadPlan base class? This is just an error flag, so it would stay false except is a derived plan wants to set it.
================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/require_hw_breakpoints/TestRequireHWBreakpoints.py:33-43
+ exe = self.getBuildArtifact("a.out")
+ target = self.dbg.CreateTarget(exe)
+
+ breakpoint = target.BreakpointCreateByLocation("main.c", 1)
+
+ self.runCmd("run")
+
----------------
This can all be done with:
(target, process, stepping_thread) = lldbutil.run_to_line_breakpoint(SBFileSpec("main.c"), 1)
================
Comment at: packages/Python/lldbsuite/test/functionalities/breakpoint/require_hw_breakpoints/TestRequireHWBreakpoints.py:47
+
+ self.expect("thread step-in")
+ self.expect("thread step-in", error=True)
----------------
Can you do this using "SBThread.StepInto" and check the error return in the case where it fails? Since we are treating the possibility of step's failing, we need to make sure that the SB API's return errors everywhere and that the errors look right. So it would be good to test that.
Ditto for the other stepping tests.
================
Comment at: source/API/SBThread.cpp:733
new_plan_sp = thread->QueueThreadPlanForStepSingleInstruction(
- false, abort_other_plans, stop_other_threads);
+ new_plan_status, false, abort_other_plans, stop_other_threads);
}
----------------
Shouldn't new_plan_status get reflected in the "error" parameter passed into SBThread::StepInto?
================
Comment at: source/API/SBThread.cpp:767
+ new_plan_status, abort_other_plans, NULL, false, stop_other_threads,
+ eVoteYes, eVoteNoOpinion, 0, avoid_no_debug));
----------------
Same here. If new_plan_status comes back with an error, we probably don't want to call ResumeNewPlan, and we want to report the error from queueing the plan.
================
Comment at: source/API/SBThread.cpp:818
+ Status new_plan_status;
ThreadPlanSP new_plan_sp(thread->QueueThreadPlanForStepOut(
----------------
Same comment here.
================
Comment at: source/API/SBThread.cpp:847
Thread *thread = exe_ctx.GetThreadPtr();
- ThreadPlanSP new_plan_sp(
- thread->QueueThreadPlanForStepSingleInstruction(step_over, true, true));
+ Status new_plan_status;
+ ThreadPlanSP new_plan_sp(thread->QueueThreadPlanForStepSingleInstruction(
----------------
And here.
================
Comment at: source/API/SBThread.cpp:881
+ Status new_plan_status;
ThreadPlanSP new_plan_sp(thread->QueueThreadPlanForRunToAddress(
----------------
And here.
================
Comment at: source/API/SBThreadPlan.cpp:155
start_address->CalculateSymbolContext(&sc);
+ Status plan_status;
return SBThreadPlan(
----------------
Can you add a variant of these calls that takes an SBError &? The scripted thread plans will need to have a way to report this error when they try to queue a plan and it fails. And below as well.
================
Comment at: source/Commands/CommandObjectThread.cpp:802
} else {
+ result.SetError(new_plan_status);
result.AppendError("Couldn't find thread plan to implement step type.");
----------------
If new_plan_status.Fail is true, then you DID find a thread plan to implement the step type, it just failed to work. So it's confusing to append "Couldn't find..." in that case.
================
Comment at: source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:169
m_run_to_sp = m_thread.QueueThreadPlanForStepOutNoShouldStop(
- abort_other_plans, &sc, first_insn, m_stop_others, eVoteNoOpinion,
- eVoteNoOpinion, frame_idx);
+ status, abort_other_plans, &sc, first_insn, m_stop_others,
+ eVoteNoOpinion, eVoteNoOpinion, frame_idx);
----------------
You need to do something if status.Fail() == true here. IIUC, QueueThreadPlan... will return an empty sp, so you don't want to call SetPrivate on it for sure.
================
Comment at: source/Target/StopInfo.cpp:731
+ ));
new_plan_sp->SetIsMasterPlan(true);
new_plan_sp->SetOkayToDiscard(false);
----------------
Again, you should check the new_plan_status here. If it is failure, you need to get out of here, and touching the new_plan_sp is probably a bad idea.
================
Comment at: source/Target/StopInfo.cpp:1047
StreamString strm;
m_plan_sp->GetDescription(&strm, eDescriptionLevelBrief);
+ if (!m_plan_sp->PlanSucceeded())
----------------
The plan's description should have the failure message, since then it can have details. But if that's true you would see in the stop message:
stop reason = step out failed - couldn't set hardware breakpoint (FAILED)
I'm not sure the (FAILED) helps.
================
Comment at: source/Target/ThreadPlanPython.cpp:49
-bool ThreadPlanPython::ValidatePlan(Stream *error) {
- // I have to postpone setting up the implementation till after the constructor
- // because I need to call
- // shared_from_this, which you can't do in the constructor. So I'll do it
- // here.
- if (m_implementation_sp)
- return true;
- else
- return false;
-}
+bool ThreadPlanPython::ValidatePlan(Stream *error) { return true; }
----------------
Rather than remove this, set a flag to remind yourself whether ThreadPlanPython::DidPush has been called. Return true if it hasn't (because at that point you don't know whether this will succeed or not) and then do the check here if it has been.
Then whoever is queuing the plan can still usefully check ValidatePlan after Queueing it.
In general, you need to check ValidatePlan both after making the plan and after doing DidPush, which is sometimes "the second half of constructing the plan." So working ValidatePlan this way would cover both uses.
================
Comment at: source/Target/ThreadPlanStepInRange.cpp:171
} else {
+ Status sub_plan_status;
// Stepping through should be done running other threads in general, since
----------------
It seems like you just lose the information of why creating the sub-plan failed. In this code, it's okay if nobody could figure out a way to go somewhere useful from here, but in that case you would get no m_sub_plan_sp BUT also no error. If you get an error then we need to report the error or things will just silently fail with no explanation, which would be confusing.
================
Comment at: source/Target/ThreadPlanStepInRange.cpp:244
if (!m_sub_plan_sp && frame_order == eFrameCompareYounger)
m_sub_plan_sp = CheckShouldStopHereAndQueueStepOut(frame_order);
----------------
You need to plumb a Status through here, this plan would like to know why creating a step out plan failed here.
================
Comment at: source/Target/ThreadPlanStepInstruction.cpp:194
const bool stop_others = false;
+ Status status;
m_thread.QueueThreadPlanForStepOutNoShouldStop(
----------------
We need to hold onto this status and report it in the thread plan's stop description.
https://reviews.llvm.org/D54221
More information about the lldb-commits
mailing list