Page MenuHomePhabricator

RenderScript pending kernel breakpoints.
ClosedPublic

Authored by EwanCrawford on Aug 26 2015, 7:08 AM.

Details

Summary

Currently the RS breakpoint command can only find a kernel if it's in an already loaded RS module.
This patch allows users to set pending breakpoints on RenderScript kernels which will be loaded in the future.

To maintain these breakpoints the command 'renderscript kernel breakpoint' is split into 3 separate sub-commands.
'renderscript kernel breakpoint set', 'renderscript kernel breakpoint list', and 'renderscript kernel breakpoint delete'.

Diff Detail

Repository
rL LLVM

Event Timeline

EwanCrawford retitled this revision from to RenderScript pending kernel breakpoints..
EwanCrawford updated this object.
EwanCrawford added reviewers: clayborg, jingham.
EwanCrawford set the repository for this revision to rL LLVM.
clayborg accepted this revision.Aug 26 2015, 10:01 AM
clayborg edited edge metadata.

Looks good for now, but we should add a way so that plug-ins can extend the "breakpoint set", "breakpoint delete", and other breakpoint commands so we don't have a bunch of fragmented extra commands.

One idea for a future patch: allow plugins to define static functions callbacks that can add new options to breakpoint commands and have the command routed to the plug-in when these special commands are specified. Lets say you set a renderscript kernel breakpoint by installing a new --renderscript-kernel option:

(lldb) breakpoint set --renderscript-kernel ...

The arguments would get parsed up and passed to a plug-in static function when the breakpoint gets requested to be made. We might not have a renderscript runtime loaded yet because we haven't detected the renderscript shared library yet, so we would need to hold off on resolving the breakpoint until the RenderScriptRuntime plug-in is loaded, and as soon as it is, the breakpoint could resolve itself.

We have a desire for this with the Objective C runtime to do special breakpoints like:

(lldb) breakpoint set --objc-class NString
This revision is now accepted and ready to land.Aug 26 2015, 10:01 AM
jingham requested changes to this revision.Aug 26 2015, 11:13 AM
jingham edited edge metadata.

So there are two issues I see here.

The first is that you seem to have invented a parallel mechanism to the breakpoint resolver mechanism to handle re-resolving breakpoints on shared library loads. That seems unfortunate. You should have been able to add a special RSBreakpointResolver that will get called by the ordinary mechanism for breakpoints on new module load and then just make new "RS Kernel" breakpoints with that resolver. I would want a good reason why that wasn't possible before I would feel good about adding this side mechanism.

The second issue, which Greg pointed out, is that there's no way for a plugin to specify a new breakpoint resolver kind that it has added to the breakpoint system. Since that doesn't currently exist, and is a decent bit of work which it isn't fair to gate this patch on, I don't mind for the nonce adding a special command to set them. But the breakpoint's resolver describes the breakpoints in a resolver agnostic way, so once added there is no need for "list" or "delete" commands.

This revision now requires changes to proceed.Aug 26 2015, 11:13 AM
EwanCrawford edited edge metadata.

Thanks for taking a look.

Previously we just tried to find a way to leverage the existing BreakpointResovlerName, and SearchFilter.
However your suggestion to create a new BreakpointResolver works a lot better, and indeed removes the need for the extra commands.

This updated patch impements the RS breakpoint resolver, allowing us to set pending breakpoints on future kernels.

Does this address your concerns Jim?

That looks great. Here are some comments that are not requirements for getting this in but just for you to consider:

If you think you are ever likely to add different ways to specify the name (e.g. if passing a regex might be useful) it might be good to pass the name as an option (-n FOO) rather than a bare argument. Once you've made it an argument it is hard to add orthogonal options in a coherent way. OTOH, in the case of regex, you could for instance do:

(lldb) renderscript kernel breakpoint -r <NAME>

and actually we do have some commands that do it that way ("image lookup" being one) but since this is kind of a breakpoint command it would be better to make it look like the "breakpoint set" command. But that only works because a regex is just a fancy name somehow. Anyway, there are also other people around who wish I'd lay off the option thing a bit, so take this suggestion as you will...

I don't know if it is useful to you, but all the "by name" breakpoint commands also take a list of names, so you can do:

(lldb) break set -n Foo -n Bar -n Baz

That's useful if you want to have some devious command on all three of those breakpoints, or want to enable & disable them at a blow, or if you are interested in the aggregated hit count for the three or whatever. Be pretty easy to do that the way you've got it set up now.

Also, if you get some extra time, it doesn't look like it would be too hard to add a command completer for this name argument. It looks like you would just run over the modules looking for kernels, then match the input against those names. That would get tab completion on the name working automatically. This is extra credit, however.

This revision was automatically updated to reflect the committed changes.