isdefined can be incorrectly constant propagated #48999

simonbyrne · 2023-03-14T20:25:24Z

If PkgA conditionally defines a variable during __init__():

module PkgA
function __init__()
    global var
    if haskey(ENV, "varA")
        var = ENV["varA"]
    end
end
end

then a check to isdefined(PkgA, :var) in a the __init__() function of a dependent package may be incorrectly compiled to be constant. This is a problem if you have code such as:

module PkgB
using PkgA

function __init__()
    if isdefined(PkgA, :var)
        @show PkgA.var
    end
end
end

If I precompile once with the environment variable varA set, then run it again without, I get the following errors:

Julia 1.8.5

ERROR: InitError: UndefVarError: var not defined
Stacktrace:
  [1] getproperty
    @ ./Base.jl:31 [inlined]
  [2] macro expansion
    @ ./show.jl:1047 [inlined]
  [3] __init__()
    @ PkgB ~/misc/undef/PkgB/src/PkgB.jl:7
  [4] _include_from_serialized(pkg::Base.PkgId, path::String, depmods::Vector{Any})
    @ Base ./loading.jl:831
  [5] _require_search_from_serialized(pkg::Base.PkgId, sourcepath::String, build_id::UInt64)
    @ Base ./loading.jl:1039
  [6] _require(pkg::Base.PkgId)
    @ Base ./loading.jl:1315
  [7] _require_prelocked(uuidkey::Base.PkgId)
    @ Base ./loading.jl:1200
  [8] macro expansion
    @ ./loading.jl:1180 [inlined]
  [9] macro expansion
    @ ./lock.jl:223 [inlined]
 [10] require(into::Module, mod::Symbol)
    @ Base ./loading.jl:1144
during initialization of module PkgB

Julia 1.9-rc1:

➜  undef git:(main) julia@1.9 --project=PkgB -e 'using PkgB'

[6018] signal (11.2): Segmentation fault: 11
in expression starting at none:1
__init__ at /Users/simon/misc/undef/PkgB/src/PkgB.jl:7
jfptr___init___11 at /Users/simon/.julia/compiled/v1.9/PkgB/DW2fn_NQBT0.dylib (unknown line)
ijl_apply_generic at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
jl_module_run_initializer at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
ijl_init_restored_modules at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
register_restored_modules at ./loading.jl:1074
_include_from_serialized at ./loading.jl:1020
_require_search_from_serialized at ./loading.jl:1465
_require at ./loading.jl:1742
_require_prelocked at ./loading.jl:1619
macro expansion at ./loading.jl:1607 [inlined]
macro expansion at ./lock.jl:267 [inlined]
require at ./loading.jl:1570
jfptr_require_28442 at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/sys.dylib (unknown line)
ijl_apply_generic at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
eval_import_path at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
jl_toplevel_eval_flex at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
jl_toplevel_eval_flex at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
ijl_toplevel_eval_in at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
jlplt_ijl_toplevel_eval_in_12236 at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/sys.dylib (unknown line)
eval at ./boot.jl:370 [inlined]
exec_options at ./client.jl:280
_start at ./client.jl:522
jfptr__start_30743 at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/sys.dylib (unknown line)
ijl_apply_generic at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
true_main at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
jl_repl_entrypoint at /Applications/Julia-1.9.app/Contents/Resources/julia/lib/julia/libjulia-internal.1.9.dylib (unknown line)
Allocations: 2995 (Pool: 2984; Big: 11); GC: 0

Originally observed in JuliaGPU/CUDA.jl#1798

I have created a simple reproducer here:
https://github.com/simonbyrne/undef

The text was updated successfully, but these errors were encountered:

KristofferC · 2023-03-16T14:58:28Z

@vchuravy, this reproduces on 1.8 (and 1.7) so I am not sure the milestone is warranted. Sure, the error mode is a bit harsher now but the bug is presumably the same.

vchuravy · 2023-03-16T15:01:29Z

A segmentation fault is not something a user can guard against. I will try to spend some cycles on it today or tomorrow.

vchuravy · 2023-03-16T15:22:57Z

RR trace available: https://julialang-dumps.s3.amazonaws.com/reports/2023-03-16T15-21-22-vchuravy.tar.zst

vchuravy · 2023-03-16T15:38:36Z

The seqfault is "just" that we expect the global to exist...

On the other hand, this seems clearly wrong:

julia/base/compiler/abstractinterpretation.jl

