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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
