[Symbol Names] Workaround: Mangle names for macOS#177
[Symbol Names] Workaround: Mangle names for macOS#177vgvassilev merged 1 commit intocompiler-research:mainfrom
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #177 +/- ##
=======================================
Coverage 77.55% 77.56%
=======================================
Files 8 8
Lines 2887 2897 +10
=======================================
+ Hits 2239 2247 +8
- Misses 648 650 +2
|
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@vgvassilev would this work? |
| @@ -211,15 +211,24 @@ getSymbolAddress(clang::Interpreter& I, llvm::StringRef IRName) { | |||
| inline llvm::Expected<llvm::JITTargetAddress> | |||
| getSymbolAddress(clang::Interpreter& I, clang::GlobalDecl GD) { | |||
| std::string MangledName; | |||
There was a problem hiding this comment.
We can't rely on __APPLE__ here since we might be running on OSX but using remote execution and the underlying platform to be different.
How about something like:
auto &DL = getExecutionEngine()->getDataLayout();
char GlobalPrefix = DL.getGlobalPrefix();
bool HasGlobalPrefix = (GlobalPrefix != '\0');
if (HasGlobalPrefix && (*Name).front() != GlobalPrefix)
// Complain?
std::string Tmp((*MangledName).data() + HasGlobalPrefix,
(*MangledName).size() - HasGlobalPrefix);
getSymbolAddress(Tmp);There was a problem hiding this comment.
MangledName should be behavior agnostic, if it has the prefix then we cannot guess if it's actually the name of the symbol or if it was intended for prefix i.e. it creates ambiguity. Which is why I propose that we handle the prefix on API's end. Would that be okay?
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
clang-tidy review says "All clean, LGTM! 👍" |
lib/Interpreter/Compatibility.h
Outdated
| auto& DL = getExecutionEngine(I)->getDataLayout(); | ||
| char GlobalPrefix = DL.getGlobalPrefix(); | ||
| std::string LinkerName = std::string(1, GlobalPrefix) + LinkerName_.str(); | ||
| auto AddrOrErr = I.getSymbolAddressFromLinkerName(LinkerName); |
There was a problem hiding this comment.
Can we sink this code in getSymbolAddressFromLinkerName?
There was a problem hiding this comment.
We do have it there
There was a problem hiding this comment.
Can we delete the new code then?
There was a problem hiding this comment.
Can we delete the new code then?
We need to have it in the upstream version or the above location. Otherwise it fails.
There was a problem hiding this comment.
I am confused then. Why this code cannot go in getSymbolAddressFromLinkerName?
There was a problem hiding this comment.
getSymbolAddressFromLinkerName
Because it goes to clang-repl's API llvm::Expected<llvm::orc::ExecutorAddr> Interpreter::getSymbolAddressFromLinkerName(llvm::StringRef Name)
There was a problem hiding this comment.
LibInterOp has compat function inline llvm::Expected<llvm::JITTargetAddress> getSymbolAddressFromLinkerName(clang::Interpreter& I, llvm::StringRef LinkerName)
There was a problem hiding this comment.
So, you are saying it needs to go in llvm upstream if we want it close to the API... Ok, understood.
|
Just thought I'd add a comment that I tested this PR on an osx arm machine with Clang-REPL on, with a patched clang 16, and it passes all the tests for CppInterOp. |
|
clang-tidy review says "All clean, LGTM! 👍" |
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
LGTM! This feature is covering OSX and codecoverage cannot detect it being tested but that's ok. Could you squash the commits into one?
- Prepend `_` before looking for symbols by default - Use compat::getExecutionEngine for data layout and then get Global Prefix Signed-off-by: Shreyas Atre <shreyasatre16@gmail.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
3 participants
_before looking for symbols