Skip to content

Commit

Permalink
effects: handle va-method properly when refining `:inaccessiblememonl…
Browse files Browse the repository at this point in the history
…y` (#48854)

Previously the `:inaccessiblememonly` effect bit may be wrongly refined
when analyzing va-method, e.g.:
```julia
julia> callgetfield1(xs...) = getfield(getfield(xs, 1), 1)
callgetfield1 (generic function with 1 method)

julia> Base.infer_effects(callgetfield1, (Base.RefValue{Symbol},))
(+c,+e,!n,+t,+s,+m,+i) # inaccessiblememonly is wrongly refined here
```

This leads to wrong concrete evaluation of `callgetfield1` and thus may
result in a problem like below:
```julia
julia> const GLOBAL_XS = Ref(:julia);

julia> global_getfield() = callgetfield1(GLOBAL_XS);

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia
Test Passed

julia> GLOBAL_XS[] = :julia2;

julia> @test let
           Base.Experimental.@force_compile
           global_getfield()
       end === :julia2 # this fails
Test Failed at REPL[8]:1
  Expression: let
        #= REPL[8]:2 =# Base.Experimental.@force_compile
        global_getfield()
    end === :julia2
   Evaluated: julia === julia2
```

This commit fixes it up.
  • Loading branch information
aviatesk authored Mar 3, 2023
1 parent a4d94a3 commit 903dbd5
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 5 deletions.
2 changes: 1 addition & 1 deletion base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2823,7 +2823,7 @@ function typeinf_local(interp::AbstractInterpreter, frame::InferenceState)
@assert !frame.inferred
frame.dont_work_on_me = true # mark that this function is currently on the stack
W = frame.ip
nargs = narguments(frame)
nargs = narguments(frame, #=include_va=#false)
slottypes = frame.slottypes
ssavaluetypes = frame.ssavaluetypes
bbs = frame.cfg.blocks
Expand Down
8 changes: 5 additions & 3 deletions base/compiler/inferencestate.jl
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,11 @@ get_curr_ssaflag(sv::InferenceState) = sv.src.ssaflags[sv.currpc]
add_curr_ssaflag!(sv::InferenceState, flag::UInt8) = sv.src.ssaflags[sv.currpc] |= flag
sub_curr_ssaflag!(sv::InferenceState, flag::UInt8) = sv.src.ssaflags[sv.currpc] &= ~flag

function narguments(sv::InferenceState)
function narguments(sv::InferenceState, include_va::Bool=true)
def = sv.linfo.def
isva = isa(def, Method) && def.isva
nargs = length(sv.result.argtypes) - isva
nargs = length(sv.result.argtypes)
if !include_va
nargs -= isa(def, Method) && def.isva
end
return nargs
end
2 changes: 1 addition & 1 deletion base/compiler/typeinfer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ function adjust_effects(sv::InferenceState)
# always throwing an error counts or never returning both count as consistent
ipo_effects = Effects(ipo_effects; consistent=ALWAYS_TRUE)
end
if is_inaccessiblemem_or_argmemonly(ipo_effects) && all(1:narguments(sv)) do i::Int
if is_inaccessiblemem_or_argmemonly(ipo_effects) && all(1:narguments(sv, #=include_va=#true)) do i::Int
return is_mutation_free_argtype(sv.slottypes[i])
end
ipo_effects = Effects(ipo_effects; inaccessiblememonly=ALWAYS_TRUE)
Expand Down
15 changes: 15 additions & 0 deletions test/compiler/effects.jl
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,21 @@ global inconsistent_condition_ref = Ref{Bool}(false)
end
end |> !Core.Compiler.is_consistent

# should handle va-method properly
callgetfield1(xs...) = getfield(getfield(xs, 1), 1)
@test !Core.Compiler.is_inaccessiblememonly(Base.infer_effects(callgetfield1, (Base.RefValue{Symbol},)))
const GLOBAL_XS = Ref(:julia)
global_getfield() = callgetfield1(GLOBAL_XS)
@test let
Base.Experimental.@force_compile
global_getfield()
end === :julia
GLOBAL_XS[] = :julia2
@test let
Base.Experimental.@force_compile
global_getfield()
end === :julia2

# the `:inaccessiblememonly` helper effect allows us to prove `:effect_free`-ness of frames
# including `setfield!` modifying local mutable object

Expand Down

0 comments on commit 903dbd5

Please sign in to comment.