[PATCH] D117173: [ORC-RT] Hook objc's class_getImageName for JITDylibs

[Lang Hames via Phabricator via llvm-commits]

lhames added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

Sorry for the delayed review on this one.

The switch to allocation actions will have broken this -- I'll try to port it to top-of-tree tomorrow.



================
Comment at: compiler-rt/lib/orc/executor_address.h:196-206
+    auto I = Map.lower_bound({Addr, Addr});
+    if (I != end() && I->first.contains(Addr))
+      return I;
+    // Since we search based on the start of the range, we need to check the
+    // previous element as well.
+    if (I != begin()) {
+      --I;
----------------
What about using upper-bound here? It has the advantage that you only need to check one range, though you do have to special-case the empty map.

```
if (Map.empty())
  return Map.end();
auto I = std::prev(Map.upper_bound({Addr, Addr}));
if (I->first.contains(Addr))
  return I;
return Map.end();
```



================
Comment at: compiler-rt/lib/orc/macho_platform.cpp:372
+    };
+    ORC_RT_DEBUG(printdbg("objc_setHook_getImageName not available; "
+                          "class_getImageName will not work for JITDylibs"));
----------------
benlangmuir wrote:
> I wasn't 100% sure whether this should be a log message or an error.  The other objc functionality uses errors, but in those cases the objc code will fail completely if you're missing the runtime functions. In this case, `class_getImageName` returning nullptr doesn't seem bad enough to make it a hard error to me -- you might not be using that at all.  It's too bad there's no such thing as a runtime warning.
For now I think we debug-log when we see objc_setHook_getImageName missing, and hard-error in the hook. Especially in early development I'd rather fail loudly wherever we deviate from AOT behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117173