Revising methods with external method tables fails

[maleadt]

The error is from JuliaInterpreter:

┌ Error: Failed to revise /home/tim/Julia/pkg/CUDA/src/device/intrinsics/dynamic_parallelism.jl
│   exception =
│    TypeError: in typeassert, expected Union{Nothing, Bool, Symbol}, got a value of type Core.MethodTable
│    Stacktrace:
│     [1] identify_framemethod_calls(frame::JuliaInterpreter.Frame)
│       @ LoweredCodeUtils ~/Julia/depot/packages/LoweredCodeUtils/jZY56/src/signatures.jl:176
└ @ Revise ~/Julia/depot/packages/Revise/OgnOk/src/packagedef.jl:709

Some info from around the error:

msrc = CodeInfo(
    @ /home/tim/Julia/pkg/CUDA/src/device/intrinsics/dynamic_parallelism.jl:25 within `none`
1 ─      res = cudaDeviceSynchronize()
│   @ /home/tim/Julia/pkg/CUDA/src/device/intrinsics/dynamic_parallelism.jl:26 within `none`
│   %2 = res != CUDA.cudaSuccess
└──      goto #3 if not %2
    @ /home/tim/Julia/pkg/CUDA/src/device/intrinsics/dynamic_parallelism.jl:27 within `none`
2 ─ %4 = CUDA.CuDeviceError(res)
│   %5 = CUDA.throw_device_cuerror(%4)
└──      return %5
3 ─      return nothing
)

stmt = :($(Expr(:method, Core.MethodTable(...), %J4, CodeInfo(...))))

key = # 167 methods:
[1] atan(x::Float32, y::Float32) in CUDA at /home/tim/Julia/pkg/CUDA/src/device/intrinsics/math.jl:70
...
[167] mul24(x::Int32, y::Int32) in CUDA at /home/tim/Julia/pkg/CUDA/src/device/intrinsics/math.jl:329

key is CUDA.jl's external method table with all device-specific functions.

This is on Julia 1.7, with Revise 3.1.19 and JuliaInterpreter 0.8.21.

[timholy]

As I'm sure you can imagine, a reproducer would be lovely. What version of LoweredCodeUtils are you on? I assume that line is https://github.com/JuliaDebug/LoweredCodeUtils.jl/blob/ff8998f6151bce37bbf6a82ce72622e692fd8df3/src/signatures.jl#L175. Obviously I can extend the Union to include Core.MethodTable, but it would be nice to have confidence that everything else is right.

[maleadt]

Yeah, sorry, was going to put up some additional details, but wanted to make a note as soon as I ran into the issue 🙂

I was using LoweredCodeUtils v2.1.2, so yeah that was the line.

The basic reproducer is as follows:

Base.Experimental.@MethodTable(method_table)

foo() = 1
Base.Experimental.@overlay method_table foo() = 2

bar() = foo()

This fails to Revise.includet, but with a different failure:

KeyError: key :method_table not found
Stacktrace:
  [1] getindex(h::Dict{Symbol, LoweredCodeUtils.MethodInfo}, key::Symbol)
    @ Base ./dict.jl:481
  [2] identify_framemethod_calls(frame::JuliaInterpreter.Frame)
    @ LoweredCodeUtils ~/.julia/packages/LoweredCodeUtils/jZY56/src/signatures.jl:168
  [3] rename_framemethods!
    @ ~/.julia/packages/LoweredCodeUtils/jZY56/src/signatures.jl:307 [inlined]
  [4] methods_by_execution!(recurse::Any, methodinfo::Revise.CodeTrackingMethodInfo, docexprs::Dict{Module, Vector{Expr}}, mod::Module, ex::Expr; mode::Symbol, disablebp::Bool, always_rethrow::Bool, kwargs::Base.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:skip_include,), Tuple{Bool}}})
    @ Revise ~/.julia/packages/Revise/OgnOk/src/lowered.jl:191
  [5] #eval_with_signatures#92
    @ ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:463 [inlined]
  [6] instantiate_sigs!(modexsigs::OrderedCollections.OrderedDict{Module, OrderedCollections.OrderedDict{Revise.RelocatableExpr, Union{Nothing, Vector{Any}}}}; mode::Symbol, kwargs::Base.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:skip_include,), Tuple{Bool}}})
    @ Revise ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:471
  [7] track(mod::Module, file::String; mode::Symbol, kwargs::Base.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:skip_include,), Tuple{Bool}}})
    @ Revise ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:899
  [8] includet(mod::Module, file::String)
    @ Revise ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:994
  [9] includet(file::String)
    @ Revise ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:1016

