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

Fix TSModule tracking. #494

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Fix TSModule tracking. #494

merged 1 commit into from
Dec 12, 2024

Conversation

maleadt
Copy link
Collaborator

@maleadt maleadt commented Dec 12, 2024

This should be NFC, but lets see if CI still fails.

@maleadt
Copy link
Collaborator Author

maleadt commented Dec 12, 2024

If this doesn't help, we may have to change how we handle contexts / thread-safe contexts. It's currently possibly to have a context() not matching the ts_context(), which seems like it could result in nasty errors. I wonder if we should simply make it so that ts_context() always returns a (uniqued) thread-safe version of the current context.

@maleadt
Copy link
Collaborator Author

maleadt commented Dec 12, 2024

I wonder if we should simply make it so that ts_context() always returns a (uniqued) thread-safe version of the current context.

That doesn't work, the ThreadSafeContext ctor taking a Context transfers ownership...

Switching everything around and only using ThreadSafeContexts in LLVM.jl would be possible, but that's a pretty invasive change. It would also make it impossible to take a foreign context and work with that, because once again we can't create a TSCtx wrapping it.

Since we use simple structs, the same handle results
in the same object object, triggering user-after-free warnings.
@maleadt maleadt changed the title Simplify TSModule construction. Fix TSModule tracking. Dec 12, 2024
@maleadt maleadt merged commit a4ed387 into master Dec 12, 2024
19 of 22 checks passed
@maleadt maleadt deleted the ts_context branch December 12, 2024 09:54
maleadt referenced this pull request Jan 10, 2025
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.

1 participant