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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to