Revising methods with external method tables fails #646

maleadt · 2021-08-24T14:51:32Z

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.

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

timholy · 2021-08-24T22:01:28Z

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 · 2021-08-25T06:08:43Z

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 · 2021-08-25T06:21:29Z

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 · 2021-08-25T06:44:26Z

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 · 2022-07-24T13:47:22Z

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

maleadt · 2022-07-25T08:49:35Z

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 · 2024-04-24T17:05:15Z

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 mentioned this issue Aug 25, 2021

Don't embed the method table in the AST. JuliaGPU/CUDA.jl#1112

Merged

timholy added the bug label Jan 7, 2024

maleadt mentioned this issue Apr 20, 2024

Mark more methods as device methods JuliaGPU/CUDA.jl#2336

Draft

vchuravy mentioned this issue Apr 24, 2024

Add test for Overlay Method Tables #815

Open

This was referenced Apr 15, 2025

Support external method tables in method evaluation JuliaDebug/JuliaInterpreter.jl#680

Merged

Correctly revise methods defined in external method tables #904

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revising methods with external method tables fails #646

Revising methods with external method tables fails #646

maleadt commented Aug 24, 2021

timholy commented Aug 24, 2021

maleadt commented Aug 25, 2021

timholy commented Aug 25, 2021

maleadt commented Aug 25, 2021

timholy commented Jul 24, 2022

maleadt commented Jul 25, 2022

vchuravy commented Apr 24, 2024

Revising methods with external method tables fails #646

Revising methods with external method tables fails #646

Comments

maleadt commented Aug 24, 2021

timholy commented Aug 24, 2021

maleadt commented Aug 25, 2021

timholy commented Aug 25, 2021

maleadt commented Aug 25, 2021

timholy commented Jul 24, 2022

maleadt commented Jul 25, 2022

vchuravy commented Apr 24, 2024