RenderScript pending kernel breakpoints.

[EwanCrawford]

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'.

[clayborg]

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

[jingham]

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.

[EwanCrawford]

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.

[EwanCrawford]

Does this address your concerns Jim?

[jingham]

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.

lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.h

 #include "lldb/lldb-private.h"
 #include "lldb/Target/LanguageRuntime.h"
 #include "lldb/Target/CPPLanguageRuntime.h"
 #include "lldb/Core/Module.h"
 
 namespace lldb_private
 {
 
+namespace lldb_renderscript
+{
+
 typedef uint32_t RSSlot;
 class RSModuleDescriptor;
 struct RSGlobalDescriptor;
 struct RSKernelDescriptor;
 
 typedef std::shared_ptr<RSModuleDescriptor> RSModuleDescriptorSP;
 typedef std::shared_ptr<RSGlobalDescriptor> RSGlobalDescriptorSP;
 typedef std::shared_ptr<RSKernelDescriptor> RSKernelDescriptorSP;
 
+// Breakpoint Resolvers decide where a breakpoint is placed,
+// so having our own allows us to limit the search scope to RS kernel modules.
+// As well as check for .expand kernels as a fallback.
+class RSBreakpointResolver : public BreakpointResolver
+{
+  public:
+
+    RSBreakpointResolver(Breakpoint *bkpt, ConstString name):
+                         BreakpointResolver (bkpt, BreakpointResolver::NameResolver),
+                         m_kernel_name(name)
+    {
+    }
+
+    void
+    GetDescription(Stream *strm) override
+    {
+        if (strm)
+            strm->Printf("RenderScript kernel breakpoint for '%s'", m_kernel_name.AsCString());
+    }
 
+    void
+    Dump(Stream *s) const override
+    {
+    }
+
+    Searcher::CallbackReturn
+    SearchCallback(SearchFilter &filter,
+                   SymbolContext &context,
+                   Address *addr,
+                   bool containing) override;
+
+    Searcher::Depth
+    GetDepth() override
+    {
+        return Searcher::eDepthModule;
+    }
+
+    lldb::BreakpointResolverSP
+    CopyForBreakpoint(Breakpoint &breakpoint) override
+    {
+        lldb::BreakpointResolverSP ret_sp(new RSBreakpointResolver(&breakpoint, m_kernel_name));
+        return ret_sp;
+    }
+
+  protected:
+    ConstString m_kernel_name;
+};
 
 struct RSKernelDescriptor
 {
   public:
     RSKernelDescriptor(const RSModuleDescriptor *module, const char *name, uint32_t slot)
         : m_module(module)
         , m_name(name)
         , m_slot(slot)
public:
 
     const lldb::ModuleSP m_module;
     std::vector<RSKernelDescriptor> m_kernels;
     std::vector<RSGlobalDescriptor> m_globals;
     std::map<std::string, std::string> m_pragmas;
     std::string m_resname;
 };
 
+} // end lldb_renderscript namespace
+
 class RenderScriptRuntime : public lldb_private::CPPLanguageRuntime
 {
   public:
 
     enum ModuleKind
     {
         eModuleKindIgnored,
         eModuleKindLibRS,
public:
     bool ProbeModules(const ModuleList module_list);
 
     void DumpModules(Stream &strm) const;
 
     void DumpContexts(Stream &strm) const;
 
     void DumpKernels(Stream &strm) const;
 
-    void AttemptBreakpointAtKernelName(Stream &strm, const char *name, Error &error);
+    void AttemptBreakpointAtKernelName(Stream &strm, const char *name, Error &error, lldb::TargetSP target);
 
     void Status(Stream &strm) const;
 
     virtual size_t GetAlternateManglings(const ConstString &mangled, std::vector<ConstString> &alternates) {
         return static_cast<size_t>(0);
     }
 
     virtual void ModulesDidLoad(const ModuleList &module_list );
 
     void Update();
 
     void Initiate();
     
   protected:
     
-    void FixupScriptDetails(RSModuleDescriptorSP rsmodule_sp);
+    void FixupScriptDetails(lldb_renderscript::RSModuleDescriptorSP rsmodule_sp);
 
     void LoadRuntimeHooks(lldb::ModuleSP module, ModuleKind kind);
     
     struct RuntimeHook;
     typedef void (RenderScriptRuntime::*CaptureStateFn)(RuntimeHook* hook_info, ExecutionContext &context);  // Please do this!
 
     struct HookDefn
     {
struct ScriptDetails
         std::string cachedir;
         lldb::addr_t context;
         lldb::addr_t script;
     };
 
     lldb::ModuleSP m_libRS;
     lldb::ModuleSP m_libRSDriver;
     lldb::ModuleSP m_libRSCpuRef;
-    std::vector<RSModuleDescriptorSP> m_rsmodules;
+    std::vector<lldb_renderscript::RSModuleDescriptorSP> m_rsmodules;
     std::vector<ScriptDetails> m_scripts;
 
-    std::map<lldb::addr_t, RSModuleDescriptorSP> m_scriptMappings;
+    std::map<lldb::addr_t, lldb_renderscript::RSModuleDescriptorSP> m_scriptMappings;
     std::map<lldb::addr_t, RuntimeHookSP> m_runtimeHooks;
 
     bool m_initiated;
     bool m_debuggerPresentFlagged;
     static const HookDefn s_runtimeHookDefns[];
     static const size_t s_runtimeHookCount;
 
   private:

lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp

 #include "lldb/Interpreter/CommandObjectMultiword.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Target/RegisterContext.h"
 
 #include "lldb/Symbol/VariableList.h"
 
 using namespace lldb;
 using namespace lldb_private;
+using namespace lldb_renderscript;
 
 //------------------------------------------------------------------
 // Static Functions
 //------------------------------------------------------------------
 LanguageRuntime *
 RenderScriptRuntime::CreateInstance(Process *process, lldb::LanguageType language)
 {
 
     if (language == eLanguageTypeExtRenderScript)
         return new RenderScriptRuntime(process);
     else
         return NULL;
 }
 
+// Callback with a module to search for matching symbols.
+// We first check that the module contains RS kernels.
+// Then look for a symbol which matches our kernel name.
+// The breakpoint address is finally set using the address of this symbol.
+Searcher::CallbackReturn
+RSBreakpointResolver::SearchCallback(SearchFilter &filter,
+                                     SymbolContext &context,
+                                     Address*,
+                                     bool)
+{
+    ModuleSP module = context.module_sp;
+
+    if (!module)
+        return Searcher::eCallbackReturnContinue;
+
+    // Is this a module containing renderscript kernels?
+    if (nullptr == module->FindFirstSymbolWithNameAndType(ConstString(".rs.info"), eSymbolTypeData))
+        return Searcher::eCallbackReturnContinue;
+
+    // Attempt to set a breakpoint on the kernel name symbol within the module library.
+    // If it's not found, it's likely debug info is unavailable - try to set a
+    // breakpoint on <name>.expand.
+
+    const Symbol* kernel_sym = module->FindFirstSymbolWithNameAndType(m_kernel_name, eSymbolTypeCode);
+    if (!kernel_sym)
+    {
+        std::string kernel_name_expanded(m_kernel_name.AsCString());
+        kernel_name_expanded.append(".expand");
+        kernel_sym = module->FindFirstSymbolWithNameAndType(ConstString(kernel_name_expanded.c_str()), eSymbolTypeCode);
+    }
+
+    if (kernel_sym)
+    {
+        Address bp_addr = kernel_sym->GetAddress();
+        if (filter.AddressPasses(bp_addr))
+            m_breakpoint->AddLocation(bp_addr);
+    }
+
+    return Searcher::eCallbackReturnContinue;
+}
+
 void
 RenderScriptRuntime::Initialize()
 {
     PluginManager::RegisterPlugin(GetPluginNameStatic(), "RenderScript language support", CreateInstance, GetCommandObject);
 }
 
 void
 RenderScriptRuntime::Terminate()
for (const auto &module : m_rsmodules)
         {
             strm.Indent(kernel.m_name.AsCString());
             strm.EOL();
         }
     }
     strm.IndentLess();
 }
 
-void 
-RenderScriptRuntime::AttemptBreakpointAtKernelName(Stream &strm, const char* name, Error& error)
+void
+RenderScriptRuntime::AttemptBreakpointAtKernelName(Stream &strm, const char* name, Error& error, TargetSP target)
 {
-    if (!name) 
+    if (!name)
     {
         error.SetErrorString("invalid kernel name");
         return;
     }
 
-    bool kernels_found = false;
-    ConstString kernel_name(name);
-    for (const auto &module : m_rsmodules)
-    {
-        for (const auto &kernel : module->m_kernels)
-        {
-            if (kernel.m_name == kernel_name)
-            {
-                //Attempt to set a breakpoint on this symbol, within the module library
-                //If it's not found, it's likely debug info is unavailable - set a
-                //breakpoint on <name>.expand and emit a warning.
-
-                const Symbol* kernel_sym = module->m_module->FindFirstSymbolWithNameAndType(kernel_name, eSymbolTypeCode);
-
-                if (!kernel_sym)
-                {
-                    std::string kernel_name_expanded(name);
-                    kernel_name_expanded.append(".expand");
-                    kernel_sym = module->m_module->FindFirstSymbolWithNameAndType(ConstString(kernel_name_expanded.c_str()), eSymbolTypeCode);
-
-                    if (kernel_sym)
-                    {
-                        strm.Printf("Kernel '%s' could not be found, but expansion exists. ", name); 
-                        strm.Printf("Breakpoint placed on expanded kernel. Have you compiled in debug mode?");
-                        strm.EOL();
-                    }
-                    else
-                    {
-                        error.SetErrorStringWithFormat("Could not locate symbols for loaded kernel '%s'.", name);
-                        return;
-                    }
-                }
-
-                addr_t bp_addr = kernel_sym->GetLoadAddress(&GetProcess()->GetTarget());
-                if (bp_addr == LLDB_INVALID_ADDRESS)
-                {
-                    error.SetErrorStringWithFormat("Could not locate load address for symbols of kernel '%s'.", name);
-                    return;
-                }
+    SearchFilterSP filter_sp(new SearchFilterForUnconstrainedSearches(target));
 
-                BreakpointSP bp = GetProcess()->GetTarget().CreateBreakpoint(bp_addr, false, false);
-                strm.Printf("Breakpoint %" PRIu64 ": kernel '%s' within script '%s'", (uint64_t)bp->GetID(), name, module->m_resname.c_str());
-                strm.EOL();
+    ConstString kernel_name(name);
+    BreakpointResolverSP resolver_sp(new RSBreakpointResolver(nullptr, kernel_name));
 
-                kernels_found = true;
-            }
-        }
-    }
+    BreakpointSP bp = target->CreateBreakpoint(filter_sp, resolver_sp, false, false, false);
+    if (bp)
+        bp->GetDescription(&strm, lldb::eDescriptionLevelInitial, false);
 
-    if (!kernels_found)
-    {
-        error.SetErrorString("kernel name not found");
-    }
     return;
 }
 
 void
 RenderScriptRuntime::DumpModules(Stream &strm) const
 {
     strm.Printf("RenderScript Modules:");
     strm.EOL();

 };
 
 class CommandObjectRenderScriptRuntimeKernelBreakpoint : public CommandObjectParsed
 {
   private:
   public:
     CommandObjectRenderScriptRuntimeKernelBreakpoint(CommandInterpreter &interpreter)
         : CommandObjectParsed(interpreter, "renderscript kernel breakpoint",
-                              "Sets a breakpoint on a renderscript kernel.", "renderscript kernel breakpoint",
+                              "Sets a breakpoint on a renderscript kernel.", "renderscript kernel breakpoint <kernel_name>",
                               eCommandRequiresProcess | eCommandProcessMustBeLaunched | eCommandProcessMustBePaused)
     {
     }
 
     ~CommandObjectRenderScriptRuntimeKernelBreakpoint() {}
 
     bool
     DoExecute(Args &command, CommandReturnObject &result)
     {
         const size_t argc = command.GetArgumentCount();
         if (argc == 1)
         {
             RenderScriptRuntime *runtime =
                 (RenderScriptRuntime *)m_exe_ctx.GetProcessPtr()->GetLanguageRuntime(eLanguageTypeExtRenderScript);
 
             Error error;
-            runtime->AttemptBreakpointAtKernelName(result.GetOutputStream(), command.GetArgumentAtIndex(0), error);
+            runtime->AttemptBreakpointAtKernelName(result.GetOutputStream(), command.GetArgumentAtIndex(0),
+                                                   error, m_exe_ctx.GetTargetSP());
 
             if (error.Success())
             {
                 result.AppendMessage("Breakpoint(s) created");
                 result.SetStatus(eReturnStatusSuccessFinishResult);
                 return true;
             }
             result.SetStatus(eReturnStatusFailed);