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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
