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

[Orc] Add NotifyCreated callback for LLJITBuilder #84175

Merged

Conversation

weliveindetail
Copy link
Member

This is useful to attach generators to JITDylibs or inject initial symbol definitions.

@weliveindetail
Copy link
Member Author

It seems like there are no tests for LLJITBuilder so far.

@weliveindetail
Copy link
Member Author

Please note the differences to the existing callbacks SetupProcessSymbolsJITDylib and SetUpPlatform. They receive a reference to the LLJIT instance as well, but they both:

  • are called during construction, i.e. before the last point of failure
  • have a well-defined purpose that we would have to mimic
  • have a default definition, that is inaccessible via the interface (e.g. setUpGenericLLVMIRPlatform)

Copy link
Contributor

@lhames lhames left a comment

Choose a reason for hiding this comment

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

I think it makes sense for NotifyCreated to return Error rather than void so that the callback itself can fail. Otherwise this looks great -- thanks @weliveindetail!

Change the return type to allow Error results
@weliveindetail weliveindetail merged commit f78129e into llvm:main Mar 7, 2024
4 checks passed
@weliveindetail weliveindetail deleted the lljit-builder-notify-created branch March 7, 2024 22:05
joker-eph added a commit that referenced this pull request Mar 7, 2024

if (impl().NotifyCreated)
if (Error Err = impl().NotifyCreated(*J))
return Err;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know why the premerge CI didn't fail, but everywhere else in this function is return std::move(Err);

I made the change in 5d33f71

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like clang requires it but not gcc (which is used in premerge) nor MSVC.

You should have got a lot of buildbot emails though, didn't you?

Copy link
Member Author

Choose a reason for hiding this comment

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

I got a lot of buildbot emails, but they also came all day for all kinds of unrelated reasons... Anyway, sorry for the mess.

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