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

effects: fix effects of atomic pointer operations #57806

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Mar 17, 2025

The _unsetindex!(::GenericMemoryRef) function was being concretely evaluated (meaning the code was actually being run during inference) by the REPLInterpreter.

The root cause is that the Base compiler system forgot to mark atomic_pointerset as "effect-free".

This is simply a bug in the effect system. It didn't cause problems during normal Base compilation because atomic_pointerset was correctly marked as "consistent" (concrete evaluation requires both "consistent" and "effect-free"). However, this was still a pretty risky situation.

The reason this only caused problems with REPL completion is that the REPLInterpreter intentionally ignores the "consistent" requirement under certain conditions to achieve better completion accuracy. This is usually fine, but it relies on "effect-free" being correctly marked. So, when there's a critical bug like this in the effect system, these kinds of dangerous issues can occur.

As discussed with Jameson earlier, the effects of atomic pointer operations are not precisely defined.
This commit includes the minimal changes necessary to fix #57780, but a more extensive audit is planned for later.

As discussed with Jameson earlier, the effects of atomic pointer
operations are not precisely defined.
This commit includes the minimal changes necessary to fix
#57780, but a more extensive audit is planned for later.

- closes #57780
@aviatesk aviatesk added the backport 1.12 Change should be backported to release-1.12 label Mar 17, 2025
@vtjnash
Copy link
Member

vtjnash commented Mar 17, 2025

I think you may still have more effect issues here though, since only some codepaths look at this function result, while others might decided to check the effect bits? Perhaps we should take the current state of https://github.com/JuliaLang/julia/compare/jn/builtin-effects-free instead?

julia> Base.infer_effects(Core.Intrinsics.atomic_pointerset, (Ptr{Int}, Int, Symbol))
(+c,+e,!n,+t,+s,+m,+u,+o,+r)

I had also previously noted the following additional mistakes in the effect system, while making that branch:

  • All of our Builtin functions seem to also potentially have similar bugs for any that accept atomic orderings
    • For example: the assumption that getfield is not effect-free is wrong if it has any atomic ordering (stronger than unordered)
    • This includes getfield_effects and isdefined_effects and possible others, since _ARGMEM_BUILTINS and _EFFECT_FREE_BUILTINS appear to make the same assumption that these do not imply synchronization with other threads (so the read of a particular value can “force” the runtime to must observe a read or write to a different location, which is a side-effect on memory which is not the argmem)
  • All of our Builtin functions assume terminates, notaskstate, nonoverlayed, and nortcall, especially if we don't have a specific model model, even when very obviously wrong (e.g. calling _call_in_world)
    julia> Base.infer_effects(Base.invoke_in_world, (Any,))
    (!c,!e,!n,+t,+s,!m,+u,+o,+r)
    

@aviatesk
Copy link
Member Author

Yeah, sure thing.
I was planning to use your commit as a base to fix all the issues you pointed out on Slack and earlier here in one go, but I haven't gotten to it yet. But they're definitely what we should do.
How about we merge this PR first to solve the immediate dangerous problem, and then we can finish your commit later? I put these fixes on my task list.

@KristofferC KristofferC mentioned this pull request Mar 20, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.12 Change should be backported to release-1.12
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REPL: Completion on a LOAD_PATH mutation breaks LOAD_PATH
2 participants