Move thunk and export evaluation out of `jl_toplevel_eval_flex` by c42f · Pull Request #60528 · JuliaLang/julia

c42f · 2026-01-02T13:20:47Z

Separate toplevel CodeInfo evaluation out of jl_toplevel_eval_flex into
jl_eval_thunk so that CodeInfo can become the basic unit of toplevel
evaluation rather than Expr and there's no need to construct an
Expr(:thunk).

The latest world age set and restore now moves entirely into
jl_eval_thunk as jl_toplevel_eval_flex no longer modifies the task's
world age. (Note that the world age needs to be set in jl_eval_thunk before
the calls to jl_resolve_definition_effects_in_ir. Previously this happened
to work because the callers of jl_toplevel_eval_flex increment the world
age)

Also move public/export batching out of jl_toplevel_eval_flex
and into a C runtime function jl_module_public which can be called
without using eval and without constructing an Expr.

Both of these changes help with JuliaLowering integration and with
disentangling top level evaluation from Expr and from the code in
toplevel.c.

Separate CodeInfo evaluation out of `jl_toplevel_eval_flex` into `jl_eval_thunk` so that `CodeInfo` can become the basic unit of toplevel evaluation rather than `Expr`. The latest world age set and restore now moves entirely into jl_eval_thunk as jl_toplevel_eval_flex no longer modifies the task's world age. The world age set and restore inside jl_eval_thunk needs to moves outward so that it wraps around the calls to jl_resolve_definition_effects_in_ir which interprets some limited top level expressions for the argument types foreigncall, etc. (Previously the world age was updated to the latest in functions which call jl_toplevel_eval_flex so resolving definition effects happened to work.)

Move public/export processing out of jl_toplevel_eval_flex and into a C runtime function which can be called without using eval and without constructing an Expr.

Various seemingly-unrelated changes in `jl_toplevel_eval_flex` mysteriously cause the static analyzer to see a rooting problem in `jl_declare_constant_val2`: The `val` argument is currently declared JL_MAYBE_UNROOTED, but we call JL_LOCK without rooting it. I've done the simplest fix here which is to root `val` inside `jl_declare_constant_val2`. An alternative would be to remove the `JL_MAYBE_UNROOTED` annotation which could be an artifact of the previous implementation which didn't require the lock prior to #57102.

c42f · 2026-01-02T23:19:01Z

Ok, the GC static analyzer failure in jl_declare_constant_val2 here was weird - it seemingly had nothing to do with the code changed in this PR - confirmed by going back to the previous version and making trivial changes to jl_toplevel_eval_flex. However it does seem be a valid issue found by the analyzer due to taking the world counter lock when the val argument is declared JL_MAYBE_UNROOTED. I've just rooted val which felt most conservative/safe but an alternative would be to remove the JL_MAYBE_UNROOTED annotation. @Keno jl_declare_constant_val2 is one of yours - what would you prefer to do?

Unfortunately these two global variables are still with us for * deprecation binding errors * printing of critical errors in jl_fprint_critical_error To allow these cases to continue working, set these variables based on the CodeInfo's debuginfo.

topolarity

Seems like a fairly mechanical / appreciated re-factor.

Changes look good to me!

topolarity · 2026-01-13T18:07:51Z

src/toplevel.c

+
+    size_t last_age = ct->world_age;
+    size_t world = jl_atomic_load_acquire(&jl_world_counter);
+    ct->world_age = world;


Did the "scope" of this world change have to expand?

Ah you noted this in the OP of the PR.

Looks good to me, another good catch. Thanks!

topolarity · 2026-01-13T18:17:57Z

