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 fiber safety to object class getter/property macros #15387

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Jan 28, 2025

closes #14905

Builds on top of the prerequisite refactors/features. Only the last commit actually implements the feature.

@ysbaddaden
Copy link
Contributor Author

Damn. The interpreter is still broken on windows.

@ysbaddaden
Copy link
Contributor Author

I'm using the released 1.15.0 compiler for windows. I'm running crystal i hello.cr (which is a mere put "hello") and it reproduces on windows (segfault when calling memmove). Of course this is working nicely on linux.

The interpreter on windows can't reach Crystal.init_runtime. If I call LibC._exit before the prelude loads string.cr then it's working.

@ysbaddaden
Copy link
Contributor Author

Yeah, if I call Crystal.init_runtime in src/crystal/once.cr, then there are no issues.

@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Jan 30, 2025

Gotcha 🐛

The interpreter requires the prelude which in turn:

  • require "string"
  • require "string/builder"
  • require "io"
  • require "io/file_descriptor"
  • require "crystal/system/win32/file_descriptor"
  • at_exit inlined calls
  • Crystal::AtExitHandlers#class_getter(handlers) { ... } 💥
  • (...)
  • require "crystal/main"
  • Crystal::Once.init

I guess we should load crystal/main sooner in the prelude.

…l-lang#15386)

Generates the macros from an external script that will inline the actual
macro implementations, instead of using macros to generate the macros
and duplicating the actual implementation multiple times.
The interpreter on windows segfaults because `Crystal::Once.init` wasn't
called soon enough:

- require "string"
- require "string/builder"
- require "io"
- require "io/file_descriptor"
- require "crystal/system/win32/file_descriptor"
- at_exit <== inlined call!
- Crystal::AtExitHandlers#class_getter(handlers) { ... } <== calls Crystal.once!
- (...)
- require "crystal/main"
- Crystal.init_runtime
- Crystal::Once.init <== only initialized now!

Requiring crystal/main sooner makes sure that the interpreter will
execute `Crystal.init_runtime` before any other inlined call that may
depend on `Crystal.once`.
@ysbaddaden ysbaddaden force-pushed the feature/add-fiber-safety-to-object-property-macros branch from b057f50 to 53e1d87 Compare February 7, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

Ensuring class_getter runs exactly once under concurrent access
1 participant