Skip to content

Build compiler with -Dexecution_context on Windows [fixup #16447]#16502

Merged
straight-shoota merged 2 commits intocrystal-lang:masterfrom
straight-shoota:infra/compiler-execution_context-windows
Dec 23, 2025
Merged

Build compiler with -Dexecution_context on Windows [fixup #16447]#16502
straight-shoota merged 2 commits intocrystal-lang:masterfrom
straight-shoota:infra/compiler-execution_context-windows

Conversation

@straight-shoota
Copy link
Member

In #16447 we forgot to add the Makefile change to Makefile.win as well.

@ysbaddaden ysbaddaden added this to the 1.19.0 milestone Dec 12, 2025
@straight-shoota
Copy link
Member Author

This breaks the interpreter on Windows: https://github.com/crystal-lang/crystal/actions/runs/20170611171/job/57926651383

We probably need to pay special attention to the interpreter, because we spawn fibers from the interpreted program in the interpeter itself. With -Dexecution_context -Dpreview_mt this means all fibers in the interpreter run in a parallel context!
So it's actually surprising that we don't see any problems on other platforms...

@ysbaddaden
Copy link
Collaborator

ysbaddaden commented Dec 13, 2025

Good point. We shouldn't resize the default context and instead create a dedicated parallel context for codegen only.

Or create a concurrent context for running the interpreted code.

Or both.

@ysbaddaden
Copy link
Collaborator

ysbaddaden commented Dec 13, 2025

What's weird is that preview_mt worked (oh, no stealing) 🤨

@straight-shoota straight-shoota removed this from the 1.19.0 milestone Dec 13, 2025
straight-shoota added a commit that referenced this pull request Dec 14, 2025
… the same time (#16503)

When changing one of these files, we're often forgetting to change the other (latest example: #16502). Most changes affect both, so I think it makes sense to add this reminder.
In cases where a change is only relevant for one of these files, we can ignore this failure. It might be possible to use a more elaborate mechanism to clarify that it's indeed fine to change only one, but this seems too much effort. The main goal is to avoid neglecting changes that should go in both.
@ysbaddaden
Copy link
Collaborator

ysbaddaden commented Dec 18, 2025

I finally reproduced locally. I tried a few things (always run interpreter in a specific single-threaded context, disabling the PCRE2 thread local, but to no avail.

Then I looked at the backtrace... and the error is on the following line:

stack_top.clear(compiled_def.local_vars.max_bytesize)

  1. PCRE2 calls back into Crystal
  2. the interpreter checkouts a fiber stack (to interpret the callback) from a local Fiber::StackPool;
  3. the interpreter tries to clear the stack;
  4. memset segfaults => I assume the stack has been freed?

@ysbaddaden
Copy link
Collaborator

This is related to reusing a dead stack fiber set on the current thread:

reusing dead stack Fiber::Stack(@pointer=Pointer(Void)@0x1bbe9550000, @bottom=Pointer(Void)@0x1bbe9d50000, @size=8388608, @reusable=true)
Invalid memory access (C0000005) at address 0x1bbe9550000

Which would point the issue to the following line:

if (stack = Thread.current.dead_fiber_stack?) && stack.reusable?

@ysbaddaden
Copy link
Collaborator

And this is likely related to the interpreter always releasing the fiber stack immediately:

# Checks out a stack from the stack pool and yields it to the given block.
# Once the block returns, the stack is returned to the pool.
# The stack is not cleared after or before it's used.
def checkout_stack(& : UInt8* -> _)
stack = @stack_pool.checkout
begin
yield stack.pointer.as(UInt8*)
ensure
@stack_pool.release(stack)
end
end

I'll try to add an option to Fiber::StackPool to disable recycling a dead fiber stack, because this is not a fiber stack pool, this is a stack pool for C callbacks!

ysbaddaden added a commit to ysbaddaden/crystal that referenced this pull request Dec 19, 2025
ysbaddaden added a commit to ysbaddaden/crystal that referenced this pull request Dec 19, 2025
@ysbaddaden
Copy link
Collaborator

ysbaddaden commented Dec 19, 2025

The bug is fixed in #16518. See this example run.

@ysbaddaden ysbaddaden added this to the 1.19.0 milestone Dec 19, 2025
@straight-shoota straight-shoota merged commit 167c0ba into crystal-lang:master Dec 23, 2025
40 of 41 checks passed
@straight-shoota straight-shoota deleted the infra/compiler-execution_context-windows branch December 23, 2025 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants