[Lldb-commits] [PATCH] D73303: [lldb/Target] Add Assert StackFrame Recognizer

[Frederic Riss via Phabricator via lldb-commits]

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