[Lldb-commits] [PATCH] D44603: [lldb] Introduce StackFrameRecognizer

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This seems like a good place to start.  Certainly producing synthetic function arguments is one of the fixed things these recognizers can do, and we can build on that as we go.  The inputs are all types that we know how to bind to Python, so it will be easy to make wrappers and eventually an affordance for making Python based frame recognizers.  I don't think that needs to be a part of this patch, though that would make writing tests easier.  How were you planning to add tests for this?



================
Comment at: include/lldb/Target/StackFrame.h:558-559
                                                  // m_variable_list_sp
+  bool m_recognized;
+  lldb::RecognizedStackFrameSP m_recognized_frame;
   StreamString m_disassembly;
----------------
Do we need m_recognized?  Wouldn't it be enough to check !m_recognized_frame?

Also, we prepend SharedPointers with _sp as a general rule since it's really handy to know that w/o having to look up the definition.


================
Comment at: source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCStackFrameRecognizer.cpp:27-28
+ public:
+  DarwinObjCExceptionRecognizedStackFrame(StackFrameSP frame_sp) {
+    ThreadSP thread_sp = frame_sp->GetThread();
+    ProcessSP process_sp = thread_sp->GetProcess();
----------------
In the rest of the Apple support for ObjC we use either AppleObjCRuntimeV1 and AppleObjCRuntimeV2, and then use AppleObjCRuntime for support that crosses versions.  It would be better not to introduce another name here.


================
Comment at: source/Target/StackFrameRecognizer.cpp:26-30
+  void AddRecognizer(StackFrameRecognizerSP recognizer, ConstString &module,
+                     ConstString &symbol, bool first_instruction_only) {
+    m_recognizers.push_back({recognizer, module, RegularExpressionSP(), symbol,
+                             RegularExpressionSP(), first_instruction_only});
+  }
----------------
Because of the way you search, you're doing first one in wins for any conflicts.  I'm not sure we need to overthink conflicts at this point, but at least it should be last one in wins.  And maybe start out returning an Status so that if there are conflicts we can warn about them?


https://reviews.llvm.org/D44603