Lines 2412 to 2423 in 551f303

    
           elseif isa(sym, Symbol) 
        
               if isdefined(sv.mod, sym) 
        
                   t = Const(true) 
        
               elseif InferenceParams(interp).assume_bindings_static 
        
                   t = Const(false) 
        
               end 
        
           elseif isa(sym, GlobalRef) 
        
               if isdefined(sym.mod, sym.name) 
        
                   t = Const(true) 
        
               elseif InferenceParams(interp).assume_bindings_static 
        
                   t = Const(false) 
        
               end

simeonschaub · 2023-03-16T15:47:06Z

I'm not sure there is an easy fix for this without breaking a bunch of crucial optimizations. The assumptions that bindings can never be undefined once they have been accessed is pretty essential to most of our handling of globals. Could you instead initialize the global as nothing first?

vchuravy · 2023-03-16T16:14:57Z

So from my brief investigation we have made the assumption throughout inference and codegen that:

A GlobalRef seen valid once will always be defined. This assumption holds in the context of JIT since we don't have a undefine this GlobalRef operation in the language. In the context of caching this assumption fails since we are not gurantueed to re-execute the codepath that defined the global.

One solution I thought about was tracking the context in which the global is defined? I.e. if it is a global in a top-level constant the assumption is fine, if it is a global defined from a "function execution/compilation" the assumption is not okay to make.

A long-term solution to this is @vtjnash proposal in #8870 (comment) i.e. extend the world-age semantics to global-ref to track the range of validity.

Either way this will require rather invasive changes through-out inference and the compiler :/

aviatesk · 2024-04-18T08:37:41Z

Fixed by #53750.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

…55297) There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

@noinline

…55297) There is still room for improvement in the accuracy of `getfield` and `isdefined` for structs with uninitialized fields. This commit aims to enhance the accuracy of struct field defined-ness by propagating such struct as `PartialStruct` in cases where fields that might be uninitialized are confirmed to be defined. Specifically, the improvements are made in the following situations: 1. when a `:new` expression receives arguments greater than the minimum number of initialized fields. 2. when new information about the initialized fields of `x` can be obtained in the `then` branch of `if isdefined(x, :f)`. Combined with the existing optimizations, these improvements enable DCE in scenarios such as: ```julia julia> @noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing); julia> @allocated broadcast_noescape1(Ref("x")) 16 # master 0 # this PR ``` One important point to note is that, as revealed in #48999, fields and globals can revert to `undef` during precompilation. This commit does not affect globals. Furthermore, even for fields, the refinements made by 1. and 2. are propagated along with data-flow, and field defined-ness information is only used when fields are confirmed to be initialized. Therefore, the same issues as #48999 will not occur by this commit.

oscardssmith added the bug Indicates an unexpected problem or unintended behavior label Mar 14, 2023

simonbyrne mentioned this issue Mar 14, 2023

Buggy precompilation of init-defined symbols can break CUDA_Driver_jll initialization JuliaGPU/CUDA.jl#1798

Closed

vchuravy added this to the 1.9 milestone Mar 14, 2023

vchuravy self-assigned this Mar 16, 2023

simeonschaub removed this from the 1.9 milestone Mar 16, 2023

vchuravy removed their assignment Mar 16, 2023

vchuravy added compiler:codegen Generation of LLVM IR and native code compiler:inference Type inference caching labels Mar 16, 2023

aviatesk self-assigned this Mar 22, 2023

simonbyrne mentioned this issue Jun 15, 2023

UndefVarError: compile_cache not defined JuliaGPU/GPUCompiler.jl#470

Closed

giordano added the rr trace included label Jul 15, 2023

LilithHafner added the regression 1.9 Regression in the 1.9 release label Aug 7, 2023

vchuravy mentioned this issue Mar 15, 2024

inference correctness: fields and globals can revert to undef #53750

Merged

aviatesk closed this as completed Apr 18, 2024

aviatesk mentioned this issue Jul 29, 2024

inference: model partially initialized structs with PartialStruct #55297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

isdefined can be incorrectly constant propagated #48999

isdefined can be incorrectly constant propagated #48999

simonbyrne commented Mar 14, 2023

KristofferC commented Mar 16, 2023

vchuravy commented Mar 16, 2023

vchuravy commented Mar 16, 2023

vchuravy commented Mar 16, 2023

simeonschaub commented Mar 16, 2023

vchuravy commented Mar 16, 2023

aviatesk commented Apr 18, 2024

isdefined can be incorrectly constant propagated #48999

isdefined can be incorrectly constant propagated #48999

Comments

simonbyrne commented Mar 14, 2023

KristofferC commented Mar 16, 2023

vchuravy commented Mar 16, 2023

vchuravy commented Mar 16, 2023

vchuravy commented Mar 16, 2023

simeonschaub commented Mar 16, 2023

vchuravy commented Mar 16, 2023

aviatesk commented Apr 18, 2024