The reason that the error is slightly different, is that in CUDA.jl/GPUCompiler.jl we use a wrapper macro that interpolates the method_table in the AST, so it isn't a Symbol but a MethodTable directly:

Base.Experimental.@MethodTable(method_table)

foo() = 1
macro override(ex)
    quote
        Base.Experimental.@overlay $method_table $ex
    end
end
@override foo() = 2

bar() = foo()

This yields the error reported above.

[timholy]

Does it work if you make it a GlobalRef instead? I can't find the reference now, but @aviatesk recently pointed out an older quote of Jeff Bezanson's effectively saying "we allow this, but it's perhaps a mis-feature, because Exprs should be close to the source code itself."

It will still throw a Revise error (GlobalRef is not currently in that Union), but that's very fixable.

[maleadt]

A GlobalRef works for Julia, but triggers another error in the Revise stack:

macro override(ex)
    quote
        Base.Experimental.@overlay $(GlobalRef(@__MODULE__, :method_table)) $ex
    end
end

MethodError: no method matching nameof(::Core.MethodTable)
Closest candidates are:
  nameof(::DataType) at reflection.jl:223
  nameof(::UnionAll) at reflection.jl:224
  nameof(::Module) at reflection.jl:16
  ...
Stacktrace:
  [1] normalize_defsig(def::Any, frame::JuliaInterpreter.Frame)
    @ LoweredCodeUtils ~/.julia/packages/LoweredCodeUtils/jZY56/src/signatures.jl:217
  [2] identify_framemethod_calls(frame::JuliaInterpreter.Frame)
    @ LoweredCodeUtils ~/.julia/packages/LoweredCodeUtils/jZY56/src/signatures.jl:166
  [3] rename_framemethods!
    @ ~/.julia/packages/LoweredCodeUtils/jZY56/src/signatures.jl:307 [inlined]
  [4] methods_by_execution!(recurse::Any, methodinfo::Revise.CodeTrackingMethodInfo, docexprs::Dict{Module, Vector{Expr}}, mod::Module, ex::Expr; mode::Symbol, disablebp::Bool, always_rethrow::Bool, kwargs::Base.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:skip_include,), Tuple{Bool}}})
    @ Revise ~/.julia/packages/Revise/OgnOk/src/lowered.jl:191
  [5] #eval_with_signatures#92
    @ ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:463 [inlined]
  [6] instantiate_sigs!(modexsigs::OrderedCollections.OrderedDict{Module, OrderedCollections.OrderedDict{Revise.RelocatableExpr, Union{Nothing, Vector{Any}}}}; mode::Symbol, kwargs::Base.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:skip_include,), Tuple{Bool}}})
    @ Revise ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:471
  [7] track(mod::Module, file::String; mode::Symbol, kwargs::Base.Pairs{Symbol, Bool, Tuple{Symbol}, NamedTuple{(:skip_include,), Tuple{Bool}}})
    @ Revise ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:899
  [8] includet(mod::Module, file::String)
    @ Revise ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:994
  [9] includet(file::String)
    @ Revise ~/.julia/packages/Revise/OgnOk/src/packagedef.jl:1016

But why a GlobalRef? Wouldn't source code typically contain a Symbol or Expr? Changing the method_table to an Expr seems 'compatible' (i.e. it doesn't error) with the current logic in LoweredCodeUtils:

macro override(ex)
    quote
        Base.Experimental.@overlay $(@__MODULE__).method_table $ex
    end
end

Similarly, without the wrapper macro, making sure the method table is an Expr (by qualifying the module) gets past Revise.includet:

Base.Experimental.@MethodTable(method_table)

foo() = 1
Base.Experimental.@overlay Main.method_table foo() = 2

So I guess this only needs a fix for the method_table::Symbol case, and if we want to, for when the method_table is actually a full-blown Core.MethodTable.

---

