proper termination, take 2 (#117)

This PR is an alternative to #99. This is
built on top of #116.

With this PR, the following test cases now pass correctly: 
```julia
#Final block is not a `return`: Need to use
#`config::SelectiveEvalRecurse` explicitly
ex = quote
    x = 1
    yy = 7
    @Label loop x += 1
    x < 5 || return yy
    @goto loop
end 
frame = Frame(ModSelective, ex)
src = frame.framecode.src
edges = CodeEdges(ModSelective, src)
config = SelectiveEvalRecurse()
isrequired = lines_required(GlobalRef(ModSelective, :x), src, edges, config)
selective_eval_fromstart!(config, frame, isrequired, true)
@test ModSelective.x == 5
@test !isdefined(ModSelective, :yy)
```

The basic approach is overloading `JuliaInterpreter.step_expr!` and
`LoweredCodeUtils.next_or_nothing!` for the new
`SelectiveEvalController` type, as described below, to perform correct
selective execution.

When `SelectiveEvalController` is passed as the `recurse` argument of
`selective_eval!`, the selective execution is adjusted as follows:

- **Implicit return**: In Julia's IR representation (`CodeInfo`), the
final block does not necessarily return and may `goto` another block.
And if the `return` statement is not included in the slice in such
cases, it is necessary to terminate `selective_eval!` when execution
reaches such implicit return statements. `controller.implicit_returns`
records the PCs of such return statements, and `selective_eval!` will
return when reaching those statements. This is the core part of the fix
for the test cases in #99.

- **CFG short-cut**: When the successors of a conditional branch are
inactive, and it is safe to move the program counter from the
conditional branch to the nearest common post-dominator of those
successors, this short-cut is taken. This short-cut is not merely an
optimization but is actually essential for the correctness of the
selective execution. This is because, in `CodeInfo`, even if we simply
fall-through dead blocks (i.e., increment the program counter without
executing the statements of those blocks), it does not necessarily lead
to the nearest common post-dominator block.

And now [`lines_required`](@ref) or [`lines_required!`](@ref) will
update the `SelectiveEvalController` passed as their argument to be
appropriate for the program slice generated.

One thing to note is that currently, the `controller` is not be
recursed. That said, in Revise, which is the main consumer of LCU, there
is no need for recursive selective execution, and so `selective_eval!`
does not provide a system for inter-procedural selective evaluation.
Accordingly `SelectiveEvalController` does not recurse too, but this can
be left as a future extension.

Co-authored-by: Tim Holy <tim.holy@gmail.com>

Author: aviatesk

Project.toml

@@ -1,6 +1,6 @@
 name = "LoweredCodeUtils"
 uuid = "6f1432cf-f94c-5a45-995e-cdbf5db27b0b"
-version = "3.5.3"
+version = "3.6.0"
 authors = ["Tim Holy <tim.holy@gmail.com>"]
 
 [deps]

docs/src/api.md

@@ -17,6 +17,7 @@ lines_required
 lines_required!
 selective_eval!
 selective_eval_fromstart!
+SelectiveEvalController
 ```
 
 ## Interpreter

src/codeedges.jl

@@ -182,6 +182,60 @@ function postprint_linelinks(io::IO, idx::Int, src::CodeInfo, cl::CodeLinks, bbc
     return nothing
 end
 
+struct CFGShortCut
+    from::Int # pc of GotoIfNot with inactive 𝑰𝑵𝑭𝑳 blocks
+    to::Int   # pc of the entry of the nearest common post-dominator of the GotoIfNot's successors
+end
+
+"""
+    controller::SelectiveEvalController
+
+When this object is passed as the `recurse` argument of `selective_eval!`,
+the selective execution is adjusted as follows:
+
+- **Implicit return**: In Julia's IR representation (`CodeInfo`), the final block does not
+  necessarily return and may `goto` another block. In such cases the actual `return` is an
+  explicit statement earlier in the code, and if the slice does not include it,
+  `selective_eval!` must still terminate when execution reaches it. `controller.implicit_returns`
+  records the PCs of such `return` statements, and `selective_eval!` will return when reaching them.
+
+- **CFG short-cut**: When the successors of a conditional branch are inactive, and it is
+  safe to move the program counter from the conditional branch to the nearest common
+  post-dominator of those successors, this short-cut is taken.
+  This short-cut is not merely an optimization but is actually essential for the correctness
+  of the selective execution. This is because, in `CodeInfo`, even if we simply fall-through
+  dead blocks (i.e., increment the program counter without executing the statements of those
+  blocks), it does not necessarily lead to the nearest common post-dominator block.
+
+These adjustments are necessary for performing selective execution correctly.
+[`lines_required`](@ref) or [`lines_required!`](@ref) will update the `SelectiveEvalController`
+passed as an argument to be appropriate for the program slice generated.
+"""
+struct SelectiveEvalController
+    implicit_returns::BitSet    # pc where selective execution should terminate even if they're inactive
+    shortcuts::Vector{CFGShortCut}
+end
+SelectiveEvalController() = SelectiveEvalController(BitSet(), CFGShortCut[])
+
+"""
+    struct SelectiveInterpreter{S<:Interpreter,T<:AbstractVector{Bool}} <: Interpreter
+        inner::S
+        isrequired::T
+        controller::SelectiveEvalController
+    end
+
+An `JuliaInterpreter.Interpreter` that executes only the statements marked `true` in `isrequired`.
+Note that this interpreter does not recurse into callee frames.
+That is, when `JuliaInterpreter.finish!(interp::SelectiveInterpreter, frame, ...)` is
+performed, the `frame` will be executed selectively according to `interp.isrequired`, but
+any callee frames within it will be executed by `interp.inner::Interpreter`, not by `interp`.
+"""
+struct SelectiveInterpreter{S<:Interpreter,T<:AbstractVector{Bool}} <: Interpreter
+    inner::S
+    isrequired::T
+    controller::SelectiveEvalController
+end
+
 function namedkeys(cl::CodeLinks)
     ukeys = Set{GlobalRef}()
     for c in (cl.namepreds, cl.namesuccs, cl.nameassigns)
@@ -606,8 +660,8 @@ function terminal_preds!(s, j, edges, covered)  # can't be an inner function bec
 end
 
 """
-    isrequired = lines_required(obj::GlobalRef, src::CodeInfo, edges::CodeEdges)
-    isrequired = lines_required(idx::Int,                     src::CodeInfo, edges::CodeEdges)
+    isrequired = lines_required(obj::GlobalRef, src::CodeInfo, edges::CodeEdges, [controller::SelectiveEvalController])
+    isrequired = lines_required(idx::Int,       src::CodeInfo, edges::CodeEdges, [controller::SelectiveEvalController])
 
 Determine which lines might need to be executed to evaluate `obj` or the statement indexed by `idx`.
 If `isrequired[i]` is `false`, the `i`th statement is *not* required.
@@ -616,21 +670,26 @@ will end up skipping a subset of such statements, perhaps while repeating others
 
 See also [`lines_required!`](@ref) and [`selective_eval!`](@ref).
 """
-function lines_required(obj::GlobalRef, src::CodeInfo, edges::CodeEdges; kwargs...)
+function lines_required(obj::GlobalRef, src::CodeInfo, edges::CodeEdges,
+                        controller::SelectiveEvalController=SelectiveEvalController();
+                        kwargs...)
     isrequired = falses(length(edges.preds))
     objs = Set{GlobalRef}([obj])
-    return lines_required!(isrequired, objs, src, edges; kwargs...)
+    return lines_required!(isrequired, objs, src, edges, controller; kwargs...)
 end
 
-function lines_required(idx::Int, src::CodeInfo, edges::CodeEdges; kwargs...)
+function lines_required(idx::Int, src::CodeInfo, edges::CodeEdges,
+                        controller::SelectiveEvalController=SelectiveEvalController();
+                        kwargs...)
     isrequired = falses(length(edges.preds))
     isrequired[idx] = true
     objs = Set{GlobalRef}()
-    return lines_required!(isrequired, objs, src, edges; kwargs...)
+    return lines_required!(isrequired, objs, src, edges, controller; kwargs...)
 end
 
 """
-    lines_required!(isrequired::AbstractVector{Bool}, src::CodeInfo, edges::CodeEdges;
+    lines_required!(isrequired::AbstractVector{Bool}, src::CodeInfo, edges::CodeEdges,
+                    [controller::SelectiveEvalController];
                     norequire = ())
 
 Like `lines_required`, but where `isrequired[idx]` has already been set to `true` for all statements
@@ -642,9 +701,11 @@ should _not_ be marked as a requirement.
 For example, use `norequire = LoweredCodeUtils.exclude_named_typedefs(src, edges)` if you're
 extracting method signatures and not evaluating new definitions.
 """
-function lines_required!(isrequired::AbstractVector{Bool}, src::CodeInfo, edges::CodeEdges; kwargs...)
+function lines_required!(isrequired::AbstractVector{Bool}, src::CodeInfo, edges::CodeEdges,
+                         controller::SelectiveEvalController=SelectiveEvalController();
+                         kwargs...)
     objs = Set{GlobalRef}()
-    return lines_required!(isrequired, objs, src, edges; kwargs...)
+    return lines_required!(isrequired, objs, src, edges, controller; kwargs...)
 end
 
 function exclude_named_typedefs(src::CodeInfo, edges::CodeEdges)
@@ -664,7 +725,9 @@ function exclude_named_typedefs(src::CodeInfo, edges::CodeEdges)
     return norequire
 end
 
-function lines_required!(isrequired::AbstractVector{Bool}, objs, src::CodeInfo, edges::CodeEdges; norequire = ())
+function lines_required!(isrequired::AbstractVector{Bool}, objs, src::CodeInfo, edges::CodeEdges,
+                         controller::SelectiveEvalController=SelectiveEvalController();
+                         norequire = ())
     # Mark any requested objects (their lines of assignment)
     objs = add_requests!(isrequired, objs, edges, norequire)
 
@@ -699,7 +762,10 @@ function lines_required!(isrequired::AbstractVector{Bool}, objs, src::CodeInfo,
     end
 
     # now mark the active goto nodes
-    add_active_gotos!(isrequired, src, cfg, postdomtree)
+    add_active_gotos!(isrequired, src, cfg, postdomtree, controller)
+
+    # check if there are any implicit return blocks
+    record_implicit_return!(controller, isrequired, cfg)
 
     return isrequired
 end
@@ -777,19 +843,19 @@ end
 
 ## Add control-flow
 
-
 # The goal of this function is to request concretization of the minimal necessary control
 # flow to evaluate statements whose concretization have already been requested.
 # The basic algorithm is based on what was proposed in [^Wei84]. If there is even one active
 # block in the blocks reachable from a conditional branch up to its successors' nearest
 # common post-dominator (referred to as 𝑰𝑵𝑭𝑳 in the paper), it is necessary to follow
-# that conditional branch and execute the code. Otherwise, execution can be short-circuited
+# that conditional branch and execute the code. Otherwise, execution can be short-cut
 # from the conditional branch to the nearest common post-dominator.
 #
-# COMBAK: It is important to note that in Julia's intermediate code representation (`CodeInfo`),
-# "short-circuiting" a specific code region is not a simple task. Simply ignoring the path
-# to the post-dominator does not guarantee fall-through to the post-dominator. Therefore,
-# a more careful implementation is required for this aspect.
+# It is important to note that in Julia's intermediate code representation (`CodeInfo`),
+# "short-cutting" a specific code region is not a simple task. Simply incrementing the
+# program counter without executing the statements of 𝑰𝑵𝑭𝑳 blocks does not guarantee that
+# the program counter fall-throughs to the post-dominator.
+# To handle such cases, `selective_eval!` needs to use `SelectiveInterpreter`.
 #
 # [Wei84]: M. Weiser, "Program Slicing," IEEE Transactions on Software Engineering, 10, pages 352-357, July 1984.
 function add_control_flow!(isrequired, src::CodeInfo, cfg::CFG, postdomtree)
@@ -864,8 +930,8 @@ function reachable_blocks(cfg, from_bb::Int, to_bb::Int)
     return visited
 end
 
-function add_active_gotos!(isrequired, src::CodeInfo, cfg::CFG, postdomtree)
-    dead_blocks = compute_dead_blocks(isrequired, src, cfg, postdomtree)
+function add_active_gotos!(isrequired, src::CodeInfo, cfg::CFG, postdomtree, controller::SelectiveEvalController)
+    dead_blocks = compute_dead_blocks!(isrequired, src, cfg, postdomtree, controller)
     changed = false
     for bbidx = 1:length(cfg.blocks)
         if bbidx ∉ dead_blocks
@@ -883,7 +949,7 @@ function add_active_gotos!(isrequired, src::CodeInfo, cfg::CFG, postdomtree)
 end
 
 # find dead blocks using the same approach as `add_control_flow!`, for the converged `isrequired`
-function compute_dead_blocks(isrequired, src::CodeInfo, cfg::CFG, postdomtree)
+function compute_dead_blocks!(isrequired, src::CodeInfo, cfg::CFG, postdomtree, controller::SelectiveEvalController)
     dead_blocks = BitSet()
     for bbidx = 1:length(cfg.blocks)
         bb = cfg.blocks[bbidx]
@@ -904,13 +970,31 @@ function compute_dead_blocks(isrequired, src::CodeInfo, cfg::CFG, postdomtree)
                 end
                 if !is_𝑰𝑵𝑭𝑳_active
                     union!(dead_blocks, delete!(𝑰𝑵𝑭𝑳, postdominator))
+                    if postdominator ≠ 0
+                        postdominator_bb = cfg.blocks[postdominator]
+                        postdominator_entryidx = postdominator_bb.stmts[begin]
+                        push!(controller.shortcuts, CFGShortCut(termidx, postdominator_entryidx))
+                    end
                 end
             end
         end
     end
     return dead_blocks
 end
 
+function record_implicit_return!(controller::SelectiveEvalController, isrequired, cfg::CFG)
+    for bbidx = 1:length(cfg.blocks)
+        bb = cfg.blocks[bbidx]
+        if isempty(bb.succs)
+            i = findfirst(idx::Int->!isrequired[idx], bb.stmts)
+            if !isnothing(i)
+                push!(controller.implicit_returns, bb.stmts[i])
+            end
+        end
+    end
+    nothing
+end
+
 # Do a traveral of "numbered" predecessors and find statement ranges and names of type definitions
 function find_typedefs(src::CodeInfo)
     typedef_blocks, typedef_names = UnitRange{Int}[], Symbol[]
@@ -1033,26 +1117,19 @@ function add_inplace!(isrequired, src, edges, norequire)
     return changed
 end
 
-"""
-    struct SelectiveInterpreter{S<:Interpreter,T<:AbstractVector{Bool}} <: Interpreter
-        inner::S
-        isrequired::T
-    end
-
-An `JuliaInterpreter.Interpreter` that executes only the statements marked `true` in `isrequired`.
-Note that this interpreter does not recurse into callee frames.
-That is, when `JuliaInterpreter.finish!(interp::SelectiveInterpreter, frame, ...)` is
-performed, the `frame` will be executed selectively according to `interp.isrequired`, but
-any callee frames within it will be executed by `interp.inner::Interpreter`, not by `interp`.
-"""
-struct SelectiveInterpreter{S<:Interpreter,T<:AbstractVector{Bool}} <: Interpreter
-    inner::S
-    isrequired::T
-end
 function JuliaInterpreter.step_expr!(interp::SelectiveInterpreter, frame::Frame, istoplevel::Bool)
     pc = frame.pc
+    if pc in interp.controller.implicit_returns
+        return nothing
+    elseif pc_expr(frame) isa GotoIfNot
+        for shortcut in interp.controller.shortcuts
+            if shortcut.from == pc
+                return frame.pc = shortcut.to
+            end
+        end
+    end
     if interp.isrequired[pc]
-        step_expr!(interp.inner, frame::Frame, istoplevel::Bool)
+        step_expr!(interp.inner, frame, istoplevel)
     else
         next_or_nothing!(interp, frame)
     end
@@ -1083,12 +1160,28 @@ See [`selective_eval_fromstart!`](@ref) to have that performed automatically.
 
 `isrequired` pertains only to `frame` itself, not any of its callees.
 
+By default `selective_eval!` runs with an empty [`SelectiveEvalController`](@ref), which
+reproduces the historical behavior. That is correct for most top-level code, but not for
+every possible Julia program: when the slice omits a `return` reached by fall-through, or
+omits a branch whose dead region does not simply fall through to its post-dominator, the
+interpreter can execute the wrong statements (see
+https://github.com/JuliaDebug/LoweredCodeUtils.jl/pull/99 for details). For full correctness,
+pass a [`SelectiveEvalController`](@ref) to [`lines_required`](@ref)/[`lines_required!`](@ref)
+and reuse that same controller, together with the `isrequired` it produced, to construct a
+[`SelectiveInterpreter`](@ref) that you run with `JuliaInterpreter.finish_and_return!`.
+Drawing both from the same call is what keeps `isrequired` and the controller synchronized.
+
+Note that the interpreter does not recurse into callees, so there is currently no
+interprocedural selective evaluation.
+
 This will return either a `BreakpointRef`, the value obtained from the last executed statement
 (if stored to `frame.framedata.ssavlues`), or `nothing`.
 Typically, assignment to a variable binding does not result in an ssa store by JuliaInterpreter.
 """
-selective_eval!(interp::Interpreter, frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false) =
-    JuliaInterpreter.finish_and_return!(SelectiveInterpreter(interp, isrequired), frame, istoplevel)
+function selective_eval!(interp::Interpreter, frame::Frame, isrequired::AbstractVector{Bool}, istoplevel::Bool=false)
+    interp = SelectiveInterpreter(interp, isrequired, SelectiveEvalController())
+    JuliaInterpreter.finish_and_return!(interp, frame, istoplevel)
+end
 selective_eval!(args...) = selective_eval!(RecursiveInterpreter(), args...)
 
 """

src/packagedef.jl

@@ -24,7 +24,8 @@ const trackedheads = (:method,)    # Revise uses this (for now), don't delete; a
 const structdecls = (:_structtype, :_abstracttype, :_primitivetype)
 
 export signature, rename_framemethods!, methoddef!, methoddefs!, bodymethod
-export CodeEdges, lines_required, lines_required!, selective_eval!, selective_eval_fromstart!
+export CodeEdges, SelectiveEvalController, SelectiveInterpreter,
+       lines_required, lines_required!, selective_eval!, selective_eval_fromstart!
 
 include("utils.jl")
 include("signatures.jl")

test/codeedges.jl

@@ -223,6 +223,25 @@ module ModSelective end
     @test ModSelective.k11 == 0
     @test 3 <= ModSelective.s11 <= 15
 
+    # Final block is not a `return`: Need to use `controller::SelectiveEvalController` explicitly
+    ex = quote
+        x = 1
+        yy = 7
+        @label loop
+        x += 1
+        x < 5 || return yy
+        @goto loop
+    end
+    frame = Frame(ModSelective, ex)
+    src = frame.framecode.src
+    edges = CodeEdges(ModSelective, src)
+    controller = SelectiveEvalController()
+    isrequired = lines_required(GlobalRef(ModSelective, :x), src, edges, controller)
+    interp = LoweredCodeUtils.SelectiveInterpreter(LoweredCodeUtils.RecursiveInterpreter(), isrequired, controller)
+    JuliaInterpreter.finish_and_return!(interp, frame, true)
+    @test ModSelective.x == 5
+    @test !isdefined(ModSelective, :yy)
+
     # Control-flow in an abstract type definition
     ex = :(abstract type StructParent{T, N} <: AbstractArray{T, N} end)
     frame = Frame(ModSelective, ex)

test/signatures.jl

@@ -436,9 +436,10 @@ bodymethtest5(x, y=Dict(1=>2)) = 5
     oldenv = Pkg.project().path
     try
         # we test with the old version of CBinding, let's do it in an isolated environment
+        # so we don't cause package conflicts with everything else
         Pkg.activate(; temp=true, io=devnull)
 
-        @info "Adding CBinding to the environment for test purposes"
+        @info "Adding CBinding v0.9.4 to the environment for test purposes"
         Pkg.add(; name="CBinding", version="0.9.4", io=devnull) # `@cstruct` isn't defined for v1.0 and above
 
         m = Module()