[lldb] Allow to register frame recognizers applied beyond the first instruction

[malor]

Summary

It is currently possible to register a frame recognizer, but it will be applied if and only if the frame's PC points to the very first instruction of the specified function, which limits usability of this feature.

The implementation already supports changing this behaviour by passing an additional flag, but it's not possible to set it via the command interface. Fix that.

[jingham]

One of the key reasons to use "frame recognizers" is to provide argument values to functions that don't have debug information. That generally only works when stopped at the first instruction, since otherwise you have to follow the value as it moves through the code, which in the absence of debug information isn't entirely trivial.

A naive user might think reporting expr (SomeType *) $arg1 is going to always work, but in fact it only works reliably on the first instruction.

I don't think that possible incorrect usage means we should only allow frame recognizers on the first instruction. But I do think we should say something more in the help for this flag, like "remember that $arg<N> is only reliable on the first instruction" to warn folks in advance of this pitfall.

[malor]

In D108510#2961332, @jingham wrote:

One of the key reasons to use "frame recognizers" is to provide argument values to functions that don't have debug information. That generally only works when stopped at the first instruction, since otherwise you have to follow the value as it moves through the code, which in the absence of debug information isn't entirely trivial.

A naive user might think reporting expr (SomeType *) $arg1 is going to always work, but in fact it only works reliably on the first instruction.

I don't think that possible incorrect usage means we should only allow frame recognizers on the first instruction. But I do think we should say something more in the help for this flag, like "remember that $arg<N> is only reliable on the first instruction" to warn folks in advance of this pitfall.

Thank you, Jim! That makes sense. I figured it was something along those lines.

Funnily enough, the way I ran into this issue is that I tried to add a frame recognizer for a binary that had debug information. In that case, doing something like breakpoint set -n foo actually sets the breakpoint *right after* the prologue of a function, so execution won't stop at the first instruction but at something like foo+4, and thus the frame recognizer won't be applied.

I think we should give users a choice and keep the current behavior of only applying recognizers to the first instruction as the default. I'll upload a new revision with the updated documentation.

[malor]

Updated the description of the flag to incorporate Jim's feedback.

[jingham]

LGTM

[malor]

Thank you, Jim! Would you be able to submit this change?

The clang-tidy premerge check has failed, but I don't think it's actionable: it's complaining about an #include in the file that I'm not modifying -- I saw it failing in the same way (although, on different files) in other review patches as well.

[mib]

Hi Roman, thanks for your patch. It also looks good to me, so I'll land it for you.

[malor]

In D108510#2971249, @mib wrote:

Hi Roman, thanks for your patch. It also looks good to me, so I'll land it for you.

Thank you, Med!

lldb/source/Commands/CommandObjectFrame.cpp

 //===-- CommandObjectFrame.cpp --------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #include "CommandObjectFrame.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/DataVisualization.h"
 #include "lldb/DataFormatters/ValueObjectPrinter.h"
 #include "lldb/Host/Config.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupFormat.h"
 #include "lldb/Interpreter/OptionGroupValueObjectDisplay.h"
 #include "lldb/Interpreter/OptionGroupVariable.h"
 #include "lldb/Interpreter/Options.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
public:
     ~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;
 
       switch (short_option) {
+      case 'f': {
+        bool value, success;
+        value = OptionArgParser::ToBoolean(option_arg, true, &success);
+        if (success) {
+          m_first_instruction_only = value;
+        } else {
+          error.SetErrorStringWithFormat(
+              "invalid boolean value '%s' passed for -f option",
+              option_arg.str().c_str());
+        }
+      } break;
       case 'l':
         m_class_name = std::string(option_arg);
         break;
       case 's':
         m_module = std::string(option_arg);
         break;
       case 'n':
         m_symbols.push_back(std::string(option_arg));
         break;
       case 'x':
         m_regex = true;
         break;
       default:
         llvm_unreachable("Unimplemented option");
       }
 
       return error;
     }
 
     void OptionParsingStarting(ExecutionContext *execution_context) override {
       m_module = "";
       m_symbols.clear();
       m_class_name = "";
       m_regex = false;
+      m_first_instruction_only = true;
     }
 
     llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
       return llvm::makeArrayRef(g_frame_recognizer_add_options);
     }
 
     // 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;
+    bool m_first_instruction_only;
   };
 
   CommandOptions m_options;
 
   Options *GetOptions() override { return &m_options; }
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override;
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);
+        recognizer_sp, module, func, m_options.m_first_instruction_only);
   } 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);
