This is an archive of the discontinued LLVM Phabricator instance.

Add setting to require hardware breakpoints.
ClosedPublic

Authored by JDevlieghere on Nov 7 2018, 12:34 PM.

Details

Summary

When debugging read-only memory we cannot use software breakpoint. We already have support for hardware breakpoints and users can specify them with -H. However, there's no option to force hardware breakpoints even while stepping.

This patch adds a setting target.require-hardware-breakpoint that forces LLDB to always use hardware breakpoints. Because hardware breakpoints are a limited resource and therefore can fail to resolve, this patch also extends error handling in thread plans, where breakpoints are used for stepping.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

JDevlieghere created this revision.Nov 7 2018, 12:34 PM
labath added a subscriber: labath.Nov 7 2018, 12:52 PM

I recall something about linux on arm having a magic unmodifiable (even by ptrace) page of memory, so this could be useful there too. However, it's not clear to me how a user is going to figure out that he needs to enable this setting. Would it make sense to automatically try setting a hardware breakpoint if software breakpoint fails?

On NetBSD one has to check PaX MPROTECT property of a traced process.

Something like:

bool IsMPROTECT(pid_t pid) {
#if defined(__NetBSD__)
  int mib[3];
  int paxflags;
  size_t len = sizeof(paxflags);

  mib[0] = CTL_PROC;
  mib[1] = pid;
  mib[2] = PROC_PID_PAXFLAGS;

  if (sysctl(mib, 3, &paxflags, &len, NULL, 0) != 0)
    err(EXIT_FAILURE, "sysctl"); /* or return true */

  return !!(paxflags & CTL_PROC_PAXFLAGS_MPROTECT);
#else
  return false;
#endif
}

If IsMPROTECT is true, then we must use hardware assisted/emulated breakpoints.

Fix bug for thread until and add test case.

I recall something about linux on arm having a magic unmodifiable (even by ptrace) page of memory, so this could be useful there too. However, it's not clear to me how a user is going to figure out that he needs to enable this setting. Would it make sense to automatically try setting a hardware breakpoint if software breakpoint fails?

My main concern would be that hardware breakpoints are a limited resource and not something we want to make transparent to the user, because it's only a matter of time before it fails. We'd also have to think about what happens to a breakpoint when one of its locations is a software bploc and the other a hardware one. I'm not against the idea but it would require some more thinking and definitely a separate patch.

Remove ValidatePlan implementation for ThreadPlanPython as I believe it's bogus.

jingham requested changes to this revision.Nov 7 2018, 4:44 PM

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.

include/lldb/Target/Thread.h
639

You need to change the HeaderDoc to describe the status parameter. Looks like you have to do this for all the QueueThreadPlan... functions.

include/lldb/Target/ThreadPlanBase.h
55 ↗(On Diff #173021)

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.

include/lldb/Target/ThreadPlanRunToAddress.h
66 ↗(On Diff #173021)

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.

packages/Python/lldbsuite/test/functionalities/breakpoint/require_hw_breakpoints/TestRequireHWBreakpoints.py
34–44

This can all be done with:

(target, process, stepping_thread) = lldbutil.run_to_line_breakpoint(SBFileSpec("main.c"), 1)

48

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.

source/API/SBThread.cpp
733

Shouldn't new_plan_status get reflected in the "error" parameter passed into SBThread::StepInto?

772

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.

825

Same comment here.

857–859

And here.

894

And here.

source/API/SBThreadPlan.cpp
161

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.

source/Commands/CommandObjectThread.cpp
799

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.

source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
165–166

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.

source/Target/StopInfo.cpp
721–722

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.

1044

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.

source/Target/ThreadPlanPython.cpp
56

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.

source/Target/ThreadPlanStepInRange.cpp
178

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.

251

You need to plumb a Status through here, this plan would like to know why creating a step out plan failed here.

source/Target/ThreadPlanStepInstruction.cpp
200

We need to hold onto this status and report it in the thread plan's stop description.

This revision now requires changes to proceed.Nov 7 2018, 4:44 PM
JDevlieghere marked 18 inline comments as done.

Feedback from Jim

Update tests to use SB API.

JDevlieghere marked an inline comment as done.Nov 8 2018, 11:29 AM

I recall something about linux on arm having a magic unmodifiable (even by ptrace) page of memory, so this could be useful there too. However, it's not clear to me how a user is going to figure out that he needs to enable this setting. Would it make sense to automatically try setting a hardware breakpoint if software breakpoint fails?

My main concern would be that hardware breakpoints are a limited resource and not something we want to make transparent to the user, because it's only a matter of time before it fails.

That is true, but on the other hand, you would only use hw breakpoints on those pieces of memory where you really need to instead of everywhere, which means (at least for the use case I have in mind) it be used very rarely. Of course, we would have to be careful do differentiate between reasons why setting a sw breakpoint failed (is it because the memory is RO, or some other reason like there is no memory at that address).

However, with this approach, it's still not clear to me how will the user know that he has to enable this setting? Will he get some sort of an error pointing here when the sw breakpoint fails? Or will you just enable this setting by default for targets where you know this is an issue?

jingham requested changes to this revision.Nov 14 2018, 12:31 PM

The lldb API's parameters are ordered input first than output. Pretty much all the API's that take a Status as a parameter take it as the last parameter. So it looks weird to have the Status &error first in the QueueThreadPlan... API's. This pattern gets annoying when you have default parameters, so it's okay to put the out parameters before the default parameters (though default parameters are also not so great, so removing them is also okay...)

Other than that this looked fine.

This revision now requires changes to proceed.Nov 14 2018, 12:31 PM

Feedback Jim: move argument to the end of the list.

jingham accepted this revision.Nov 14 2018, 2:48 PM

Looks good to me.

This revision is now accepted and ready to land.Nov 14 2018, 2:48 PM
This revision was automatically updated to reflect the committed changes.