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

spectest: ensure compiled functions can outlive module #2105

Closed
wants to merge 2 commits into from
Closed

spectest: ensure compiled functions can outlive module #2105

wants to merge 2 commits into from

Conversation

ncruces
Copy link
Collaborator

@ncruces ncruces commented Mar 1, 2024

Tentative fix for #2039 for the compiler, not wazevo.

In linking.wast we have:

  • a module A that exports its memory and table
  • a module B that imports both
    • it initializes the (shared) memory, and
    • it initializes the (shared) table, setting entry zero to one of its functions
    • then it fails while running main

The test then verifies that initializing the memory and the table both succeeded before main failed. It does so by reading from the memory, and indirect calling the function in the table.

Both the memory and table are owned by A, so still exist. But the function, or rather the compiled code (and mmaped memory) for the function is/was owned by B and can be GCed, because we SetFinalizer on an object owned by B.

This change makes it so that the finalizer is set on the compiledCode rather than on the compiledModule. Since the resolved function import is a function, and that owns a compiledFunction, which owns the compiledCode, this should fix the issue.

Signed-off-by: Nuno Cruces <[email protected]>
@ncruces ncruces requested a review from achille-roussel March 1, 2024 00:44
Copy link
Collaborator

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

The change looks good, do you want to merge this in independently of #2102 ?

@ncruces
Copy link
Collaborator Author

ncruces commented Mar 1, 2024

The change looks good, do you want to merge this in independently of #2102 ?

I'd reviewed a previous version of that, and (I guess because the title was not updated) totally missed the progress.
Then went on and duplicate all the work. 🤦

Happy to shelve this.

@ncruces
Copy link
Collaborator Author

ncruces commented Mar 1, 2024

Everything is already in #2102.

@ncruces ncruces closed this Mar 1, 2024
@ncruces ncruces deleted the finalizer branch March 24, 2024 10:14
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.

2 participants