Skip to content

Added clarifying comment to 'LLVMLinkInMCJIT' and 'LLVMLinkInInterpreter' #92467

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

Merged
merged 3 commits into from
Jun 18, 2025

Conversation

Minding000
Copy link
Contributor

@Minding000 Minding000 commented May 16, 2024

Issue

  • The purpose of LLVMLinkInMCJIT and LLVMLinkInInterpreter is not obvious (especially when using a pre-built library)
  • Many projects call these functions when using pre-built LLVM libraries

Changes

  • Added clarifying comment to LLVMLinkInMCJIT and LLVMLinkInInterpreter

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@Minding000 Minding000 force-pushed the document/link-functions branch 2 times, most recently from 36f3f90 to 697b826 Compare May 20, 2024 10:39
@Minding000
Copy link
Contributor Author

@bob-wilson Review and merge would be appreciated :)

@bob-wilson
Copy link
Contributor

@bob-wilson Review and merge would be appreciated :)

I'm not the right person to do that. I don't regularly work on LLVM and don't even have write access at this point.

@Minding000
Copy link
Contributor Author

@lhames I'm looking for a reviewer with write access

@lhames
Copy link
Contributor

lhames commented May 7, 2025

@Minding000 I assume you're not seeing any bugs from this, you just want to document that these are noops when linking against LLVM shared libraries?

Documenting that they're no-ops seems ok, but we could also give them hidden visibility so that they just don't show up in the dylib interface at all. The only drawback there would be for people who want to switch back and forward between dylib and archive builds of LLVM: they'd have to conditionalize their calls to LLVMLinkInMCJIT and LLVMLinkInInterpreter.

@Minding000
Copy link
Contributor Author

@lhames Yes, there are no bugs, just confusion.

I like the idea of hiding these functions in the dylib interface, as they don't serve a purpose there, but I don't know how to do that. I'm open to commits or hints though.

I guess it depends on how common the usecase of switching between static and dynamic libraries is. To me it sounds like a niche usecase and the workaround is pretty humane. But for these cases this would be a breaking change.

@Minding000 Minding000 force-pushed the document/link-functions branch from 697b826 to d0cb5b2 Compare May 15, 2025 23:53
@Minding000
Copy link
Contributor Author

@lhames I changed the visibility to hidden. Feel free to merge if it looks good to you

@lhames
Copy link
Contributor

lhames commented Jun 5, 2025

Hi @Minding000. Sorry that I did not respond to your message earlier!

On reflection I think we should go with your original solution and just document the existing behavior: These functions are going to be deprecated and removed in the not too distant future anyway, at which point the problem goes away. In the mean time hiding them from the dylib interface would require clients to call them conditionally based on how they're linking LLVM, which seems like it'll be more error-prone (e.g. when people copy/paste example code they find that assumes dynamic linking).

If you're happy to restore your original comments I can go ahead and merge them.

Sorry for the run-around!

@Minding000
Copy link
Contributor Author

@lhames No worries. I wasn't aware the functions were going to be deprecated. I reverted back to comments now - feel free to merge.

@lhames lhames merged commit 64155a3 into llvm:main Jun 18, 2025
6 of 11 checks passed
@lhames
Copy link
Contributor

lhames commented Jun 18, 2025

Thanks very much for that @Minding000, and for your patience with this review!

Copy link

@Minding000 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

fschlimb pushed a commit to fschlimb/llvm-project that referenced this pull request Jun 18, 2025
…ter' (llvm#92467)

Clarify that these functions are no-ops when linking to LLVM as a shared object.
@Minding000 Minding000 deleted the document/link-functions branch June 20, 2025 20:27
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