friss added a comment.
This generally looks good. I'm still not fond of registering this in the Target
itself. But I don't have a immediately better idea as we don't have a C
language runtime.
A couple more comments/questions that can be addressed as followups:
================
Comment at: lldb/include/lldb/Target/StackFrameRecognizer.h:38
+ virtual lldb::StackFrameSP GetMostRelevantFrame() { return nullptr; };
+ virtual llvm::StringRef GetStopDescription() { return ""; }
virtual ~RecognizedStackFrame(){};
----------------
The fact that this returns a StringRef, means it must return a pointer to
permanent string storage. In this case, you're returning a constant string, and
it's fine, but it means that if you wanted to construct a return value (for
example out of data you extract from the inferior), you need to store the
string backing the StringRef somewhere. The concrete `RecognizedStackFrame`
instance seems like a good fit, but I have no clue about their lifetime
guarantees compared to the lifetime of the StringRef returned here. Maybe we
should make this a `std::string`?
================
Comment at:
lldb/packages/Python/lldbsuite/test/functionalities/inferior-assert/TestInferiorAssert.py:113
+
+ stop_reason = 'stop reason = ' + thread.GetStopDescription(256)
----------------
Here, you're now just checking self consistency, I son't think that's valuable.
This should check for the actual text of the assert recognizer (and potentially
something else on the systems where the recognizer is not supported).
================
Comment at:
lldb/packages/Python/lldbsuite/test/functionalities/inferior-assert/TestInferiorAssert.py:191
+
+ self.assertTrue(thread.GetFrameAtIndex(0) == frame, "Frame #0
selected")
+
----------------
as we just did `self.runCmd` above anyway, I'd replace all of this by
`self.runCmd('frame select 0')`. Or at least just write it as a single line, no
need to check for every intermediate result. Such verbosity really distracts
from what the test is trying to verify.
================
Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:110-112
+AssertRecognizedStackFrame::AssertRecognizedStackFrame(
+ StackFrameSP most_relevant_frame_sp)
+ : m_most_relevant_frame(most_relevant_frame_sp) {}
----------------
Can you move this with the other AssertRecognizedStackFrame methods?
================
Comment at: lldb/source/Target/StackFrameRecognizer.cpp:95
const SymbolContext &symctx =
- frame->GetSymbolContext(eSymbolContextModule | eSymbolContextFunction);
+ frame->GetSymbolContext(eSymbolContextEverything);
ConstString function_name = symctx.GetFunctionName();
----------------
What's the reason of this change?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73303/new/
https://reviews.llvm.org/D73303
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