I quoted 'compatible' above, because even though Revise doesn't choke on these sources anymore, it incorrectly redefines methods. I guess that's the second part of this issue.

Expected behavior:

julia> Base.Experimental.@MethodTable(method_table)

julia> foo() = 1
julia> Base.Experimental.@overlay Main.method_table foo() = 2

julia> foo()
1

julia> Base.Experimental.@overlay Main.method_table foo() = 3

julia> foo()
1

Whereas putting this in a file and having Revise track it yields the following:

julia> using Revise

julia> Revise.includet("wip.jl")

julia> foo()
1

# change @overlay foo()=2 to =3
julia> Revise.revise()

julia> foo()
3

[timholy]

Is this still relevant or did the PR above fix it?

[maleadt]

Yes, this is still relevant. The PR just made it so that Revise doesn't error out, but it doesn't correctly handle overlay method tables. Let me demonstrate:

Base.Experimental.@MethodTable(method_table)

foo() = 1
Base.Experimental.@overlay Main.method_table foo() = 2

function main()
    println("Method in main method table:")
    methods = Base._methods_by_ftype(Tuple{typeof(foo)}, nothing, 1, Base.get_world_counter())
    display(Base.uncompressed_ir(methods[1].method))

    println("Method in overlay method table:")
    methods = Base._methods_by_ftype(Tuple{typeof(foo)}, method_table, 1, Base.get_world_counter())
    display(Base.uncompressed_ir(methods[1].method))

    return
end

julia> using Revise

julia> Revise.includet("...")

julia> main()
Method in main method table:
CodeInfo(
    @ /tmp/wip.jl:3 within `foo`
1 ─     return 1
)
Method in overlay method table:
CodeInfo(
    @ /tmp/wip.jl:4 within `#foo`
1 ─     return 2
)

julia> # redefine `@overlay foo() = 2` to `@overlay foo() = 3`

julia> main()
Method in main method table:
CodeInfo(
    @ /tmp/wip.jl:4 within `foo`
1 ─     return 3
)
Method in overlay method table:
CodeInfo(
    @ /tmp/wip.jl:4 within `#foo`
1 ─     return 2
)

i.e. revising the overlaid method results in the main method table method being redefined, instead of the one on the overlay method table.

[vchuravy]

I tried to investigate this a bit:

julia> using CodeTracking

julia> Base.Experimental.@MethodTable(method_table)
# 0 methods for callable object

julia> foo() = 1
foo (generic function with 1 method)

julia> Base.Experimental.@overlay Main.method_table foo() = 2

julia> methods = Base._methods_by_ftype(Tuple{typeof(foo)}, nothing, 1, Base.get_world_counter())
julia> CodeTracking.definition(String, methods[1].method)
("foo() = 1", 1)

julia> methods = Base._methods_by_ftype(Tuple{typeof(foo)}, method_table, 1, Base.get_world_counter())
julia> CodeTracking.definition(String, methods[1].method)

Don't know how big of a problem this is.

On the other hand if I print here:

https://github.com/vchuravy/Revise.jl/blob/5c6015edcb6efb849d371ae73f603c44fcfdbe06/src/lowered.jl#L399-L400

add_signature!(methodinfo, sig, lnn) = Revise.CodeTrackingMethodInfo(Expr[quote
    #= /tmp/mPP2zGfmrJ/overlaymt.jl:3 =#
    foo_mt() = begin
            #= /tmp/mPP2zGfmrJ/overlaymt.jl:3 =#
            1
        end
end], Any[Tuple{typeof(foo_mt)}], Set{Union{GlobalRef, Symbol}}(), Pair{Module, String}[])
add_signature!(methodinfo, sig, lnn) = Revise.CodeTrackingMethodInfo(Expr[quote
    #= /tmp/mPP2zGfmrJ/overlaymt.jl:4 =#
    #= /tmp/mPP2zGfmrJ/overlaymt.jl:4 =# Base.Experimental.@overlay Main.method_table foo_mt() = begin
                #= /tmp/mPP2zGfmrJ/overlaymt.jl:4 =#
                2
            end
end], Any[Tuple{typeof(foo_mt)}], Set{Union{GlobalRef, Symbol}}(), Pair{Module, String}[])

[maleadt]

🎉