+        recognizer_sp, module, symbols, m_options.m_first_instruction_only);
   }
 #endif
 
   result.SetStatus(eReturnStatusSuccessFinishNoResult);
   return result.Succeeded();
 }
 
 class CommandObjectFrameRecognizerClear : public CommandObjectParsed {

lldb/source/Commands/Options.td

def frame_recognizer_function : Option<"function", "n">, Arg<"Name">,
     Completion<"Symbol">,
     Desc<"Name of the function that this recognizer applies to. "
          "Can be specified more than once except if -x|--regex is provided.">;
   def frame_recognizer_python_class : Option<"python-class", "l">, Group<2>,
     Arg<"PythonClass">,
     Desc<"Give the name of a Python class to use for this frame recognizer.">;
   def frame_recognizer_regex : Option<"regex", "x">,
     Desc<"Function name and module name are actually regular expressions.">;
+  def frame_recognizer_first_instruction_only : Option<"first-instruction-only", "f">, Arg<"Boolean">,
+    Desc<"If true, only apply this recognizer to frames whose PC currently points to the "
+    "first instruction of the specified function. If false, the recognizer "
+    "will always be applied, regardless of the current position within the specified function. The "
+    "implementor should keep in mind that some features, e.g. accessing function argument "
+    "values via $arg<N>, are not guaranteed to work reliably in this case, so extra care must "
+    "be taken to make the recognizer operate correctly. Defaults to true.">;
 }
 
 let Command = "history" in {
   def history_count : Option<"count", "c">, Group<1>, Arg<"UnsignedInteger">,
     Desc<"How many history commands to print.">;
   def history_start_index : Option<"start-index", "s">, Group<1>,
     Arg<"UnsignedInteger">, Desc<"Index at which to start printing history "
     "commands (or end to mean tail mode).">;

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_not_only_first_instruction(self):
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        # Clear internal & plugins recognizers that get initialized at launch.
+        self.runCmd("frame recognizer clear")
+
+        self.runCmd("command script import " + os.path.join(self.getSourceDir(), "recognizer.py"))
+
+        self.expect("frame recognizer list",
+                    substrs=['no matching results found.'])
+
+        # Create a target.
+        target, process, thread, _ = lldbutil.run_to_name_breakpoint(self, "foo",
+                                                                 exe_name = exe)
+
+        # Move the PC one instruction further.
+        self.runCmd("next")
+
+        # Add a frame recognizer in that target.
+        self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar")
+
+        # It's not applied to foo(), because frame's PC is not at the first instruction of the function.
+        self.expect("frame recognizer info 0",
+                    substrs=['frame 0 not recognized by any recognizer'])
+
+        # Add a frame recognizer with --first-instruction-only=true.
+        self.runCmd("frame recognizer clear")
+
+        self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar --first-instruction-only=true")
+
+        # It's not applied to foo(), because frame's PC is not at the first instruction of the function.
+        self.expect("frame recognizer info 0",
+                    substrs=['frame 0 not recognized by any recognizer'])
+
+        # Now add a frame recognizer with --first-instruction-only=false.
+        self.runCmd("frame recognizer clear")
+
+        self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo -n bar --first-instruction-only=false")
+
+        # This time it should recognize the frame.
+        self.expect("frame recognizer info 0",
+                    substrs=['frame 0 is recognized by recognizer.MyFrameRecognizer'])
+
+        opts = lldb.SBVariablesOptions()
+        opts.SetIncludeRecognizedArguments(True)
+        frame = thread.GetSelectedFrame()
+        variables = frame.GetVariables(opts)
+
+        self.assertEqual(variables.GetSize(), 2)
+        self.assertEqual(variables.GetValueAtIndex(0).name, "a")
+        self.assertEqual(variables.GetValueAtIndex(0).signed, 42)
+        self.assertEqual(variables.GetValueAtIndex(0).GetValueType(), lldb.eValueTypeVariableArgument)
+        self.assertEqual(variables.GetValueAtIndex(1).name, "b")
+        self.assertEqual(variables.GetValueAtIndex(1).signed, 56)
+        self.assertEqual(variables.GetValueAtIndex(1).GetValueType(), lldb.eValueTypeVariableArgument)
+
     @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."])
         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."])
+
+    @no_debug_info_test
+    def test_frame_recognizer_add_invalid_arg(self):
+        self.expect("frame recognizer add -f", error=True,
+                    substrs=["error: last option requires an argument"])
+        self.expect("frame recognizer add -f -1", error=True,
+                    substrs=["error: invalid boolean value '-1' passed for -f option"])
+        self.expect("frame recognizer add -f foo", error=True,
+                    substrs=["error: invalid boolean value 'foo' passed for -f option"])