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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.


================
Comment at: lldb/include/lldb/Target/AbortRecognizer.h:29
+  GetAbortLocation(Process *process_sp);
+  static llvm::Optional<std::tuple<FileSpec, ConstString>>
+  GetAssertLocation(Process *process_sp);
----------------
This function (especially the return types) deserve some documentation.


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:1
+//===-- AbortRecognizer.cpp -------------------------------------*- C++ -*-===//
+//
----------------
Please remove the `-*- C++ -*-` as that's only for header files.


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:85
+  // Fetch most relevant frame
+  for (uint32_t i = 0; i < frames_to_fetch; i++) {
+    prev_frame_sp = thread_sp->GetStackFrameAtIndex(i);
----------------
Could we make this `frame_index` instead of just `I`. I was confused that `I` isn't just an index into a vector when you later interpret it as a stack frame index.


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:99
+    if (sym_ctx.module_sp->GetFileSpec().GetFilename() ==
+            module_spec.GetFilename() &&
+        sym_ctx.GetFunctionName() == function_name) {
----------------
You can just do `sym_ctx.module_sp->GetFileSpec().FileEquals(module_spec)` which also handles case insensitive file systems IIRC.


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:101
+        sym_ctx.GetFunctionName() == function_name) {
+      if (i < frames_to_fetch - 1)
+        m_most_relevant_frame = thread_sp->GetStackFrameAtIndex(i + 1);
----------------
Maybe add a comment why you need to skip one frame.


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:104
+      else
+        m_most_relevant_frame = thread_sp->GetStackFrameAtIndex(i);
+      break;
----------------
Maybe something like this? Just a suggestion though.

```
lang=c++
uint32_t last_frame_index = frames_to_fetch - 1;
m_most_relevant_frame = thread_sp->GetStackFrameAtIndex(std::min(i + 1, last_frame_index));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73303