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