trimming: follow-ups to finalizer support (#58311)

Should cover all the post-review comments from
#58014

~~Also disables the eager compilation of finalizers unless `--trim` is
enabled, but I'm happy to re-enable that.~~ edit: re-enabled

Author: topolarity

Compiler/src/typeinfer.jl

@@ -1611,29 +1611,28 @@ const TRIM_UNSAFE = 2
 const TRIM_UNSAFE_WARN = 3
 function typeinf_ext_toplevel(methods::Vector{Any}, worlds::Vector{UInt}, trim_mode::Int)
     inf_params = InferenceParams(; force_enable_inference = trim_mode != TRIM_NO)
+
+    # Create an "invokelatest" queue to enable eager compilation of speculative
+    # invokelatest calls such as from `Core.finalizer` and `ccallable`
     invokelatest_queue = CompilationQueue(;
         interp = NativeInterpreter(get_world_counter(); inf_params)
     )
+
     codeinfos = []
-    is_latest_world = true # whether this_world == world_counter()
     workqueue = CompilationQueue(; interp = nothing)
     for this_world in reverse!(sort!(worlds))
         workqueue = CompilationQueue(workqueue;
             interp = NativeInterpreter(this_world; inf_params)
         )
 
         append!(workqueue, methods)
-        if is_latest_world
-            # Provide the `invokelatest` queue so that we trigger "best-effort" code generation
-            # for, e.g., finalizers and cfunction.
-            #
-            # The queue is intentionally aliased, to handle e.g. a `finalizer` calling `Core.finalizer`
-            # (it will enqueue into itself and immediately drain)
-            compile!(codeinfos, workqueue; invokelatest_queue = workqueue)
-        else
-            compile!(codeinfos, workqueue)
-        end
-        is_latest_world = false
+        compile!(codeinfos, workqueue; invokelatest_queue)
+    end
+
+    if invokelatest_queue !== nothing
+        # This queue is intentionally aliased, to handle e.g. a `finalizer` calling `Core.finalizer`
+        # (it will enqueue into itself and immediately drain)
+        compile!(codeinfos, invokelatest_queue; invokelatest_queue)
     end
 
     if trim_mode != TRIM_NO && trim_mode != TRIM_UNSAFE

Compiler/src/verifytrim.jl

@@ -15,9 +15,9 @@ using ..Compiler:
      hasintersect, haskey, in, isdispatchelem, isempty, isexpr, iterate, length, map!, max,
      pop!, popfirst!, push!, pushfirst!, reinterpret, reverse!, reverse, setindex!,
      setproperty!, similar, singleton_type, sptypes_from_meth_instance,
-     unsafe_pointer_to_objref, widenconst,
+     unsafe_pointer_to_objref, widenconst, isconcretetype,
      # misc
-     @nospecialize, C_NULL
+     @nospecialize, @assert, C_NULL
 using ..IRShow: LineInfoNode, print, show, println, append_scopes!, IOContext, IO, normalize_method_name
 using ..Base: Base, sourceinfo_slotnames
 using ..Base.StackTraces: StackFrame
@@ -166,7 +166,7 @@ function may_dispatch(@nospecialize ftyp)
     end
 end
 
-function verify_codeinstance!(codeinst::CodeInstance, codeinfo::CodeInfo, inspected::IdSet{CodeInstance}, caches::IdDict{MethodInstance,CodeInstance}, parents::ParentMap, errors::ErrorList)
+function verify_codeinstance!(interp::NativeInterpreter, codeinst::CodeInstance, codeinfo::CodeInfo, inspected::IdSet{CodeInstance}, caches::IdDict{MethodInstance,CodeInstance}, parents::ParentMap, errors::ErrorList)
     mi = get_ci_mi(codeinst)
     sptypes = sptypes_from_meth_instance(mi)
     src = codeinfo.code
@@ -199,9 +199,9 @@ function verify_codeinstance!(codeinst::CodeInstance, codeinfo::CodeInfo, inspec
                 if !may_dispatch(ftyp)
                     continue
                 end
-                # TODO: Make interp elsewhere
-                interp = NativeInterpreter(Base.get_world_counter())
-                if Core._apply_iterate isa ftyp
+                if !isconcretetype(ftyp)
+                    error = "unresolved call to (unknown) builtin"
+                elseif Core._apply_iterate isa ftyp
                     if length(stmt.args) >= 3
                         # args[1] is _apply_iterate object
                         # args[2] is invoke object
@@ -233,9 +233,23 @@ function verify_codeinstance!(codeinst::CodeInstance, codeinfo::CodeInfo, inspec
 
                         error = "unresolved finalizer registered"
                     end
-                else
-                    error = "unresolved call to builtin"
-                end
+                elseif Core._apply isa ftyp
+                    error = "trim verification not yet implemented for builtin `Core._apply`"
+                elseif Core._call_in_world_total isa ftyp
+                    error = "trim verification not yet implemented for builtin `Core._call_in_world_total`"
+                elseif Core.invoke isa ftyp
+                    error = "trim verification not yet implemented for builtin `Core.invoke`"
+                elseif Core.invoke_in_world isa ftyp
+                    error = "trim verification not yet implemented for builtin `Core.invoke_in_world`"
+                elseif Core.invokelatest isa ftyp
+                    error = "trim verification not yet implemented for builtin `Core.invokelatest`"
+                elseif Core.modifyfield! isa ftyp
+                    error = "trim verification not yet implemented for builtin `Core.modifyfield!`"
+                elseif Core.modifyglobal! isa ftyp
+                    error = "trim verification not yet implemented for builtin `Core.modifyglobal!`"
+                elseif Core.memoryrefmodify! isa ftyp
+                    error = "trim verification not yet implemented for builtin `Core.memoryrefmodify!`"
+                else @assert false "unexpected builtin" end
             end
             extyp = argextype(SSAValue(i), codeinfo, sptypes)
             if extyp === Union{}
@@ -267,6 +281,7 @@ end
 
 function get_verify_typeinf_trim(codeinfos::Vector{Any})
     this_world = get_world_counter()
+    interp = NativeInterpreter(this_world)
     inspected = IdSet{CodeInstance}()
     caches = IdDict{MethodInstance,CodeInstance}()
     errors = ErrorList()
@@ -287,7 +302,7 @@ function get_verify_typeinf_trim(codeinfos::Vector{Any})
         item = codeinfos[i]
         if item isa CodeInstance
             src = codeinfos[i + 1]::CodeInfo
-            verify_codeinstance!(item, src, inspected, caches, parents, errors)
+            verify_codeinstance!(interp, item, src, inspected, caches, parents, errors)
         elseif item isa SimpleVector
             rt = item[1]::Type
             sig = item[2]::Type

Compiler/test/verifytrim.jl

@@ -18,6 +18,24 @@ let infos = Any[]
     @test isempty(parents)
 end
 
+finalizer(@nospecialize(f), @nospecialize(o)) = Core.finalizer(f, o)
+
+let infos = typeinf_ext_toplevel(Any[Core.svec(Nothing, Tuple{typeof(finalizer), typeof(identity), Any})], [Base.get_world_counter()], TRIM_UNSAFE)
+    errors, parents = get_verify_typeinf_trim(infos)
+    @test !isempty(errors) # unresolvable finalizer
+
+    # the only error should be a CallMissing error for the Core.finalizer builtin
+    (warn, desc) = only(errors)
+    @test !warn
+    @test desc isa CallMissing
+    @test occursin("finalizer", desc.desc)
+    repr = sprint(verify_print_error, desc, parents)
+    @test occursin(
+        r"""^unresolved finalizer registered from statement \(Core.finalizer\)\(f::Any, o::Any\)::Nothing
+            Stacktrace:
+             \[1\] finalizer\(f::Any, o::Any\)""", repr)
+end
+
 make_cfunction() = @cfunction(+, Float64, (Int64,Int64))
 
 # use TRIM_UNSAFE to bypass verifier inside typeinf_ext_toplevel

HISTORY.md

@@ -5,8 +5,11 @@ New language features
 ---------------------
 
 * New option `--trim` creates smaller binaries by removing code that was not proven to be reachable from
-  entry points. Entry points can be marked using `Base.Experimental.entrypoint` ([#55047]).
-* Redefinition of constants is now well defined and follows world age semantics. Additional redefinitions (e.g. of structs) are now allowed. See [the new manual chapter on world age](https://docs.julialang.org/en/v1.13-dev/manual/worldage/).
+  entry points. Entry points can be marked using `Base.Experimental.entrypoint` ([#55047]). To support
+  Core.finalizer, inference will now opportunistically discover future invokelatest calls and compile
+  the required code for them.
+* Redefinition of constants is now well defined and follows world age semantics. Additional redefinitions
+  (e.g. of structs) are now allowed. See [the new manual chapter on world age](https://docs.julialang.org/en/v1.13-dev/manual/worldage/).
 * A new keyword argument `usings::Bool` has been added to `names`, returning all names visible
   via `using` ([#54609]).
 * The `@atomic` macro family now supports reference assignment syntax, e.g. `@atomic :monotonic v[3] += 4`,