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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Target/AbortRecognizer.h:27
+
+  static llvm::Optional<std::tuple<FileSpec, ConstString>>
+  GetAbortLocation(Process *process_sp);
----------------
This doesn't really look much like a class with just two static member functions. Assuming that the `Process` is going to be the same for both, maybe store that as a member? Otherwise you might be better off having them as static functions in the implementation.


================
Comment at: lldb/include/lldb/Target/AbortRecognizer.h:33
+
+#pragma mark Frame recognizers
+
----------------
Although LLDB already has a lot of these markers, I don't think we want to add more of them. They're very specific to a single IDE available on a single platform :-) 


================
Comment at: lldb/include/lldb/Target/Thread.h:219
 
+  void ApplyMostRelevantFrames();
+
----------------
What does it mean to "apply" a frame? I think this needs a comment with some explanation or a better name. 


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:25
+
+llvm::Optional<std::tuple<FileSpec, ConstString>>
+AbortRecognizerHandler::GetAbortLocation(Process *process) {
----------------
Why does this need to return a `ConstString`? 


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:35
+  case llvm::Triple::MacOSX:
+    module_name = "libsystem_kernel.dylib";
+    symbol_name = "__pthread_kill";
----------------
Personally I'd return here: 
```
return std::make_tuple(FileSpec(module_name), symbol_name);
```
which would make it possible to...

 - get rid of the temporary `std::string`s, 
 - make the second element of the tuple just a StringRef. 


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:51
+
+llvm::Optional<std::tuple<FileSpec, ConstString>>
+AbortRecognizerHandler::GetAssertLocation(Process *process) {
----------------
Same comments as the previous method. 


================
Comment at: lldb/source/Target/AbortRecognizer.cpp:81
+    ThreadSP thread_sp, FileSpec module_spec, ConstString function_name) {
+  const uint32_t frames_to_fetch = 10;
+  StackFrameSP prev_frame_sp = nullptr;
----------------
Magic value? Why 10?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73303