[lldb] Make deleting frame recognizers actually work

[teemperor]

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.

[mib]

Looks good to me.

lldb/include/lldb/Target/StackFrameRecognizer.h

public:
 
   lldb::StackFrameRecognizerSP GetRecognizerForFrame(lldb::StackFrameSP frame);
 
   lldb::RecognizedStackFrameSP RecognizeFrame(lldb::StackFrameSP frame);
 
 private:
   struct RegisteredEntry {
     uint32_t recognizer_id;
-    bool deleted;
     lldb::StackFrameRecognizerSP recognizer;
     bool is_regexp;
     ConstString module;
     lldb::RegularExpressionSP module_regexp;
     std::vector<ConstString> symbols;
     lldb::RegularExpressionSP symbol_regexp;
     bool first_instruction_only;
   };

lldb/source/Commands/CommandObjectFrame.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
     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;
     }
 
-    GetSelectedOrDummyTarget()
+    if (!GetSelectedOrDummyTarget()
              .GetFrameRecognizerManager()
-        .RemoveRecognizerWithID(recognizer_id);
+             .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 {
 public:
   CommandObjectFrameRecognizerList(CommandInterpreter &interpreter)

lldb/source/Target/StackFrameRecognizer.cpp

ScriptedStackFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame) {
 
   return RecognizedStackFrameSP(
       new ScriptedRecognizedStackFrame(args_synthesized));
 }
 
 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(
     const std::function<void(uint32_t, std::string, std::string,
                              llvm::ArrayRef<ConstString>, bool)> &callback) {
   for (auto entry : m_recognizers) {
     if (entry.is_regexp) {
       std::string module_name;
for (auto entry : m_recognizers) {
     }
   }
 }
 
 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;
 }
 
 void StackFrameRecognizerManager::RemoveAllRecognizers() {
   m_recognizers.clear();
 }
 
 StackFrameRecognizerSP
 StackFrameRecognizerManager::GetRecognizerForFrame(StackFrameSP frame) {
   const SymbolContext &symctx = frame->GetSymbolContext(
       eSymbolContextModule | eSymbolContextFunction | eSymbolContextSymbol);
   ConstString function_name = symctx.GetFunctionName();
   ModuleSP module_sp = symctx.module_sp;
   if (!module_sp)
     return StackFrameRecognizerSP();
   ConstString module_name = module_sp->GetFileSpec().GetFilename();
   Symbol *symbol = symctx.symbol;
   if (!symbol)
     return StackFrameRecognizerSP();
   Address start_addr = symbol->GetAddress();
   Address current_addr = frame->GetFrameCodeAddress();
 
   for (auto entry : m_recognizers) {
-    if (entry.deleted)
-      continue;
     if (entry.module)
       if (entry.module != module_name)
         continue;
 
     if (entry.module_regexp)
       if (!entry.module_regexp->Execute(module_name.GetStringRef()))
         continue;
 

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

def test_frame_recognizer_1(self):
             "frame recognizer list",
             substrs=[
                 '1: recognizer.MyOtherFrameRecognizer, module a.out, symbol bar (regexp)',
                 '0: recognizer.MyFrameRecognizer, module a.out, symbol foo'
             ])
 
         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")
 
         self.expect("frame recognizer list",
                     substrs=['no matching results found.'])
 
         self.runCmd("frame recognizer add -l recognizer.MyFrameRecognizer -s a.out -n foo")