[lldb] Add support for disabling frame recognizers #109219
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sometimes you only want to temporarily disable a frame recognizer instead of deleting it. In particular, when dealing with one of the builtin frame recognizers, which cannot be restored after deletion.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jimingham
reviewed
Sep 19, 2024
|
This looks good to me so far, but of course it needs tests. |
|
@llvm/pr-subscribers-lldb Author: Adrian Vogelsgesang (vogelsgesang) ChangesSometimes you only want to temporarily disable a frame recognizer instead of deleting it. In particular, when dealing with one of the builtin frame recognizers, which cannot be restored after deletion. To be able to write test cases for this functionality, I also changed Patch is 22.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/109219.diff 7 Files Affected:
diff --git a/lldb/include/lldb/Target/StackFrameRecognizer.h b/lldb/include/lldb/Target/StackFrameRecognizer.h
index 617b1617d404a1..6c67a7fb4f68dd 100644
--- a/lldb/include/lldb/Target/StackFrameRecognizer.h
+++ b/lldb/include/lldb/Target/StackFrameRecognizer.h
@@ -124,12 +124,14 @@ class StackFrameRecognizerManager {
Mangled::NamePreference symbol_mangling,
bool first_instruction_only = true);
- void ForEach(std::function<
- void(uint32_t recognizer_id, std::string recognizer_name,
- std::string module, llvm::ArrayRef<ConstString> symbols,
- Mangled::NamePreference name_reference, bool regexp)> const
- &callback);
-
+ void
+ ForEach(std::function<void(uint32_t recognizer_id, bool enabled,
+ std::string recognizer_name, std::string module,
+ llvm::ArrayRef<ConstString> symbols,
+ Mangled::NamePreference name_preference,
+ bool regexp)> const &callback);
+
+ bool SetEnabledForID(uint32_t recognizer_id, bool enabled);
bool RemoveRecognizerWithID(uint32_t recognizer_id);
void RemoveAllRecognizers();
@@ -155,6 +157,7 @@ class StackFrameRecognizerManager {
lldb::RegularExpressionSP symbol_regexp;
Mangled::NamePreference symbol_mangling;
bool first_instruction_only;
+ bool enabled;
};
std::deque<RegisteredEntry> m_recognizers;
diff --git a/lldb/source/Commands/CommandObjectFrame.cpp b/lldb/source/Commands/CommandObjectFrame.cpp
index d8091e8993fde1..d42cbeaba15f22 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -31,8 +31,10 @@
#include "lldb/Target/Thread.h"
#include "lldb/Utility/Args.h"
+#include <iostream>
#include <memory>
#include <optional>
+#include <ostream>
#include <string>
using namespace lldb;
@@ -930,10 +932,13 @@ class CommandObjectFrameRecognizerClear : public CommandObjectParsed {
};
static void
-PrintRecognizerDetails(Stream &strm, const std::string &name,
+PrintRecognizerDetails(Stream &strm, const std::string &name, bool enabled,
const std::string &module,
llvm::ArrayRef<lldb_private::ConstString> symbols,
Mangled::NamePreference symbol_mangling, bool regexp) {
+ if (!enabled)
+ strm << "[disabled] ";
+
strm << name << ", ";
if (!module.empty())
@@ -957,53 +962,45 @@ PrintRecognizerDetails(Stream &strm, const std::string &name,
llvm::interleaveComma(symbols, strm);
}
-class CommandObjectFrameRecognizerDelete : public CommandObjectParsed {
+// Base class for commands which accept a single frame recognizer as an argument
+class CommandObjectWithFrameRecognizerArg : public CommandObjectParsed {
public:
- CommandObjectFrameRecognizerDelete(CommandInterpreter &interpreter)
- : CommandObjectParsed(interpreter, "frame recognizer delete",
- "Delete an existing frame recognizer by id.",
- nullptr) {
+ CommandObjectWithFrameRecognizerArg(CommandInterpreter &interpreter,
+ const char *name,
+ const char *help = nullptr,
+ const char *syntax = nullptr,
+ uint32_t flags = 0)
+ : CommandObjectParsed(interpreter, name, help, syntax, flags) {
AddSimpleArgumentList(eArgTypeRecognizerID);
}
- ~CommandObjectFrameRecognizerDelete() override = default;
-
void
HandleArgumentCompletion(CompletionRequest &request,
OptionElementVector &opt_element_vector) override {
+ std::cerr << request.GetCursorIndex() << std::endl;
if (request.GetCursorIndex() != 0)
return;
GetTarget().GetFrameRecognizerManager().ForEach(
- [&request](uint32_t rid, std::string rname, std::string module,
+ [&request](uint32_t rid, bool enabled, std::string rname,
+ std::string module,
llvm::ArrayRef<lldb_private::ConstString> symbols,
Mangled::NamePreference symbol_mangling, bool regexp) {
StreamString strm;
if (rname.empty())
rname = "(internal)";
- PrintRecognizerDetails(strm, rname, module, symbols, symbol_mangling,
- regexp);
+ PrintRecognizerDetails(strm, rname, enabled, module, symbols,
+ symbol_mangling, regexp);
request.TryCompleteCurrentArg(std::to_string(rid), strm.GetString());
});
}
-protected:
- void DoExecute(Args &command, CommandReturnObject &result) override {
- 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...");
- return;
- }
-
- GetTarget().GetFrameRecognizerManager().RemoveAllRecognizers();
- result.SetStatus(eReturnStatusSuccessFinishResult);
- return;
- }
+ virtual void DoExecuteWithId(CommandReturnObject &result,
+ uint32_t recognizer_id) = 0;
+ void DoExecute(Args &command, CommandReturnObject &result) override {
if (command.GetArgumentCount() != 1) {
result.AppendErrorWithFormat("'%s' takes zero or one arguments.\n",
m_cmd_name.c_str());
@@ -1017,10 +1014,79 @@ class CommandObjectFrameRecognizerDelete : public CommandObjectParsed {
return;
}
- if (!GetTarget().GetFrameRecognizerManager().RemoveRecognizerWithID(
- recognizer_id)) {
- result.AppendErrorWithFormat("'%s' is not a valid recognizer id.\n",
- command.GetArgumentAtIndex(0));
+ DoExecuteWithId(result, recognizer_id);
+ }
+};
+
+class CommandObjectFrameRecognizerEnable
+ : public CommandObjectWithFrameRecognizerArg {
+public:
+ CommandObjectFrameRecognizerEnable(CommandInterpreter &interpreter)
+ : CommandObjectWithFrameRecognizerArg(
+ interpreter, "frame recognizer enable",
+ "Enable a frame recognizer by id.", nullptr) {
+ AddSimpleArgumentList(eArgTypeRecognizerID);
+ }
+
+ ~CommandObjectFrameRecognizerEnable() override = default;
+
+protected:
+ void DoExecuteWithId(CommandReturnObject &result,
+ uint32_t recognizer_id) override {
+ auto &recognizer_mgr = GetTarget().GetFrameRecognizerManager();
+ if (!recognizer_mgr.SetEnabledForID(recognizer_id, true)) {
+ result.AppendErrorWithFormat("'%u' is not a valid recognizer id.\n",
+ recognizer_id);
+ return;
+ }
+ result.SetStatus(eReturnStatusSuccessFinishResult);
+ }
+};
+
+class CommandObjectFrameRecognizerDisable
+ : public CommandObjectWithFrameRecognizerArg {
+public:
+ CommandObjectFrameRecognizerDisable(CommandInterpreter &interpreter)
+ : CommandObjectWithFrameRecognizerArg(
+ interpreter, "frame recognizer disable",
+ "Disable a frame recognizer by id.", nullptr) {
+ AddSimpleArgumentList(eArgTypeRecognizerID);
+ }
+
+ ~CommandObjectFrameRecognizerDisable() override = default;
+
+protected:
+ void DoExecuteWithId(CommandReturnObject &result,
+ uint32_t recognizer_id) override {
+ auto &recognizer_mgr = GetTarget().GetFrameRecognizerManager();
+ if (!recognizer_mgr.SetEnabledForID(recognizer_id, false)) {
+ result.AppendErrorWithFormat("'%u' is not a valid recognizer id.\n",
+ recognizer_id);
+ return;
+ }
+ result.SetStatus(eReturnStatusSuccessFinishResult);
+ }
+};
+
+class CommandObjectFrameRecognizerDelete
+ : public CommandObjectWithFrameRecognizerArg {
+public:
+ CommandObjectFrameRecognizerDelete(CommandInterpreter &interpreter)
+ : CommandObjectWithFrameRecognizerArg(
+ interpreter, "frame recognizer delete",
+ "Delete an existing frame recognizer by id.", nullptr) {
+ AddSimpleArgumentList(eArgTypeRecognizerID);
+ }
+
+ ~CommandObjectFrameRecognizerDelete() override = default;
+
+protected:
+ void DoExecuteWithId(CommandReturnObject &result,
+ uint32_t recognizer_id) override {
+ auto &recognizer_mgr = GetTarget().GetFrameRecognizerManager();
+ if (!recognizer_mgr.RemoveRecognizerWithID(recognizer_id)) {
+ result.AppendErrorWithFormat("'%u' is not a valid recognizer id.\n",
+ recognizer_id);
return;
}
result.SetStatus(eReturnStatusSuccessFinishResult);
@@ -1041,7 +1107,7 @@ class CommandObjectFrameRecognizerList : public CommandObjectParsed {
bool any_printed = false;
GetTarget().GetFrameRecognizerManager().ForEach(
[&result,
- &any_printed](uint32_t recognizer_id, std::string name,
+ &any_printed](uint32_t recognizer_id, bool enabled, std::string name,
std::string module, llvm::ArrayRef<ConstString> symbols,
Mangled::NamePreference symbol_mangling, bool regexp) {
Stream &stream = result.GetOutputStream();
@@ -1050,8 +1116,8 @@ class CommandObjectFrameRecognizerList : public CommandObjectParsed {
name = "(internal)";
stream << std::to_string(recognizer_id) << ": ";
- PrintRecognizerDetails(stream, name, module, symbols, symbol_mangling,
- regexp);
+ PrintRecognizerDetails(stream, name, enabled, module, symbols,
+ symbol_mangling, regexp);
stream.EOL();
stream.Flush();
@@ -1135,18 +1201,24 @@ class CommandObjectFrameRecognizer : public CommandObjectMultiword {
interpreter, "frame recognizer",
"Commands for editing and viewing frame recognizers.",
"frame recognizer [<sub-command-options>] ") {
+ LoadSubCommand("info", CommandObjectSP(new CommandObjectFrameRecognizerInfo(
+ interpreter)));
+ LoadSubCommand("list", CommandObjectSP(new CommandObjectFrameRecognizerList(
+ interpreter)));
LoadSubCommand("add", CommandObjectSP(new CommandObjectFrameRecognizerAdd(
interpreter)));
LoadSubCommand(
- "clear",
- CommandObjectSP(new CommandObjectFrameRecognizerClear(interpreter)));
+ "enable",
+ CommandObjectSP(new CommandObjectFrameRecognizerEnable(interpreter)));
+ LoadSubCommand(
+ "disable",
+ CommandObjectSP(new CommandObjectFrameRecognizerDisable(interpreter)));
LoadSubCommand(
"delete",
CommandObjectSP(new CommandObjectFrameRecognizerDelete(interpreter)));
- LoadSubCommand("list", CommandObjectSP(new CommandObjectFrameRecognizerList(
- interpreter)));
- LoadSubCommand("info", CommandObjectSP(new CommandObjectFrameRecognizerInfo(
- interpreter)));
+ LoadSubCommand(
+ "clear",
+ CommandObjectSP(new CommandObjectFrameRecognizerClear(interpreter)));
}
~CommandObjectFrameRecognizer() override = default;
diff --git a/lldb/source/Target/StackFrameRecognizer.cpp b/lldb/source/Target/StackFrameRecognizer.cpp
index fa24253320a3f2..d23c1fa1a928be 100644
--- a/lldb/source/Target/StackFrameRecognizer.cpp
+++ b/lldb/source/Target/StackFrameRecognizer.cpp
@@ -67,7 +67,7 @@ void StackFrameRecognizerManager::AddRecognizer(
m_recognizers.push_front({(uint32_t)m_recognizers.size(), recognizer, false,
module, RegularExpressionSP(), symbols,
RegularExpressionSP(), symbol_mangling,
- first_instruction_only});
+ first_instruction_only, true});
BumpGeneration();
}
@@ -77,14 +77,15 @@ void StackFrameRecognizerManager::AddRecognizer(
bool first_instruction_only) {
m_recognizers.push_front({(uint32_t)m_recognizers.size(), recognizer, true,
ConstString(), module, std::vector<ConstString>(),
- symbol, symbol_mangling, first_instruction_only});
+ symbol, symbol_mangling, first_instruction_only,
+ true});
BumpGeneration();
}
void StackFrameRecognizerManager::ForEach(
- const std::function<
- void(uint32_t, std::string, std::string, llvm::ArrayRef<ConstString>,
- Mangled::NamePreference name_reference, bool)> &callback) {
+ const std::function<void(
+ uint32_t, bool, std::string, std::string, llvm::ArrayRef<ConstString>,
+ Mangled::NamePreference name_preference, bool)> &callback) {
for (auto entry : m_recognizers) {
if (entry.is_regexp) {
std::string module_name;
@@ -95,22 +96,32 @@ void StackFrameRecognizerManager::ForEach(
if (entry.symbol_regexp)
symbol_name = entry.symbol_regexp->GetText().str();
- callback(entry.recognizer_id, entry.recognizer->GetName(), module_name,
- llvm::ArrayRef(ConstString(symbol_name)), entry.symbol_mangling,
- true);
-
+ callback(entry.recognizer_id, entry.enabled, entry.recognizer->GetName(),
+ module_name, llvm::ArrayRef(ConstString(symbol_name)),
+ entry.symbol_mangling, true);
} else {
- callback(entry.recognizer_id, entry.recognizer->GetName(),
+ callback(entry.recognizer_id, entry.enabled, entry.recognizer->GetName(),
entry.module.GetCString(), entry.symbols, entry.symbol_mangling,
false);
}
}
}
+bool StackFrameRecognizerManager::SetEnabledForID(uint32_t recognizer_id,
+ bool enabled) {
+ auto found =
+ llvm::find_if(m_recognizers, [recognizer_id](const RegisteredEntry &e) {
+ return e.recognizer_id == recognizer_id;
+ });
+ if (found == m_recognizers.end())
+ return false;
+ found->enabled = enabled;
+ BumpGeneration();
+ return true;
+}
+
bool StackFrameRecognizerManager::RemoveRecognizerWithID(
uint32_t recognizer_id) {
- if (recognizer_id >= m_recognizers.size())
- return false;
auto found =
llvm::find_if(m_recognizers, [recognizer_id](const RegisteredEntry &e) {
return e.recognizer_id == recognizer_id;
@@ -142,6 +153,9 @@ StackFrameRecognizerManager::GetRecognizerForFrame(StackFrameSP frame) {
Address current_addr = frame->GetFrameCodeAddress();
for (auto entry : m_recognizers) {
+ if (!entry.enabled)
+ continue;
+
if (entry.module)
if (entry.module != module_name)
continue;
diff --git a/lldb/test/API/commands/frame/recognizer/Makefile b/lldb/test/API/commands/frame/recognizer/Makefile
index 09f6bd59a2fb32..796767e2425343 100644
--- a/lldb/test/API/commands/frame/recognizer/Makefile
+++ b/lldb/test/API/commands/frame/recognizer/Makefile
@@ -1,4 +1,4 @@
-OBJC_SOURCES := main.m
+C_SOURCES := main.c
CFLAGS_EXTRAS := -g0 # No debug info.
MAKE_DSYM := NO
diff --git a/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py b/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
index 24d6a67c0ccd48..17396fb2c5b37f 100644
--- a/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
+++ b/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
@@ -14,10 +14,13 @@
class FrameRecognizerTestCase(TestBase):
NO_DEBUG_INFO_TESTCASE = True
- @skipUnlessDarwin
def test_frame_recognizer_1(self):
self.build()
exe = self.getBuildArtifact("a.out")
+ target, process, thread, _ = lldbutil.run_to_name_breakpoint(
+ self, "foo", exe_name=exe
+ )
+ frame = thread.GetSelectedFrame()
# Clear internal & plugins recognizers that get initialized at launch
self.runCmd("frame recognizer clear")
@@ -96,11 +99,6 @@ def test_frame_recognizer_1(self):
"frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo"
)
- target, process, thread, _ = lldbutil.run_to_name_breakpoint(
- self, "foo", exe_name=exe
- )
- frame = thread.GetSelectedFrame()
-
self.expect("frame variable", substrs=["(int) a = 42", "(int) b = 56"])
# Recognized arguments don't show up by default...
@@ -164,7 +162,6 @@ def test_frame_recognizer_1(self):
substrs=['*a = 78'])
"""
- @skipUnlessDarwin
def test_frame_recognizer_hiding(self):
self.build()
@@ -204,7 +201,6 @@ def test_frame_recognizer_hiding(self):
frame = thread.GetSelectedFrame()
self.assertIn("main", frame.name)
- @skipUnlessDarwin
def test_frame_recognizer_multi_symbol(self):
self.build()
exe = self.getBuildArtifact("a.out")
@@ -250,7 +246,6 @@ def test_frame_recognizer_multi_symbol(self):
substrs=["frame 0 is recognized by recognizer.MyFrameRecognizer"],
)
- @skipUnlessDarwin
def test_frame_recognizer_target_specific(self):
self.build()
exe = self.getBuildArtifact("a.out")
@@ -318,7 +313,138 @@ def test_frame_recognizer_target_specific(self):
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(
+ ...
[truncated]
|
|
This is ready for review now. While adding the test cases, I had to migrate it from Objective-C to C, since I was unable to run it on my Linux machine otherwise |
vogelsgesang
force-pushed
the
avogelsgesang-recognizer-disable
branch
from
a788e1b to
488a322
Compare
vogelsgesang
force-pushed
the
avogelsgesang-recognizer-disable
branch
from
488a322 to
53410d7
Compare
Michael137
reviewed
Sep 20, 2024
Michael137
reviewed
Sep 20, 2024
Michael137
reviewed
Sep 20, 2024
Michael137
approved these changes
Sep 20, 2024
Michael137
approved these changes
Sep 20, 2024
|
@vogelsgesang Nice! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes you only want to temporarily disable a frame recognizer instead of deleting it. In particular, when dealing with one of the builtin frame recognizers, which cannot be restored after deletion.
To be able to write test cases for this functionality, I also changed
lldb/test/API/commands/frame/recognizerto use normal C instead of Objective-C