[Lldb-commits] [PATCH] D123421: Handle the new "selector-specific stub" ObjC Method dispatch method

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:52-53
+    return_struct.impl_addr =
+    class_getMethodImplementation_stret (return_struct.class_addr,
+                                             return_struct.sel_addr);
+  } else {
----------------
Nit: Seems like the formatting is a bit off. I would copy/paste this code in a separate file, run clang-format over it, and paste it back. 


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:637-643
     // Also we will use the version of the lookup code that doesn't rely on the
     // stret version of the function.
-    m_lookup_implementation_function_code =
-        g_lookup_implementation_no_stret_function_code;
+    m_lookup_implementation_function_code.append(
+        g_lookup_implementation_no_stret_function_code);
   } else {
-    m_lookup_implementation_function_code =
-        g_lookup_implementation_with_stret_function_code;
+    m_lookup_implementation_function_code.append(
+        g_lookup_implementation_with_stret_function_code);
----------------
I don't understand why we need to have two versions of the code. IIUC, if there's a "stret return lookup function", then if we pass `is_stret == true` we'll use it. Otherwise we'll unconditionally use the straight lookup. Why do we need two variants of the code at all? Can't we pass false to `is_stret` and achieve the same result with the `g_lookup_implementation_with_stret_function_code` variant? 


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:816-818
+    if (curr_sym) {
+      sym_name = curr_sym->GetName().GetStringRef();
+    }
----------------
Nit: for consistency with the line below


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:1012-1015
+        if (log) {
+          LLDB_LOG(log, "Resolving call for class - {0} and selector - {1}",
+                   isa_addr, sel_addr);
+        }
----------------
The `if (log)` check is redundant. It's the purpose of the macro to expand to that that. Same below.


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp:1074
+        if (sel_str_addr == LLDB_INVALID_ADDRESS || error.Fail()) {
+          if (log)
+            LLDB_LOG(log,
----------------
redundant


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.h:55-65
+  // These hold the code for the function that finds the implementation of
+  // an ObjC message send given the class & selector and the kind of dispatch.
+  // There are two variants depending on whether the platform uses a separate
+  // _stret passing convention (e.g. Intel) or not (e.g. ARM).  The difference
+  // is only at the very end of the function, so the code is broken into the
+  // common prefix and the suffix, which get composed appropriately before
+  // the function gets compiled.
----------------
Nit: I think these should be doxygen comments, so ///. You can group them like this:


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.cpp:82
+  Log *log = GetLog(LLDBLog::Step);
+  if (log) {
+    LLDB_LOG(log, "Caching: class {0} selector {1} implementation {2}.",
----------------
redundant


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h:404-407
+      if (class_addr == rhs.class_addr && sel_name == rhs.sel_name)
+        return true;
+      else
+        return false;
----------------
```
return class_addr == rhs.class_addr && sel_name == rhs.sel_name;
```


================
Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h:411-417
+      if (class_addr < rhs.class_addr)
+        return true;
+      else if (class_addr > rhs.class_addr)
+        return false;
+      else {
+        return ConstString::Compare(sel_name, rhs.sel_name);
+      }
----------------
```
      if (class_addr < rhs.class_addr)
        return true;
      if (class_addr > rhs.class_addr)
        return false;
      return ConstString::Compare(sel_name, rhs.sel_name);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123421