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.

Sign up for GitHub

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

Jump to bottom

effects: fix effects of atomic pointer operations #57806

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

effects: fix effects of atomic pointer operations #57806

wants to merge 3 commits into from
+72 −23

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Mar 17, 2025 •
edited
Loading

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.

@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 •
edited
Loading

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
Open
85 tasks
aviatesk added a commit that referenced this pull request Mar 21, 2025
@aviatesk
effects: give the most conservative effects to unhandled builtins
f0c7c24 
This commit fixes a bug where `builtin_tfunction` was giving the wrong
effects to builtins that weren't actually implemented.
xref: #57806 (comment)

Now, when we add new builtins, unless we manually update
`_EFFECTS_KNOWN_BUILTINS`, they'll automatically get the most
conservative `Effects()`.

Effects for builtins like `getglobal` are calculated individually inside
`abstract_call_builtin`, so there is no handling for them within
`builtin_effects` and thus they aren't in `_EFFECTS_KNOWN_BUILTINS`.
If we want to give these built-ins a better `stmt_effect_flag`, we need
to handle it in `builtin_effects` properly.
@aviatesk aviatesk mentioned this pull request Mar 21, 2025
Open
aviatesk and others added 3 commits March 22, 2025 15:20
@aviatesk
effects: fix effects of atomic pointer operations
ce0aae0 
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
@vtjnash @aviatesk
[Compiler] fix intrinsic_effects computation
be4ab9b 
The atomic operators, in particular, where missing from the list, though
many others were also in incorrect lists also. Try to deduplicate the
lists slightly.

Co-authored-by: Shuhei Kadowaki <[email protected]>
@aviatesk
add few more tests
bd67367
aviatesk added a commit that referenced this pull request Mar 22, 2025
@aviatesk
effects: give the most conservative effects to unhandled builtins
af154ee 
This commit fixes a bug where `builtin_tfunction` was giving the wrong
effects to builtins that weren't actually implemented.
xref: #57806 (comment)

Now, when we add new builtins, unless we manually update
`_EFFECTS_KNOWN_BUILTINS`, they'll automatically get the most
conservative `Effects()`.

Effects for builtins like `getglobal` are calculated individually inside
`abstract_call_builtin`, so there is no handling for them within
`builtin_effects` and thus they aren't in `_EFFECTS_KNOWN_BUILTINS`.
If we want to give these built-ins a better `stmt_effect_flag`, we need
to handle it in `builtin_effects` properly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KristofferC KristofferC KristofferC left review comments

@vtjnash vtjnash vtjnash approved these changes

Assignees
No one assigned
Labels
backport 1.12 Change should be backported to release-1.12
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

REPL: Completion on a LOAD_PATH mutation breaks LOAD_PATH
3 participants
@aviatesk @vtjnash @KristofferC