Fix :noub inference for pointerset/pointerref, getfield with bounds checking disabled

Compiler/src/tfuncs.jl

@@ -1033,7 +1033,7 @@ end
         # we may assume that we don't throw
         @assert !boundscheck
         ismod && return false
-        name ⊑ Int || name ⊑ Symbol || return false
+        name ⊑ Int || return false
         sty.name.n_uninitialized == 0 && return true
         nflds === nothing && return false
         for i = (datatype_min_ninitialized(sty)+1):nflds
@@ -1053,10 +1053,10 @@ end
     if isa(s, DataType)
         # Can't say anything about abstract types
         isabstracttype(s) && return false
-        # If all fields are always initialized, and bounds check is disabled,
-        # we can assume we don't throw
+        # If all fields are always initialized, bounds check is disabled, and
+        # name is an integer, we can assume we don't throw.
         if !boundscheck && s.name.n_uninitialized == 0
-            name ⊑ Int || name ⊑ Symbol || return false
+            name ⊑ Int || return false
             return true
         end
         # Else we need to know what the field is
@@ -2429,7 +2429,7 @@ const _ARGMEM_BUILTINS = Any[
 ]
 
 const _INCONSISTENT_INTRINSICS = Any[
-    Intrinsics.pointerref,      # this one is volatile
+    Intrinsics.pointerref,
     Intrinsics.sqrt_llvm_fast,  # this one may differ at runtime (by a few ulps)
     Intrinsics.have_fma,        # this one depends on the runtime environment
     Intrinsics.cglobal,         # cglobal lookup answer changes at runtime
@@ -2486,6 +2486,7 @@ end
 function getfield_effects(𝕃::AbstractLattice, argtypes::Vector{Any}, @nospecialize(rt))
     length(argtypes) < 2 && return EFFECTS_THROWS
     obj = argtypes[1]
+    name = argtypes[2]
     if isvarargtype(obj)
         return Effects(EFFECTS_TOTAL;
             consistent=CONSISTENT_IF_INACCESSIBLEMEMONLY,
@@ -2499,18 +2500,16 @@ function getfield_effects(𝕃::AbstractLattice, argtypes::Vector{Any}, @nospeci
     noub = ALWAYS_TRUE
     bcheck = getfield_boundscheck(argtypes)
     nothrow = getfield_nothrow(𝕃, argtypes, bcheck)
-    if !nothrow
-        if bcheck !== :on
-            # If we cannot independently prove inboundsness, taint `:noub`.
-            # The inbounds-ness assertion requires dynamic reachability,
-            # while `:noub` needs to be true for all input values.
-            # However, as a special exception, we do allow literal `:boundscheck`.
-            # `:noub` will be tainted in any caller using `@inbounds`
-            # based on the `:noinbounds` effect.
-            # N.B. We do not taint for `--check-bounds=no` here.
-            # That is handled in concrete evaluation.
-            noub = ALWAYS_FALSE
-        end
+    if !(bcheck === :on || isdefined_tfunc(𝕃, obj, name) === Const(true))
+        # If we cannot independently prove inboundsness, taint `:noub`.
+        # The inbounds-ness assertion requires dynamic reachability,
+        # while `:noub` needs to be true for all input values.
+        # However, as a special exception, we do allow literal `:boundscheck`.
+        # `:noub` will be tainted in any caller using `@inbounds`
+        # based on the `:noinbounds` effect.
+        # N.B. We do not taint for `--check-bounds=no` here.
+        # That is handled in concrete evaluation.
+        noub = ALWAYS_FALSE
     end
     if hasintersect(widenconst(obj), Module)
         # Modeled more precisely in abstract_eval_getglobal
@@ -2940,7 +2939,11 @@ function intrinsic_effects(f::IntrinsicFunction, argtypes::Vector{Any})
     effect_free = !(f === Intrinsics.pointerset) ? ALWAYS_TRUE : ALWAYS_FALSE
     nothrow = intrinsic_nothrow(f, argtypes)
     inaccessiblememonly = ALWAYS_TRUE
-    return Effects(EFFECTS_TOTAL; consistent, effect_free, nothrow, inaccessiblememonly)
+    noub = ALWAYS_TRUE
+    if f === Intrinsics.pointerref || f === Intrinsics.pointerset
+        noub = ALWAYS_FALSE
+    end
+    return Effects(EFFECTS_TOTAL; consistent, effect_free, nothrow, inaccessiblememonly, noub)
 end
 
 # TODO: this function is a very buggy and poor model of the return_type function

Compiler/test/effects.jl

@@ -443,13 +443,35 @@ end |> Compiler.is_consistent
 @test Base.infer_effects((Tuple{Int,Int},Int)) do t, i
     getfield(t, i, false)
 end |> Compiler.is_nothrow
+
+# getfield can throw when bounds checking is turned off when name is a symbol
 @test Base.infer_effects((Tuple{Int,Int},Symbol)) do t, i
     getfield(t, i, false)
-end |> Compiler.is_nothrow
+end |> !Compiler.is_nothrow
 @test Base.infer_effects((Tuple{Int,Int},String)) do t, i
     getfield(t, i, false) # invalid name type
 end |> !Compiler.is_nothrow
 
+# check for :noub tainted when bounds checking off
+let e = Base.infer_effects((Tuple{Int, Int}, Int)) do t, i
+        getfield(t, i)
+    end
+    @test !Compiler.is_nothrow(e)
+    @test Compiler.is_noub(e)
+end
+let e = Base.infer_effects((Tuple{Int, Int}, Int)) do t, i
+        getfield(t, i, false)
+    end
+    @test Compiler.is_nothrow(e)
+    @test !Compiler.is_noub(e)
+end
+let e = Base.infer_effects((Tuple{Int, Int}, Int)) do t, i
+        getfield(t, 2, false)
+    end
+    @test Compiler.is_nothrow(e)
+    @test Compiler.is_noub(e)
+end
+
 @test Base.infer_effects((Some{Any},)) do some
     getfield(some, 1, :not_atomic)
 end |> Compiler.is_nothrow
@@ -1349,6 +1371,15 @@ let; Base.Experimental.@force_compile; func52843(); end
 @test !Compiler.intrinsic_nothrow(Core.Intrinsics.pointerref, Any[Type{Ptr{Vector{Int64}}}, Int, Int])
 @test !Compiler.intrinsic_nothrow(Core.Intrinsics.pointerref, Any[Type{Ptr{T}} where T, Int, Int])
 
+let e = Base.infer_effects(Core.Intrinsics.pointerref, (Ptr{Int}, Int, Int))
+    @test !Compiler.is_noub(e)
+    @test !Compiler.is_consistent(e)
+end
+let e = Base.infer_effects(unsafe_load, (Ptr{Int},))
+    @test !Compiler.is_noub(e)
+    @test !Compiler.is_consistent(e)
+end
+
 # post-opt :consistent-cy analysis correctness
 # https://github.com/JuliaLang/julia/issues/53508
 @test !Compiler.is_consistent(Base.infer_effects(getindex, (UnitRange{Int},Int)))

Compiler/test/inference.jl

@@ -5915,25 +5915,25 @@ end == Val
 @test Base.infer_effects((Any,Any); optimize=false) do a, b
     getfield(PartiallyInitialized1(a, b), :b)
 end |> Compiler.is_nothrow
-@test Base.infer_effects((Any,Any,Symbol,); optimize=false) do a, b, f
+@test Base.infer_effects((Any,Any,Int,); optimize=false) do a, b, f
     getfield(PartiallyInitialized1(a, b), f, #=boundscheck=#false)
 end |> !Compiler.is_nothrow
 @test Base.infer_effects((Any,Any,Any); optimize=false) do a, b, c
     getfield(PartiallyInitialized1(a, b, c), :c)
 end |> Compiler.is_nothrow
-@test Base.infer_effects((Any,Any,Any,Symbol); optimize=false) do a, b, c, f
+@test Base.infer_effects((Any,Any,Any,Int); optimize=false) do a, b, c, f
     getfield(PartiallyInitialized1(a, b, c), f, #=boundscheck=#false)
 end |> Compiler.is_nothrow
 @test Base.infer_effects((Any,Any); optimize=false) do a, b
     getfield(PartiallyInitialized2(a, b), :b)
 end |> Compiler.is_nothrow
-@test Base.infer_effects((Any,Any,Symbol,); optimize=false) do a, b, f
+@test Base.infer_effects((Any,Any,Int,); optimize=false) do a, b, f
     getfield(PartiallyInitialized2(a, b), f, #=boundscheck=#false)
 end |> !Compiler.is_nothrow
 @test Base.infer_effects((Any,Any,Any); optimize=false) do a, b, c
     getfield(PartiallyInitialized2(a, b, c), :c)
 end |> Compiler.is_nothrow
-@test Base.infer_effects((Any,Any,Any,Symbol); optimize=false) do a, b, c, f
+@test Base.infer_effects((Any,Any,Any,Int); optimize=false) do a, b, c, f
     getfield(PartiallyInitialized2(a, b, c), f, #=boundscheck=#false)
 end |> Compiler.is_nothrow
 

base/docs/basedocs.jl

@@ -2435,14 +2435,19 @@ julia> Tuple(Real[1, 2, pi])  # takes a collection
 tuple
 
 """
-    getfield(value, name::Symbol, [order::Symbol])
-    getfield(value, i::Int, [order::Symbol])
-
-Extract a field from a composite `value` by name or position. Optionally, an
-ordering can be defined for the operation. If the field was declared `@atomic`,
-the specification is strongly recommended to be compatible with the stores to
-that location. Otherwise, if not declared as `@atomic`, this parameter must be
-`:not_atomic` if specified.
+    getfield(value, name::Symbol, [boundscheck::Bool=true], [order::Symbol])
+    getfield(value, i::Int, [boundscheck::Bool=true], [order::Symbol])
+
+Extract a field from a composite `value` by name or position.
+
+Optionally, an ordering can be defined for the operation.  If the field was
+declared `@atomic`, the specification is strongly recommended to be compatible
+with the stores to that location. Otherwise, if not declared as `@atomic`, this
+parameter must be `:not_atomic` if specified.
+
+The bounds check may be disabled, in which case the behavior of this function is
+undefined if `i` is out of bounds.
+
 See also [`getproperty`](@ref Base.getproperty) and [`fieldnames`](@ref).
 
 # Examples

base/docs/intrinsicsdocs.jl

@@ -22,6 +22,15 @@ The `Core.Intrinsics` module holds the `Core.IntrinsicFunction` objects.
 """
 Core.Intrinsics
 
+"""
+    Core.memorynew(::Type{T} where T <: GenericMemory, n::Int)
+
+Construct an uninitialized [`GenericMemory`](@ref) of length `n`.
+
+See also [`Memory`](@ref Base.Memory).
+"""
+Core.memorynew
+
 """
     Core.memoryrefnew(::GenericMemory)
     Core.memoryrefnew(::GenericMemoryRef, index::Int, [boundscheck::Bool])
@@ -129,6 +138,35 @@ See also [`setpropertyonce!`](@ref Base.replaceproperty!) and [`Core.memoryrefse
 """
 Core.memoryrefsetonce!
 
+
+"""
+    Core.Intrinsics.pointerref(p::Ptr{T}, i::Int, align::Int)
+
+Load a value of type `T` from the address of the `i`th element (1-indexed)
+starting at `p`. This is equivalent to the C expression `p[i-1]`.
+
+The alignment must be a power of two, or 0, indicating the default alignment
+for `T`. If `p[i-1]` is out of bounds, invalid, or is not aligned, the behavior
+is undefined. An alignment of 1 is always safe.
+
+See also [`unsafe_load`](@ref).
+"""
+Core.Intrinsics.pointerref
+
+"""
+    Core.Intrinsics.pointerset(p::Ptr{T}, x::T, i::Int, align::Int)
+
+Store a value of type `T` to the address of the `i`th element (1-indexed)
+starting at `p`.  This is equivalent to the C expression `p[i-1] = x`.
+
+The alignment must be a power of two, or `0`, indicating the default alignment
+for `T`. If `p[i-1]` is out of bounds, invalid, or is not aligned, the behavior
+is undefined. An alignment of 1 is always safe.
+
+See also [`unsafe_store!`](@ref).
+"""
+Core.Intrinsics.pointerset
+
 """
     Core.Intrinsics.atomic_pointerref(pointer::Ptr{T}, order::Symbol) --> T
 

doc/src/devdocs/builtins.md

@@ -1,12 +1,25 @@
 # [Core.Builtins](@id lib-builtins)
 
-## Builtin Function APIs
+The following builtin functions are considered unstable, but provide the basic
+definitions for what defines the abilities and behaviors of a Julia
+program. They are typically accessed through a higher level generic API.
 
-The following Builtin function APIs are considered unstable, but provide the basic
-definitions for what defines the abilities and behaviors of a Julia program. They are
-typically accessed through a higher level generic API.
+## Raw access to memory
 
 ```@docs
+Core.Intrinsics.pointerref
+Core.Intrinsics.pointerset
+Core.Intrinsics.atomic_pointerref
+Core.Intrinsics.atomic_pointerset
+Core.Intrinsics.atomic_pointerswap
+Core.Intrinsics.atomic_pointermodify
+Core.Intrinsics.atomic_pointerreplace
+```
+
+## Managed memory
+
+```@docs
+Core.memorynew
 Core.memoryrefnew
 Core.memoryrefoffset
 Core.memoryrefget
@@ -16,12 +29,15 @@ Core.memoryrefswap!
 Core.memoryrefmodify!
 Core.memoryrefreplace!
 Core.memoryrefsetonce!
-Core.Intrinsics.atomic_pointerref
-Core.Intrinsics.atomic_pointerset
-Core.Intrinsics.atomic_pointerswap
-Core.Intrinsics.atomic_pointermodify
-Core.Intrinsics.atomic_pointerreplace
+```
+
+## Module bindings
+
 Core.get_binding_type
+
+## Other
+
+```@docs
 Core.IntrinsicFunction
 Core.Intrinsics
 Core.IR