-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multiversioning: support for aliases (from @ccallable) #37530
Conversation
I don't think this is correct. If the function is cloned then all uses of it, including global aliases must be updated. |
Yeah, this was a quick fix that happened to work 🙂 Don't we keep the original functions for aliases to point to? From my reading of |
Correct, it was not supported and doesn't have a trivial fix #21849 (review) . There are basically two options,
|
Also ref #21849 (comment) which is essentially about the second option above. |
I went with the custom trampoline + slot option, which seems to work nicely for the
|
1ba4a27
to
dc8afa0
Compare
8a47efb
to
d2e20ef
Compare
d2e20ef
to
8064e00
Compare
24f2816
to
b874a04
Compare
The way the information is collected is more or less OK I guess. There are still a few other issues though. The module merger doesn't seem to handle use of |
Any update, @yuyichao? I had hoped to get this in 1.6. |
Not before the build on master is unbroken... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b874a04
to
4f0fc5e
Compare
Rebased. @yuyichao, could you give this another look? @KristofferC Verified that this still fixes the issue (the MWE from #34064 (comment)). I did run into an LLVM assertion failure in some of the code added here, but can't reproduce that anymore; could you give this a quick spin to see if you can? |
I do in fact get an assertion with this PR:
Example repro (in PackageCompiler repo): using PackageCompiler
create_app("examples/MyApp/", "/tmp/MyAppCompiled";force=true, incremental=true, cpu_target="native")
run(`/tmp/MyAppCompiled/bin/MyApp`) |
784e919
to
c0af7e9
Compare
c0af7e9
to
85e2647
Compare
Verified to fix the linked PackageCompiler issue, so let's go ahead and merge this. |
(cherry picked from commit 4170090)
Fixes #34064, cc @KristofferC, verified with https://s3.amazonaws.com/julialangnightlies/assert_pretesting/linux/x64/1.6/julia-c2db4248a9-linux64.tar.gz (from #37510, which includes the change) where the MWE from #34064 works: