more field property annotations (JuliaLang#47677)

Mostly looking at fixing more implicit data race with more atomics. Some of this (at the Julia level) is not strictly necessary since we already intend to start using implicit release-consume ordering for references. But since the user should not be changing these fields anyways, or risk severe breakage, this should be fine to mark explicitly.
alahmdi · Dec 7, 2022 · c8662b5 · c8662b5
1 parent db00cc1
commit c8662b5
Show file tree

Hide file tree

Showing 25 changed files with 259 additions and 167 deletions.
diff --git a/base/Base.jl b/base/Base.jl
@@ -112,7 +112,7 @@ function Core.kwcall(kwargs, ::typeof(invoke), f, T, args...)
     return invoke(Core.kwcall, T, kwargs, f, args...)
 end
 # invoke does not have its own call cache, but kwcall for invoke does
-typeof(invoke).name.mt.max_args = 3 # invoke, f, T, args...
+setfield!(typeof(invoke).name.mt, :max_args, 3, :monotonic) # invoke, f, T, args...
 
 # core operations & types
 include("promotion.jl")

diff --git a/base/compiler/tfuncs.jl b/base/compiler/tfuncs.jl
@@ -809,36 +809,58 @@ function try_compute_fieldidx(typ::DataType, @nospecialize(field))
     return field
 end
 
-function getfield_boundscheck(argtypes::Vector{Any}) # ::Union{Bool, Nothing, Type{Bool}}
+function getfield_boundscheck(argtypes::Vector{Any}) # ::Union{Bool, Nothing}
     if length(argtypes) == 2
         return true
     elseif length(argtypes) == 3
         boundscheck = argtypes[3]
-        if boundscheck === Const(:not_atomic) # TODO: this is assuming not atomic
-            boundscheck = Bool
+        isvarargtype(boundscheck) && return nothing
+        if widenconst(boundscheck) === Symbol
+            return true
         end
     elseif length(argtypes) == 4
         boundscheck = argtypes[4]
     else
         return nothing
     end
     isvarargtype(boundscheck) && return nothing
-    widenconst(boundscheck) !== Bool && return nothing
+    widenconst(boundscheck) === Bool || return nothing
     boundscheck = widenconditional(boundscheck)
     if isa(boundscheck, Const)
-        return boundscheck.val
+        return boundscheck.val::Bool
     else
-        return Bool
+        return nothing
     end
 end
 
-function getfield_nothrow(argtypes::Vector{Any})
-    boundscheck = getfield_boundscheck(argtypes)
+function getfield_nothrow(argtypes::Vector{Any}, boundscheck::Union{Bool,Nothing}=getfield_boundscheck(argtypes))
+    @specialize boundscheck
     boundscheck === nothing && return false
-    return getfield_nothrow(argtypes[1], argtypes[2], !(boundscheck === false))
+    ordering = Const(:not_atomic)
+    if length(argtypes) == 3
+        isvarargtype(argtypes[3]) && return false
+        if widenconst(argtypes[3]) !== Bool
+            ordering = argtypes[3]
+        end
+    elseif length(argtypes) == 4
+        ordering = argtypes[4]
+    elseif length(argtypes) != 2
+        return false
+    end
+    isvarargtype(ordering) && return false
+    widenconst(ordering) === Symbol || return false
+    if isa(ordering, Const)
+        ordering = ordering.val::Symbol
+        if ordering !== :not_atomic # TODO: this is assuming not atomic
+            return false
+        end
+        return getfield_nothrow(argtypes[1], argtypes[2], !(boundscheck === false))
+    else
+        return false
+    end
 end
 function getfield_nothrow(@nospecialize(s00), @nospecialize(name), boundscheck::Bool)
-    # If we don't have boundscheck and don't know the field, don't even bother
+    # If we don't have boundscheck off and don't know the field, don't even bother
     if boundscheck
         isa(name, Const) || return false
     end
@@ -880,7 +902,6 @@ function getfield_nothrow(@nospecialize(s00), @nospecialize(name), boundscheck::
     if isa(s, DataType)
         # Can't say anything about abstract types
         isabstracttype(s) && return false
-        s.name.atomicfields == C_NULL || return false # TODO: currently we're only testing for ordering === :not_atomic
         # If all fields are always initialized, and bounds check is disabled, we can assume
         # we don't throw
         if !boundscheck && s.name.n_uninitialized == 0
@@ -890,6 +911,7 @@ function getfield_nothrow(@nospecialize(s00), @nospecialize(name), boundscheck::
         isa(name, Const) || return false
         field = try_compute_fieldidx(s, name.val)
         field === nothing && return false
+        isfieldatomic(s, field) && return false # TODO: currently we're only testing for ordering === :not_atomic
         field <= datatype_min_ninitialized(s) && return true
         # `try_compute_fieldidx` already check for field index bound.
         !isvatuple(s) && isbitstype(fieldtype(s0, field)) && return true
@@ -1210,12 +1232,12 @@ function setfield!_nothrow(@specialize(𝕃::AbstractLattice), s00, name, v)
         # Can't say anything about abstract types
         isabstracttype(s) && return false
         ismutabletype(s) || return false
-        s.name.atomicfields == C_NULL || return false # TODO: currently we're only testing for ordering === :not_atomic
         isa(name, Const) || return false
         field = try_compute_fieldidx(s, name.val)
         field === nothing && return false
         # `try_compute_fieldidx` already check for field index bound.
         isconst(s, field) && return false
+        isfieldatomic(s, field) && return false # TODO: currently we're only testing for ordering === :not_atomic
         v_expected = fieldtype(s0, field)
         return v ⊑ v_expected
     end
@@ -2074,20 +2096,14 @@ function getfield_effects(argtypes::Vector{Any}, @nospecialize(rt))
     if !(length(argtypes) ≥ 2 && getfield_notundefined(widenconst(obj), argtypes[2]))
         consistent = ALWAYS_FALSE
     end
-    if getfield_boundscheck(argtypes) !== true
+    nothrow = getfield_nothrow(argtypes, true)
+    if !nothrow && getfield_boundscheck(argtypes) !== true
         # If we cannot independently prove inboundsness, taint consistency.
         # The inbounds-ness assertion requires dynamic reachability, while
         # :consistent needs to be true for all input values.
         # N.B. We do not taint for `--check-bounds=no` here -that happens in
         # InferenceState.
-        if length(argtypes) ≥ 2 && getfield_nothrow(argtypes[1], argtypes[2], true)
-            nothrow = true
-        else
-            consistent = ALWAYS_FALSE
-            nothrow = false
-        end
-    else
-        nothrow = getfield_nothrow(argtypes)
+        consistent = ALWAYS_FALSE
     end
     if hasintersect(widenconst(obj), Module)
         inaccessiblememonly = getglobal_effects(argtypes, rt).inaccessiblememonly

diff --git a/base/operators.jl b/base/operators.jl
@@ -568,7 +568,7 @@ function afoldl(op, a, bs...)
     end
     return y
 end
-typeof(afoldl).name.mt.max_args = 34
+setfield!(typeof(afoldl).name.mt, :max_args, 34, :monotonic)
 
 for op in (:+, :*, :&, :|, :xor, :min, :max, :kron)
     @eval begin

diff --git a/base/reflection.jl b/base/reflection.jl
@@ -294,6 +294,29 @@ function isconst(@nospecialize(t::Type), s::Int)
     return unsafe_load(Ptr{UInt32}(constfields), 1 + s÷32) & (1 << (s%32)) != 0
 end
 
+"""
+    isfieldatomic(t::DataType, s::Union{Int,Symbol}) -> Bool
+
+Determine whether a field `s` is declared `@atomic` in a given type `t`.
+"""
+function isfieldatomic(@nospecialize(t::Type), s::Symbol)
+    t = unwrap_unionall(t)
+    isa(t, DataType) || return false
+    return isfieldatomic(t, fieldindex(t, s, false))
+end
+function isfieldatomic(@nospecialize(t::Type), s::Int)
+    t = unwrap_unionall(t)
+    # TODO: what to do for `Union`?
+    isa(t, DataType) || return false # uncertain
+    ismutabletype(t) || return false # immutable structs are never atomic
+    1 <= s <= length(t.name.names) || return false # OOB reads are not atomic (they always throw)
+    atomicfields = t.name.atomicfields
+    atomicfields === C_NULL && return false
+    s -= 1
+    return unsafe_load(Ptr{UInt32}(atomicfields), 1 + s÷32) & (1 << (s%32)) != 0
+end
+
+
 
 """
     @locals()

diff --git a/doc/src/base/base.md b/doc/src/base/base.md
@@ -188,6 +188,7 @@ Base.fieldcount
 Base.hasfield
 Core.nfields
 Base.isconst
+Base.isfieldatomic
 ```
 
 ### Memory layout

diff --git a/src/Makefile b/src/Makefile
@@ -455,7 +455,7 @@ SA_EXCEPTIONS-jloptions.c                   := -Xanalyzer -analyzer-config -Xana
 SA_EXCEPTIONS-subtype.c                     := -Xanalyzer -analyzer-config -Xanalyzer silence-checkers="core.uninitialized.Assign;core.UndefinedBinaryOperatorResult"
 SA_EXCEPTIONS-codegen.c                     := -Xanalyzer -analyzer-config -Xanalyzer silence-checkers="core"
  # these need to be annotated (and possibly fixed)
-SKIP_IMPLICIT_ATOMICS := module.c staticdata.c codegen.cpp
+SKIP_IMPLICIT_ATOMICS := staticdata.c
  # these need to be annotated (and possibly fixed)
 SKIP_GC_CHECK := codegen.cpp rtutils.c
 

diff --git a/src/ccall.cpp b/src/ccall.cpp
@@ -85,7 +85,7 @@ static bool runtime_sym_gvs(jl_codectx_t &ctx, const char *f_lib, const char *f_
     else {
         std::string name = "ccalllib_";
         name += llvm::sys::path::filename(f_lib);
-        name += std::to_string(globalUniqueGeneratedNames++);
+        name += std::to_string(jl_atomic_fetch_add(&globalUniqueGeneratedNames, 1));
         runtime_lib = true;
         auto &libgv = ctx.emission_context.libMapGV[f_lib];
         if (libgv.first == NULL) {
@@ -105,7 +105,7 @@ static bool runtime_sym_gvs(jl_codectx_t &ctx, const char *f_lib, const char *f_
         std::string name = "ccall_";
         name += f_name;
         name += "_";
-        name += std::to_string(globalUniqueGeneratedNames++);
+        name += std::to_string(jl_atomic_fetch_add(&globalUniqueGeneratedNames, 1));
         auto T_pvoidfunc = JuliaType::get_pvoidfunc_ty(M->getContext());
         llvmgv = new GlobalVariable(*M, T_pvoidfunc, false,
                                     GlobalVariable::ExternalLinkage,
@@ -215,7 +215,7 @@ static Value *runtime_sym_lookup(
         std::string gvname = "libname_";
         gvname += f_name;
         gvname += "_";
-        gvname += std::to_string(globalUniqueGeneratedNames++);
+        gvname += std::to_string(jl_atomic_fetch_add(&globalUniqueGeneratedNames, 1));
         llvmgv = new GlobalVariable(*jl_Module, T_pvoidfunc, false,
                                     GlobalVariable::ExternalLinkage,
                                     Constant::getNullValue(T_pvoidfunc), gvname);
@@ -244,7 +244,7 @@ static GlobalVariable *emit_plt_thunk(
     libptrgv = prepare_global_in(M, libptrgv);
     llvmgv = prepare_global_in(M, llvmgv);
     std::string fname;
-    raw_string_ostream(fname) << "jlplt_" << f_name << "_" << globalUniqueGeneratedNames++;
+    raw_string_ostream(fname) << "jlplt_" << f_name << "_" << jl_atomic_fetch_add(&globalUniqueGeneratedNames, 1);
     Function *plt = Function::Create(functype,
                                      GlobalVariable::ExternalLinkage,
                                      fname, M);
@@ -858,7 +858,7 @@ static jl_cgval_t emit_llvmcall(jl_codectx_t &ctx, jl_value_t **args, size_t nar
     // Make sure to find a unique name
     std::string ir_name;
     while (true) {
-        raw_string_ostream(ir_name) << (ctx.f->getName().str()) << "u" << globalUniqueGeneratedNames++;
+        raw_string_ostream(ir_name) << (ctx.f->getName().str()) << "u" << jl_atomic_fetch_add(&globalUniqueGeneratedNames, 1);
         if (jl_Module->getFunction(ir_name) == NULL)
             break;
     }