[lldb] Add a --dummy option to the frame recognizer commands

[teemperor]

Summary

As suggested in D83757, frame recognizers should have an option to act on the dummy
target instead of the real target (similar to breakpoints). The implementation is essentially
just what the breakpoint command is doing.

[teemperor]

• Fix diff

[mib]

Just a little nit, but as we already discussed offline, I have some doubts about adding a --dummy option.
It sounds unnecessary to me, so I'd like to hear @jingham take on this.

lldb/source/Commands/CommandObjectFrame.cpp

768–772

I think we can get rid of the method override by giving m_use_dummy a default value.

[jingham]

This is doing the right thing. Ismail and I talked a little off-line and I allayed his concerns.

Except for the bit where it would be really nice not to duplicate the OptionGroupDummy in two places, this looks good to me.

lldb/source/Commands/CommandObjectFrame.cpp

738

It would be nice not to make two of these. Can we make an OptionGroupDummy in source/Interpreter and then use it here and in the Breakpoint commands?

[teemperor]

lldb/source/Commands/CommandObjectFrame.cpp

738

Sure, but then the two options would have the same description. Right now it's already a quite vague to make it fit to all the different subcommands ("Act on Dummy frame recognizers - i.e. frame recognizers added before a file is provided, which prime new targets."), so then it would be even more abstract.

Open for suggestions, something like this maybe?

"Modify the dummy target instead of the selected target. The dummy target's properties will be inherited by new targets".

768–772

Sadly I don't think that's not how the Option/OptionGroup work (but IMHO they probably should work that way). The function above is supposed to reset the object to the default state. So if we use a default value then the first 'frame recognizer * -D' invocation will set the '-D' flag for all following invocations.

// Subclasses must reset their option values prior to starting a new option
// parse. Each subclass must override this function and revert all option
// settings to default values.
virtual void OptionParsingStarting(ExecutionContext *execution_context) = 0;

Fun fact: I thought the same thing when I touched 'frame var' a few months ago and when I changed that code 'up'/'down' ended up being broken for a few months...

[teemperor]

lldb/source/Commands/CommandObjectFrame.cpp

738

Actually nevermind, I just realized you probably meant that the class code should be shared.

lldb/source/Commands/CommandObjectFrame.cpp

protected:
   OptionGroupOptions m_option_group;
   OptionGroupVariable m_option_variable;
   OptionGroupFormat m_option_format;
   OptionGroupValueObjectDisplay m_varobj_options;
 };
 
 #pragma mark CommandObjectFrameRecognizer
 
