This is an archive of the discontinued LLVM Phabricator instance.

[ORC] Introduce RedirectionManager interface and implementation using JITLink.
Needs ReviewPublic

Authored by sunho on Aug 14 2023, 5:14 AM.

Details

Summary

This patch introduces RedirectionManager and RedirectableSymbolManager interfaces which abstracts the redirection of call to certain symbols. It also adds JITLink implmenetation of such interface along with testcases covering basic use cases.

Diff Detail

Event Timeline

sunho created this revision.Aug 14 2023, 5:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 5:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
sunho requested review of this revision.Aug 14 2023, 5:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 5:14 AM
lkail added a subscriber: lkail.Aug 14 2023, 5:33 AM
lhames added inline comments.Aug 15 2023, 9:22 PM
llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h
59

Can we remove the JD argument here? The ResourceTracker argument to createRedirectableSymbols tells us the target JITDylib already, and this would allow us to have one global JITLinkRediretableSymbolManager, rather than one per JITDylib.

100–101

Why a separate StringSaver here, rather than using the SymbolStringPool?

llvm/include/llvm/ExecutionEngine/Orc/RedirectionManager.h
32–34

I think this should be non-virtual for now. We can make it virtual later if we find a use-case where optimizing the single-symbol case is important.

54

Similarly here.

sunho added inline comments.Aug 16 2023, 5:51 AM
llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h
59

We need this JD since we are going to generate a stub pool inside this JD. Alternative is manage stub pool per JD within single manager, how does this sound to you?

100–101

We need to get actual string from SymbolStringPtr. Is there a way to do this?

llvm/include/llvm/ExecutionEngine/Orc/RedirectionManager.h
32–34

👍

sunho added inline comments.Aug 16 2023, 5:55 AM
llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h
100–101

Oh actually this was needed since JITLink isn't based on SymbolStringPtr yet. We still need to save the string cause JITLink assumes all the StringRefs are properly owned by external object like symbol string table.

lhames added inline comments.Aug 16 2023, 1:34 PM
llvm/include/llvm/ExecutionEngine/Orc/JITLinkRedirectableSymbolManager.h
59

You can get the target JITDylib from the tracker, so the aim should be to have one manager for all JITDylibs. In this case the createRedirectableSymbols method should probably be changed to a set of functions:

virtual Error createRedirectableSymbols(ResourceTrackerSP RT, const SymbolAddrMap &InitialDests) = 0;

Error createRedirectableSymbol(JITDylib &JD, const SymbolAddrMap &InitialDests) {
  return createRedirectableSymbol(JD.getDefaultResourceTracker(), InitialDests);
}

Error createRedirectableSymbol(
    ResourceTracker RT, SymbolStringPtr Symbol, ExecutorSymbolDef InitialDest) {
  return createRedirectableSymbols(std::move(RT), {{Symbol, InitialDest}});
}

Error createRedirectableSymbol(
    JITDylib &JD, SymbolStringPtr Symbol, ExecutorSymbolDef InitialDest) {
  return createRedirectableSymbol(JD.getDefaultResourceTracker(), std::move(Symbol), InitialDest);
}
100–101

You can get a StringRef for a SymbolStringPtr using the dereference operator, and that should be safe to use in a LinkGraph.