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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

Thanks Jim. This LGTM.



================
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);
----------------
jingham wrote:
> JDevlieghere wrote:
> > 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? 
> On the systems that don't use stret returns, class_getMethodImplementation_stret doesn't exist.  So we would either have to have two versions of that part of the code, or passing function pointers to the two functions (making them the same in the no _stret case) or do some dlsym type ugliness.
> 
> This solution is straightforward, and reducing complexity in these Utility functions is a virtue.  Having two copies of all the code was a bit bogus, but this solution is straightforward and easy to debug.
Thanks. I saw the `m_impl_stret_fn_addr` and figured we were passing in the function pointer ourselves. Makes sense. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123421