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

[Frederic Riss via Phabricator via lldb-commits]

friss added inline comments.


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:126-127
+  // Fetch abort location
+  auto abort_location =
+      AbortRecognizerHandler::GetAbortLocation(process_sp.get());
+
----------------
Why do you even check the abort location? The recognizer has been registered for this specific module and function, so you're never going to get here with a different top frame.


================
Comment at: lldb/source/Target/Process.cpp:543
+
+  RegisterAbortRecognizer(this);
 }
----------------
We need to find a better place to register this.


================
Comment at: lldb/source/Target/Process.cpp:945
+
+          uint32_t mrf_idx = 0;
+
----------------
This variable name just looks weird. I know what it is in the context of this patch, but I would never get it re-reading this code a year from now. Just hoist the `start_frame` variable from bellow and use it.


================
Comment at: lldb/source/Target/Process.cpp:947
+
+          auto frame_sp = cur_thread->GetStackFrameAtIndex(mrf_idx);
+
----------------
No point in using the variable here. You want the top frame, let the code reflect this and just use 0.


================
Comment at: lldb/source/Target/Process.cpp:956
           const bool only_threads_with_stop_reason = true;
-          const uint32_t start_frame = 0;
+          const uint32_t start_frame = mrf_idx;
           const uint32_t num_frames = 1;
----------------
Huh, reading further down the patch, it feels pretty wrong that you have to compute the "most relevant frame" here and in `WillStop()`. Should this just be something like `start_frame = cur_thread->GetSelectedFrame()->GetIndex()`?


================
Comment at: lldb/source/Target/Thread.cpp:576
 
+void Thread::ApplyMostRelevantFrames() {
+  if (!m_curr_frames_sp)
----------------
Why does this function name use a plural form? Wouldn't `SelectMostRelevantFrame` be a more explicit name?


================
Comment at: lldb/source/Target/Thread.cpp:581
+  auto cur_frame = m_curr_frames_sp->GetFrameAtIndex(0);
+  SymbolContext sym_ctx = cur_frame->GetSymbolContext(eSymbolContextEverything);
+
----------------
Unused variable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73303