[Lldb-commits] [PATCH] D73225: Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:786
+
+      m_opt_dispatch_map.insert(std::pair<lldb::addr_t, int>(sym_addr, i));
+    }
----------------
You could simplify this by using `.emplace(sym_addr, i)`. 


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:889
+  pos = m_msgSend_map.find(addr);
+  if (pos != m_msgSend_map.end()) {
+    return_ptr = &g_dispatch_functions[(*pos).second];
----------------
If you rewrite this you can get rid of the return_ptr temporary. 

```
if (pos != m_msgSend_map.end()) {
    return &g_dispatch_functions[(*pos).second];
}
return nullptr;
```


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:918
 
-  MsgsendMap::iterator pos;
-  pos = m_msgSend_map.find(curr_pc);
-  if (pos != m_msgSend_map.end()) {
-    this_dispatch = g_dispatch_functions[(*pos).second];
-    found_it = true;
-  }
-
+  this_dispatch = FindDispatchFunction(curr_pc);
+  
----------------
Can you initialize this on line 910?


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:1200
+  if (!this_dispatch && !ret_plan_sp) {
+    const char *opt_name = nullptr;
+    MsgsendMap::iterator pos;
----------------
Same here, why not do the assignment when you declare the variables?


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:202
+
+// objc uses optimized dispatch functions for some common and seldom overridden
+// methods.  For instance 
----------------
`/// Objective-C ...`


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:317
+  else
+    stop_reason = stop_info_sp->GetStopReason();
+
----------------
```
StopReason stop_reason = stop_info_sp ? stop_info_sp->GetStopReason() : eStopReasonNone;
```


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:320
+  // See if this is one of our msgSend breakpoints:
+  if (stop_reason == eStopReasonBreakpoint) {
+    ProcessSP process_sp = GetThread().GetProcess();
----------------
I'd invert the condition and make this an early return with the comment below. 


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:358
+bool AppleThreadPlanStepThroughDirectDispatch::ShouldStop(Event *event_ptr) {
+
+  // If step out plan finished, that means we didn't find our way into a method 
----------------
Spurious empty line


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp:364
+  bool step_out_should_stop = ThreadPlanStepOut::ShouldStop(event_ptr);
+  if (step_out_should_stop) {
+    SetPlanComplete(true);
----------------
Looks like `step_out_should_stop ` isn't used anywhere else? 

```if(ThreadPlanStepOut::ShouldStop(event_ptr)) {```


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:67
   lldb::addr_t m_sel_addr;
   lldb::ThreadPlanSP m_func_sp; // This is the function call plan.  We fill it
+                                // at start, then set it to NULL when this plan
----------------
These should be Doxygen comments before the variable if you're already touching them...

```
/// This is the function call plan.  We fill it at the start, then ...
lldb::ThreadPlanSP m_func_sp;
```


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:77
 
+class AppleThreadPlanStepThroughDirectDispatch 
+    : public ThreadPlanStepOut {
----------------
Can you please clang-format the patch, this should fit on one line.


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:93
+
+  // The base class MischiefManaged does some cleanup - so you have to call it
+  // in your MischiefManaged derived class.
----------------
`///` 


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h:110
+  std::vector<lldb::BreakpointSP> m_msgSend_bkpts;
+  bool m_at_msg_send = false;
+  bool m_stop_others;
----------------
Let's initialize this in the ctor as well, the inconsistency with the next line leaves me wondering if this is a bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73225