+#define LLDB_OPTIONS_frame_recognizer_dummy
+#include "CommandOptions.inc"
+
+class FrameRecognizerDummyOptionGroup : public OptionGroup {
+public:
+  FrameRecognizerDummyOptionGroup() : OptionGroup() {}
+
+  ~FrameRecognizerDummyOptionGroup() override = default;
+
+  llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+    return llvm::makeArrayRef(g_frame_recognizer_dummy_options);
+  }
+
+  Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+                        ExecutionContext *execution_context) override {
+    Status error;
+    const int short_option =
+        g_frame_recognizer_dummy_options[option_idx].short_option;
+
+    switch (short_option) {
+    case 'D':
+      m_use_dummy = true;
+      break;
+    default:
+      llvm_unreachable("Unimplemented option");
+    }
+
+    return error;
+  }
+
+  void OptionParsingStarting(ExecutionContext *execution_context) override {
+    m_use_dummy = false;
+  }
+
+  bool m_use_dummy;
+};
+
 #define LLDB_OPTIONS_frame_recognizer_add
 #include "CommandOptions.inc"
 
 class CommandObjectFrameRecognizerAdd : public CommandObjectParsed {
 private:
-  class CommandOptions : public Options {
+  class CommandOptions : public OptionGroup {
   public:
-    CommandOptions() : Options() {}
+    CommandOptions() = default;
     ~CommandOptions() override = default;
 
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
                           ExecutionContext *execution_context) override {
       Status error;
-      const int short_option = m_getopt_table[option_idx].val;
+      const int short_option =
+          g_frame_recognizer_add_options[option_idx].short_option;
 
       switch (short_option) {
       case 'l':
         m_class_name = std::string(option_arg);
         break;
       case 's':
         m_module = std::string(option_arg);
         break;
public:
 
     // Instance variables to hold the values for command options.
     std::string m_class_name;
     std::string m_module;
     std::vector<std::string> m_symbols;
     bool m_regex;
   };
 
+  FrameRecognizerDummyOptionGroup m_dummy_options;
   CommandOptions m_options;
+  OptionGroupOptions m_all_options;
 
-  Options *GetOptions() override { return &m_options; }
+  Options *GetOptions() override { return &m_all_options; }
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override;
 
 public:
   CommandObjectFrameRecognizerAdd(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "frame recognizer add",
                             "Add a new frame recognizer.", nullptr),
         m_options() {
+    m_all_options.Append(&m_dummy_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_ALL);
+    m_all_options.Append(&m_options);
+    m_all_options.Finalize();
     SetHelpLong(R"(
 Frame recognizers allow for retrieving information about special frames based on
 ABI, arguments or other special properties of that frame, even without source
 code or debug info. Currently, one use case is to extract function arguments
 that would otherwise be unaccesible, or augment existing arguments.
 
 Adding a custom frame recognizer is possible by implementing a Python class
 and using the 'frame recognizer add' command. The Python class should have a
#if LLDB_ENABLE_PYTHON
   StackFrameRecognizerSP recognizer_sp =
       StackFrameRecognizerSP(new ScriptedStackFrameRecognizer(
           interpreter, m_options.m_class_name.c_str()));
   if (m_options.m_regex) {
     auto module =
         RegularExpressionSP(new RegularExpression(m_options.m_module));
     auto func =
         RegularExpressionSP(new RegularExpression(m_options.m_symbols.front()));
-    GetSelectedOrDummyTarget().GetFrameRecognizerManager().AddRecognizer(
-        recognizer_sp, module, func);
+    GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy)
+        .GetFrameRecognizerManager()
+        .AddRecognizer(recognizer_sp, module, func);
   } else {
     auto module = ConstString(m_options.m_module);
     std::vector<ConstString> symbols(m_options.m_symbols.begin(),
                                      m_options.m_symbols.end());
-    GetSelectedOrDummyTarget().GetFrameRecognizerManager().AddRecognizer(
-        recognizer_sp, module, symbols);
+    GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy)
+        .GetFrameRecognizerManager()
+        .AddRecognizer(recognizer_sp, module, symbols);
   }
 #endif
 
   result.SetStatus(eReturnStatusSuccessFinishNoResult);
   return result.Succeeded();
 }
 
 class CommandObjectFrameRecognizerClear : public CommandObjectParsed {
+  FrameRecognizerDummyOptionGroup m_dummy_options;
+  OptionGroupOptions m_all_options;
+
 public:
   CommandObjectFrameRecognizerClear(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "frame recognizer clear",
-                            "Delete all frame recognizers.", nullptr) {}
+                            "Delete all frame recognizers.", nullptr) {
+    m_all_options.Append(&m_dummy_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_ALL);
+    m_all_options.Finalize();
+  }
 
   ~CommandObjectFrameRecognizerClear() override = default;
 
+  Options *GetOptions() override { return &m_all_options; }
+
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
-    GetSelectedOrDummyTarget()
+    GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy)
         .GetFrameRecognizerManager()
         .RemoveAllRecognizers();
     result.SetStatus(eReturnStatusSuccessFinishResult);
     return result.Succeeded();
   }
 };
 
 class CommandObjectFrameRecognizerDelete : public CommandObjectParsed {
+  FrameRecognizerDummyOptionGroup m_dummy_options;
+  OptionGroupOptions m_all_options;
+
 public:
   CommandObjectFrameRecognizerDelete(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "frame recognizer delete",
-                            "Delete an existing frame recognizer.", nullptr) {}
+                            "Delete an existing frame recognizer.", nullptr) {
+    m_all_options.Append(&m_dummy_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_ALL);
+    m_all_options.Finalize();
+  }
 
   ~CommandObjectFrameRecognizerDelete() override = default;
 
