[Lldb-commits] [lldb] 5477fbc - [lldb] Make deleting frame recognizers actually work
Raphael Isemann via lldb-commits
lldb-commits at lists.llvm.org
Thu Jul 23 08:44:05 PDT 2020
Author: Raphael Isemann
Date: 2020-07-23T17:43:37+02:00
New Revision: 5477fbc294469a1cc543d6d816c0b1519e632735
URL: https://github.com/llvm/llvm-project/commit/5477fbc294469a1cc543d6d816c0b1519e632735
DIFF: https://github.com/llvm/llvm-project/commit/5477fbc294469a1cc543d6d816c0b1519e632735.diff
LOG: [lldb] Make deleting frame recognizers actually work
Summary:
Frame recognizers are stored alongside a flag that indicates whether they were
deleted by the user. If the flag is set, they are supposed to be ignored by the
rest of the frame recognizer code. 'frame recognizer delete' is supposed to set
that flag. 'frame recognizer clear' however actually deletes all frame
recognizers (so, it doesn't set the flag but directly deletes them from the
list).
The current implementation of this concept is pretty broken. `frame recognizer
delete` sets the flag, but it somehow thinks that the recognizer id is an index
in the recognizer list. That's not true as it's actually just a member of each
recognizer entry. So it actually just sets the `deleted` flag for a random other
recognizer. The tests for the recognizer still pass as `frame recognizer list`
is also broken and just completely ignored the `deleted` flag and lists all
recognizers. Also `frame recognizer delete` just ignores if it can't actually
delete a recognizer if the id is invalid.
I think we can simplify this whole thing by just actually deleting recognizers
instead of making sure all code is actually respecting the `deleted` flag. I
assume the intention of this was to make sure that all recognizers are getting
unique ids over the course of an LLDB session, but as `clear` is actually
deleting them and we keep recycling ids, that didn't really work to begin with.
This patch deletes the `deleted` flag and just actually deletes the stored
recognizer. Also adds the missing error message in case it find a recognizer
with a given id.
Reviewers: mib
Reviewed By: mib
Subscribers: abidh, JDevlieghere
Differential Revision: https://reviews.llvm.org/D84404
Added:
Modified:
lldb/include/lldb/Target/StackFrameRecognizer.h
lldb/source/Commands/CommandObjectFrame.cpp
lldb/source/Target/StackFrameRecognizer.cpp
lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/StackFrameRecognizer.h b/lldb/include/lldb/Target/StackFrameRecognizer.h
index 38b21e7c9856..302b56bec907 100644
--- a/lldb/include/lldb/Target/StackFrameRecognizer.h
+++ b/lldb/include/lldb/Target/StackFrameRecognizer.h
@@ -125,7 +125,6 @@ class StackFrameRecognizerManager {
private:
struct RegisteredEntry {
uint32_t recognizer_id;
- bool deleted;
lldb::StackFrameRecognizerSP recognizer;
bool is_regexp;
ConstString module;
diff --git a/lldb/source/Commands/CommandObjectFrame.cpp b/lldb/source/Commands/CommandObjectFrame.cpp
index b42020d76751..c7b67b59d288 100644
--- a/lldb/source/Commands/CommandObjectFrame.cpp
+++ b/lldb/source/Commands/CommandObjectFrame.cpp
@@ -999,9 +999,14 @@ class CommandObjectFrameRecognizerDelete : public CommandObjectParsed {
return false;
}
- GetSelectedOrDummyTarget()
- .GetFrameRecognizerManager()
- .RemoveRecognizerWithID(recognizer_id);
+ if (!GetSelectedOrDummyTarget()
+ .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();
}
diff --git a/lldb/source/Target/StackFrameRecognizer.cpp b/lldb/source/Target/StackFrameRecognizer.cpp
index 6fa09a387ad2..73d22d5bb4e6 100644
--- a/lldb/source/Target/StackFrameRecognizer.cpp
+++ b/lldb/source/Target/StackFrameRecognizer.cpp
@@ -50,17 +50,17 @@ ScriptedStackFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame) {
void StackFrameRecognizerManager::AddRecognizer(
StackFrameRecognizerSP recognizer, ConstString module,
llvm::ArrayRef<ConstString> symbols, bool first_instruction_only) {
- m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer,
- false, module, RegularExpressionSP(), symbols,
+ m_recognizers.push_front({(uint32_t)m_recognizers.size(), recognizer, false,
+ module, RegularExpressionSP(), symbols,
RegularExpressionSP(), first_instruction_only});
}
void StackFrameRecognizerManager::AddRecognizer(
StackFrameRecognizerSP recognizer, RegularExpressionSP module,
RegularExpressionSP symbol, bool first_instruction_only) {
- m_recognizers.push_front(
- {(uint32_t)m_recognizers.size(), false, recognizer, true, ConstString(),
- module, std::vector<ConstString>(), symbol, first_instruction_only});
+ m_recognizers.push_front({(uint32_t)m_recognizers.size(), recognizer, true,
+ ConstString(), module, std::vector<ConstString>(),
+ symbol, first_instruction_only});
}
void StackFrameRecognizerManager::ForEach(
@@ -90,9 +90,13 @@ bool StackFrameRecognizerManager::RemoveRecognizerWithID(
uint32_t recognizer_id) {
if (recognizer_id >= m_recognizers.size())
return false;
- if (m_recognizers[recognizer_id].deleted)
+ 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;
- m_recognizers[recognizer_id].deleted = true;
+ m_recognizers.erase(found);
return true;
}
@@ -116,8 +120,6 @@ StackFrameRecognizerManager::GetRecognizerForFrame(StackFrameSP frame) {
Address current_addr = frame->GetFrameCodeAddress();
for (auto entry : m_recognizers) {
- if (entry.deleted)
- continue;
if (entry.module)
if (entry.module != module_name)
continue;
diff --git a/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py b/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
index f807937f2f17..b025a2255f07 100644
--- a/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
+++ b/lldb/test/API/commands/frame/recognizer/TestFrameRecognizer.py
@@ -44,8 +44,24 @@ def test_frame_recognizer_1(self):
self.runCmd("frame recognizer delete 0")
+ # Test that it deleted the recognizer with id 0.
self.expect("frame recognizer list",
substrs=['1: recognizer.MyOtherFrameRecognizer, module a.out, symbol bar (regexp)'])
+ self.expect("frame recognizer list", matching=False,
+ substrs=['MyFrameRecognizer'])
+
+ # Test that an invalid index and deleting the same index again
+ # is an error and doesn't do any changes.
+ self.expect("frame recognizer delete 2", error=True,
+ substrs=["error: '2' is not a valid recognizer id."])
+ self.expect("frame recognizer delete 0", error=True,
+ substrs=["error: '0' is not a valid recognizer id."])
+ # Recognizers should have the same state as above.
+ self.expect("frame recognizer list",
+ substrs=['1: recognizer.MyOtherFrameRecognizer, module a.out, symbol bar (regexp)'])
+ self.expect("frame recognizer list", matching=False,
+ substrs=['MyFrameRecognizer'])
+
self.runCmd("frame recognizer clear")
More information about the lldb-commits
mailing list