src/toplevel.c

    jl_binding_t *b, jl_module_t *mod, jl_sym_t *var, jl_value_t *val,
    enum jl_partition_kind constant_kind)
 {
+    JL_GC_PUSH1(&val);


I don't know why we have this as JL_MAYBE_UNROOTED.

This is a real bug though, so thanks for the fix!

I don't know either.

My best guess is that in a previous implementation it wasn't required to be rooted in the call to the internals so @Keno opted to relax the ABI annotations under the idea that it'd be slightly more efficient for use by other C code. However in the meantime the internals changed to take a lock here which forces the rooting.

Then again it could have been intentional so I opted just to root it which seemed more conservative 🤷‍♀️

topolarity · 2026-01-13T18:20:06Z

src/julia.h

 JL_DLLEXPORT void jl_import_module(jl_task_t *ct, jl_module_t *m, jl_module_t *import, jl_sym_t *asname);
 JL_DLLEXPORT void jl_module_using(jl_module_t *to, jl_module_t *from, size_t flags);
-int jl_module_public_(jl_module_t *from, jl_sym_t *s, int exported, size_t new_world);
+JL_DLLEXPORT void jl_module_public(jl_module_t *from, jl_value_t **symbols, size_t nsymbols, int exported);


@vtjnash deleted this export in #57765

I assume that was just part of the re-factor around handling the world counter lock correctly and not removed due to API concerns, but mentioning here just in case.

Yep 👍 I believe the reason Jameson removed it was that the refactored jl_module_public_() requires the user to hold the world counter lock so it's strictly internal to the C code. An alternative public API wasn't provided in that PR, but we need one if we want to replicate the current jl_toplevel_eval_flex behavior without calling jl_toplevel_eval_flex.

(Side note - I've chosen the signature so the symbols array can be allocated in any way rather than forcing it to be a Julia data structure. So this should be reasonable to use from C code as well.)

jl_module_public was meant to be converted to a Builtin (e.g. similar to #58279), but I guess that hasn't happened yet

c42f · 2026-01-14T04:39:59Z

fairly mechanical

Haha! You would think so, hey :)

Mostly it was, but you saw the world age "scope expansion" in jl_toplevel_eval_thunk right?? (good eye :) ) Well. I originally refactored it without that scope change and the only immediate effects were some extremely subtle and confusing bugs involving a ccall() within the precompile script for REPL. (Like ... what do you mean runtime?? Cvoid is definitely defined. Look: I can even call println(Cvoid) on the previous line of the code before the ccall)

c42f · 2026-01-14T04:45:49Z

Ok, I'm going to merge this for the sake of progress as it's been up for a while. Much appreciate the review @topolarity!

Couple of things I'm more than happy to tweak or have someone else tweak:

The details of how the rooting works
Adding builtins for jl_eval_thunk / jl_module_public.

vtjnash · 2026-01-19T16:38:32Z

src/toplevel.c

        jl_resolve_definition_effects_in_ir((jl_array_t*)thk->code, m, NULL, NULL, 0);
        // Don't infer blocks containing e.g. method definitions, since it's probably not worthwhile.
-        size_t world = jl_atomic_load_acquire(&jl_world_counter);
-        ct->world_age = world;


jl_resolve_definition_effects_in_ir has side effects, so this seems should be mandatory to appear afterwards (sorry about being so slow to post-review this)

I see the comment about this:

(Note that the world age needs to be set in jl_eval_thunk before
the calls to jl_resolve_definition_effects_in_ir. Previously this happened
to work because the callers of jl_toplevel_eval_flex increment the world
age)

But it seems like you're introducing a bug here to be changing the order here (afaik, everything in jl_resolve_definition_effects_in_ir requires having an implied invokelatest to correctly observe any updated bindings from previous statements in the same block)

I assume this is a concrete example:

julia> MyInt() = (Core.eval(Main, :(const MyString = Cstring)); Cint) julia> ccall(:puts, MyInt(), (MyString,), "hello") WARNING: Detected access to binding `Main.MyString` in a world prior to its definition world. Julia 1.12 has introduced more strict world age semantics for global bindings. !!! This code may malfunction under Revise. !!! This code will error in future versions of Julia. Hint: Add an appropriate `invokelatest` around the access to this binding. To make this warning an error, and hence obtain a stack trace, use `julia --depwarn=error`. hello 6

(which seems to misbehave strangely in older Julia versions, but I assume that's for other reasons)

This is not an example of the regression (although arguably we are missing world updates here IMO)

@c42f found one that is:

julia> side_effect() = eval(:(x = 100; Cint)) julia> let @info "" x ccall(:puts, side_effect(), (Cstring,), "hi") end

similarly:

julia> rettype() = (Core.eval(Main, :(newf() = 42)); Cint) julia> begin ccall(:puts, rettype(), (Cstring,), "hello") newf() end

PR #60528 removed a world age update after `jl_resolve_definition_effects` as part of a larger refactoring. However `jl_resolve_definition_effects` evaluates selected subexpressions of a thunk - such as the ccall return type - and this may run arbitrary code. Thus a world age update seems required for consistency.

@vtjnash

#61349) PR #60528 removed a world age update after `jl_resolve_definition_effects` as part of a larger refactoring. However `jl_resolve_definition_effects` evaluates selected subexpressions of a thunk - such as the ccall return type - and this may run arbitrary code. Thus a world age update seems required for consistency. IIUC this is the fix requested in the post-commit review by @vtjnash here https://github.com/JuliaLang/julia/pull/60528/files#r2705468506 (Side note - does anyone have an example of code which isn't actively perverse and where this would be required? I think this is good to have regardless (at least as long as we allow arbitrary code to run "before" running the thunk proper) but I couldn't think of an example where this is required to run clean user code.)

c42f added 2 commits January 2, 2026 19:44

jl_module_public API for declaring symbols public/exported

e850b3b

Move public/export processing out of jl_toplevel_eval_flex and into a C runtime function which can be called without using eval and without constructing an Expr.

c42f mentioned this pull request Jan 2, 2026

Compiler frontend API / lowering iterator #60529

Open

5 tasks

c42f force-pushed the caf/jl-eval-thunk branch from fe5e1da to cd731a5 Compare January 7, 2026 02:57

oscardssmith requested a review from mlechu January 7, 2026 16:33

topolarity approved these changes Jan 13, 2026

View reviewed changes

c42f merged commit 26c109e into master Jan 14, 2026
8 checks passed

c42f deleted the caf/jl-eval-thunk branch January 14, 2026 04:57

vtjnash reviewed Jan 19, 2026

View reviewed changes

c42f mentioned this pull request Mar 18, 2026

Fix world update after resolving definition effects in jl_eval_thunk #61349

Merged

Uh oh!

Conversation

c42f commented Jan 2, 2026

Uh oh!

c42f commented Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topolarity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c42f Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c42f commented Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

c42f commented Jan 14, 2026

Uh oh!

Uh oh!

vtjnash Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topolarity Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topolarity Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

c42f commented Jan 2, 2026 •

edited

Loading

c42f Jan 14, 2026 •

edited

Loading

c42f commented Jan 14, 2026 •

edited

Loading

vtjnash Jan 19, 2026 •

edited

Loading

topolarity Mar 17, 2026 •

edited

Loading

topolarity Mar 17, 2026 •

edited

Loading