+  Options *GetOptions() override { return &m_all_options; }
+
   void
   HandleArgumentCompletion(CompletionRequest &request,
                            OptionElementVector &opt_element_vector) override {
     if (request.GetCursorIndex() != 0)
       return;
 
-    GetSelectedOrDummyTarget().GetFrameRecognizerManager().ForEach(
-        [&request](uint32_t rid, std::string rname, std::string module,
+    GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy)
+        .GetFrameRecognizerManager()
+        .ForEach([&request](uint32_t rid, std::string rname, std::string module,
                             llvm::ArrayRef<lldb_private::ConstString> symbols,
                             bool regexp) {
           StreamString strm;
           if (rname.empty())
             rname = "(internal)";
 
           strm << rname;
           if (!module.empty())
             strm << ", module " << module;
           if (!symbols.empty())
if (command.GetArgumentCount() == 0) {
       if (!m_interpreter.Confirm(
               "About to delete all frame recognizers, do you want to do that?",
               true)) {
         result.AppendMessage("Operation cancelled...");
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
 
-      GetSelectedOrDummyTarget()
+      GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy)
           .GetFrameRecognizerManager()
           .RemoveAllRecognizers();
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return result.Succeeded();
     }
 
     if (command.GetArgumentCount() != 1) {
       result.AppendErrorWithFormat("'%s' takes zero or one arguments.\n",
                                    m_cmd_name.c_str());
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
     uint32_t recognizer_id;
     if (!llvm::to_integer(command.GetArgumentAtIndex(0), recognizer_id)) {
       result.AppendErrorWithFormat("'%s' is not a valid recognizer id.\n",
                                    command.GetArgumentAtIndex(0));
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
-    if (!GetSelectedOrDummyTarget()
+    if (!GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy)
              .GetFrameRecognizerManager()
              .RemoveRecognizerWithID(recognizer_id)) {
       result.AppendErrorWithFormat("'%s' is not a valid recognizer id.\n",
                                    command.GetArgumentAtIndex(0));
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
     result.SetStatus(eReturnStatusSuccessFinishResult);
     return result.Succeeded();
   }
 };
 
 class CommandObjectFrameRecognizerList : public CommandObjectParsed {
+  FrameRecognizerDummyOptionGroup m_dummy_options;
+  OptionGroupOptions m_all_options;
+
 public:
   CommandObjectFrameRecognizerList(CommandInterpreter &interpreter)
       : CommandObjectParsed(interpreter, "frame recognizer list",
                             "Show a list of active frame recognizers.",
-                            nullptr) {}
+                            nullptr) {
+    m_all_options.Append(&m_dummy_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_ALL);
+    m_all_options.Finalize();
+  }
 
   ~CommandObjectFrameRecognizerList() override = default;
 
+  Options *GetOptions() override { return &m_all_options; }
+
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     bool any_printed = false;
-    GetSelectedOrDummyTarget().GetFrameRecognizerManager().ForEach(
-        [&result, &any_printed](
-            uint32_t recognizer_id, std::string name, std::string module,
-            llvm::ArrayRef<ConstString> symbols, bool regexp) {
+    GetSelectedOrDummyTarget(m_dummy_options.m_use_dummy)
+        .GetFrameRecognizerManager()
+        .ForEach([&result, &any_printed](uint32_t recognizer_id,
+                                         std::string name, std::string module,
+                                         llvm::ArrayRef<ConstString> symbols,
+                                         bool regexp) {
           Stream &stream = result.GetOutputStream();
 
           if (name.empty())
             name = "(internal)";
 
           stream << std::to_string(recognizer_id) << ": " << name;
           if (!module.empty())
             stream << ", module " << module;

lldb/source/Commands/Options.td

def frame_diag_offset : Option<"offset", "o">, Group<1>, Arg<"Offset">,
     Desc<"An optional offset.  Requires --register.">;
 }
 
 let Command = "frame select" in {
   def frame_select_relative : Option<"relative", "r">, Group<1>, Arg<"Offset">,
     Desc<"A relative frame index offset from the current frame index.">;
 }
 
+let Command = "frame recognizer dummy" in {
+  def recognizer_dummy_options_dummy :
+    Option<"dummy-frame-recognizer", "D">, Group<1>,
+    Desc<"Act on Dummy frame recognizers - i.e. frame recognizers added before"
+    " a file is provided, which prime new targets.">;
+}
+
 let Command = "frame recognizer add" in {
   def frame_recognizer_shlib : Option<"shlib", "s">, Arg<"ShlibName">,
     Completion<"Module">,
     Desc<"Name of the module or shared library that this recognizer applies "
     "to.">;
   def frame_recognizer_function : Option<"function", "n">, Arg<"Name">,
     Completion<"Symbol">,
     Desc<"Name of the function that this recognizer applies to. "

lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py

def test_frame_recognizer_target_specific(self):
 
         self.expect("frame recognizer list",
                     substrs=['recognizer.MyFrameRecognizer, module a.out, symbol bar'])
 
         # Now the new target should also recognize the frame.
         self.expect("frame recognizer info 0",
                     substrs=['frame 0 is recognized by recognizer.MyFrameRecognizer'])
 
+    @skipUnlessDarwin
+    def test_frame_recognizer_dummy_target(self):
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        # Clear internal & plugins recognizers that get initialized at launch.
+        self.runCmd("frame recognizer clear -D")
+
+        self.runCmd("command script import " + os.path.join(self.getSourceDir(), "recognizer.py"))
+
+        # Add a frame recognizer to the dummy target.
+        self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo")
+        # Should be in the dummy target (which is used even without '-D' as
+        # there is no selected target).
+        self.expect("frame recognizer list -D",
+                    substrs=['recognizer.MyFrameRecognizer, module a.out, symbol foo'])
+        self.expect("frame recognizer list",
+                    substrs=['recognizer.MyFrameRecognizer, module a.out, symbol foo'])
+
+        # Create a real target now.
+        lldbutil.run_to_name_breakpoint(self, "foo", exe_name = exe)
+
+        # The frame recognizer is still in the dummy target.
+        self.expect("frame recognizer list -D",
+                    substrs=['recognizer.MyFrameRecognizer, module a.out, symbol foo'])
+        # And also in the real target.
+        self.expect("frame recognizer list",
+                    substrs=['recognizer.MyFrameRecognizer, module a.out, symbol foo'])
+
+        # Delete recognizers from the dummy target.
+        self.runCmd("frame recognizer clear -D")
+
+        # The recognizer should be gone from the dummy target.
+        self.expect("frame recognizer list -D",
+                    matching=False, substrs=['MyFrameRecognizer'])
+        # But the real target should still have it.
+        self.expect("frame recognizer list",
+                    substrs=['recognizer.MyFrameRecognizer, module a.out, symbol foo'])
+
+        # Add a recognizer to the real target now.
+        self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n bar")
+        # The dummy target should not have this new recognizer.
+        self.expect("frame recognizer list -D",
+                    matching=False, substrs=['MyFrameRecognizer'])
+        self.runCmd("frame recognizer clear")
+
+        # Add a recognizer to the dummy target.
+        self.runCmd("frame recognizer add -D -l recognizer.MyFrameRecognizer -s a.out -n bar")
+        # The recognizer should be in the dummy target.
+        self.expect("frame recognizer list -D",
+                    substrs=['recognizer.MyFrameRecognizer, module a.out, symbol bar'])
+        # The recognizer shouldn't be in the real target.
+        self.expect("frame recognizer list",
+                    substrs=['recognizer.MyFrameRecognizer, module a.out, symbol bar'])
+
+        # Delete the recognizer from the dummy target by index.
+        self.runCmd("frame recognizer delete -D 0")
+        self.expect("frame recognizer list -D",
+                    matching=False, substrs=['MyFrameRecognizer'])
+
     @no_debug_info_test
     def test_frame_recognizer_delete_invalid_arg(self):
         self.expect("frame recognizer delete a", error=True,
                     substrs=["error: 'a' is not a valid recognizer id."])
         self.expect("frame recognizer delete \"\"", error=True,
                     substrs=["error: '' is not a valid recognizer id."])
         self.expect("frame recognizer delete -1", error=True,
-                    substrs=["error: '-1' is not a valid recognizer id."])
+                    substrs=["error: unknown or ambiguous option"])
         self.expect("frame recognizer delete 4294967297", error=True,
                     substrs=["error: '4294967297' is not a valid recognizer id."])
 
     @no_debug_info_test
     def test_frame_recognizer_info_invalid_arg(self):
         self.expect("frame recognizer info a", error=True,
                     substrs=["error: 'a' is not a valid frame index."])
         self.expect("frame recognizer info \"\"", error=True,
                     substrs=["error: '' is not a valid frame index."])
         self.expect("frame recognizer info -1", error=True,
                     substrs=["error: '-1' is not a valid frame index."])
         self.expect("frame recognizer info 4294967297", error=True,
                     substrs=["error: '4294967297' is not a valid frame index."])