Skip to content
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

Add the yk-linkage llvm pass. #62

Merged
merged 1 commit into from
Apr 21, 2023
Merged

Add the yk-linkage llvm pass. #62

merged 1 commit into from
Apr 21, 2023

Conversation

vext01
Copy link

@vext01 vext01 commented Apr 20, 2023

Compiler side fix for ykjit/yk#647.

Depends on ykjit/yk#650 and #61.

See commit message for a detailed description of what this does.

Associated PRs for yk and yklua coming soon.

(I wanted to test the name mangling aspect of this, but the llvm test suite isn't geared up for multi-object tests. They defer such tests to another repo, which I don't really want to fork and maintain. We can rely on yk and interpreter tests)

@ltratt
Copy link

ltratt commented Apr 20, 2023

Note that whilst symbols with internal linkage can have the same name and be
distinct, this is not so for symbols with external linkage. That's OK for
us because Yk requires the use of LTO, and the LTO module merger will have
already mangled the names for us so that symbol clashes can't occur.

I'm fine with this -- but I'm also starting to get a bit nervous about the quantity of things where we're subtly adding conditions above and beyond the normal. I think we should probably have a page in the docs along the lines of "yk details that might affect you" (not a good name, but hopefully you get the idea!) that documents these things. One day, we'll probably be grateful that there's a central record of these things. You could add such a page in this PR, and then flesh it out with the older quirky stuff we've done in another PR?

@vext01
Copy link
Author

vext01 commented Apr 20, 2023

You could add such a page in this PR

I think it would be better as a separate PR, as this is the compiler repo.

@ltratt
Copy link

ltratt commented Apr 20, 2023

Good point!

@vext01
Copy link
Author

vext01 commented Apr 20, 2023

Would this page be the one to expand? I can probably think of lots of other things to put here.

@ltratt
Copy link

ltratt commented Apr 20, 2023

Yes, that might be something to build upon. I think we would, at least, need two sections: "how yk changes the build process" and "how yk changes the language/run-time".

@vext01
Copy link
Author

vext01 commented Apr 20, 2023

Now that Lukas' PRs are done, can I rebase this?

@ltratt
Copy link

ltratt commented Apr 20, 2023

I don't think it needs rebasing.

bors r+

//===- Linkage.cpp - Ajdust linkage for the Yk JIT -----------------===//
//
// The JIT relies upon the use of `dlsym()` at runtime in order to lookup any
// given function from its virtual addresses. For this to work the symbols for
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: virtual address, not addresses

@vext01
Copy link
Author

vext01 commented Apr 20, 2023

bors r-

@bors
Copy link

bors bot commented Apr 20, 2023

Canceled.

@vext01
Copy link
Author

vext01 commented Apr 20, 2023

Fixed typo. OK to squash?

@ltratt
Copy link

ltratt commented Apr 20, 2023

Please squash.

The JIT relies upon the use of `dlsym()` at runtime in order to lookup
any given function from its virtual address. For this to work the
symbols for all functions must be in the dynamic symbol table.

`yk-config` already provides the `--export-dynamic` flag in order to
ensure that all *externally visible* symbols make it in to the dynamic
symbol table, but that's not enough: functions marked for internal
linkage (e.g. `static` functions in C) will be missed.

This pass walks the functions of a module and flips any with internal
linkage to external linkage.

Note that whilst symbols with internal linkage can have the same name
and be distinct, this is not so for symbols with external linkage.
That's OK for us because Yk requires the use of LTO, and the LTO module
merger will have already mangled the names for us so that symbol clashes
can't occur.
@vext01 vext01 force-pushed the internal-linkage branch from b6ac65b to 547f154 Compare April 20, 2023 13:09
@vext01
Copy link
Author

vext01 commented Apr 20, 2023

splat.

@ptersilie
Copy link

the LTO module merger will have already mangled the names for us so that symbol clashes can't occur

Well isn't that lucky!

@ltratt
Copy link

ltratt commented Apr 21, 2023

bors r+

@bors
Copy link

bors bot commented Apr 21, 2023

Build succeeded:

@bors bors bot merged commit 03da81d into ykjit:main Apr 21, 2023
@vext01 vext01 deleted the internal-linkage branch April 21, 2023 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants