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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Being able to match multiple symbols sounds useful in its own right, but the motivating case is a bit shady. `raise`, `__GI_raise` and `gsignal` are all aliases to one another (they have the same address). The reason you're sometimes getting `gsignal` here is not because some glibcs really call this function from their assert macro. It's because we happen to pick that symbol (maybe because it comes first in the symtab, depending on how the library was linked) when doing address resolution.

I'm wondering if that doesn't signal a flaw in the recognizer infrastructure. If we changed the matching logic so that it resolves the name it is supposed to search for, and then does a match by address, then only one name would be sufficient here.



================
Comment at: lldb/include/lldb/Target/StackFrameRecognizer.h:105
+                            ConstString module,
+                            std::vector<ConstString> symbols,
                             bool first_instruction_only = true);
----------------
Using std::vector might be good here as the function "consumes" the argument (by assigning it to the internal recognizer list. But in that case you ought to use std::move, as appropriate to prevent needless copying. If you don't want to optimize things that much, maybe just use ArrayRef here too?


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:880
     auto func =
-        RegularExpressionSP(new RegularExpression(m_options.m_function));
+        RegularExpressionSP(new RegularExpression(m_options.m_symbols.front()));
     StackFrameRecognizerManager::AddRecognizer(recognizer_sp, module, func);
----------------
Is there something which ensure that m_symbols contains at least one element here? (i.e., that we do not silently drop the extra symbols arguments)


================
Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:27
+  bool MatchesSymbol(ConstString symbol) {
+    return std::find(symbols.begin(), symbols.end(), symbol) != symbols.end();
+  }
----------------
`llvm::is_contained(symbols, symbol)`


================
Comment at: lldb/source/Target/StackFrameRecognizer.cpp:64-72
+    m_recognizers.push_front({(uint32_t)m_recognizers.size(),
+                              false,
+                              recognizer,
+                              true,
+                              ConstString(),
+                              module,
+                              {},
----------------
mib wrote:
> friss wrote:
> > Is this really the clang-format formatting?
> It is but I'll change it back to be more compact.
instead of arguing with clang-format, you could change this to `m_recognizers.emplace_front(arguments, without, braces)`, which will be more efficient, and probably result in better formatting.


================
Comment at: lldb/source/Target/StackFrameRecognizer.cpp:125-126
+      if (!entry.symbols.empty())
+        if (std::find(entry.symbols.begin(), entry.symbols.end(),
+                      function_name) == entry.symbols.end())
           continue;
----------------
is_contained


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76188