[Lldb-commits] [PATCH] D76188: [lldb/Target] Support more than 2 symbols in StackFrameRecognizer

Frederic Riss via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Sat Mar 14 20:26:22 PDT 2020


friss added inline comments.


================
Comment at: lldb/include/lldb/Target/StackFrameRecognizer.h:115
       std::function<void(uint32_t recognizer_id, std::string recognizer_name,
-                         std::string module, std::string symbol,
-                         std::string alternate_symbol, bool regexp)> const
-          &callback);
+                         std::string module, std::vector<ConstString> &symbols,
+                         bool regexp)> const &callback);
----------------
can this be a const vector, or even better an ArrayRef? We certainly don't want the callbacks to modify the list? 


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:749
       case 'n':
-        m_function = std::string(option_arg);
+        m_symbols.push_back(std::string(option_arg));
         break;
----------------
Does the command actually accept multiple symbols names now? If yes, it should be tested and a warning should be added when you use the regex option and you pass multiple -n options. If it doesn't accept multiple names, it shouldn't store a vector.


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:859
+    result.AppendErrorWithFormat(
+        "%s needs at lease one symbol name (-n argument).\n",
+        m_cmd_name.c_str());
----------------
*at least


================
Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:145
 
-    if (func_name == location.symbol_name ||
-        (!location.alternate_symbol_name.IsEmpty() &&
-         func_name == location.alternate_symbol_name)) {
-
+    if (location.HasSymbol(func_name)) {
       // We go a frame beyond the assert location because the most relevant
----------------
This reads a little weird to me. Should we call it MatchesSymbol instead?


================
Comment at: lldb/source/Target/StackFrameRecognizer.cpp:64-72
+    m_recognizers.push_front({(uint32_t)m_recognizers.size(),
+                              false,
+                              recognizer,
+                              true,
+                              ConstString(),
+                              module,
+                              {},
----------------
Is this really the clang-format formatting?


================
Comment at: lldb/source/Target/StackFrameRecognizer.cpp:89-90
 
+        entry.symbols.clear();
+        entry.symbols.push_back(ConstString(symbol_name));
+
----------------
It's kinda weird to do this here, and I also think it's wrong as it will leave the recognizer with an entry in its symbols list which changes its semantics. If the callback was taking an ArrayRef, it would be easy (and cheap) to create one for a single element as well as from a vector.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76188/new/

https://reviews.llvm.org/D76188





More information about the lldb-commits mailing list