Skip to content

Commit

Permalink
fix code coverage bug in tail position and else
Browse files Browse the repository at this point in the history 
This was due to lowering keeping the same location info for the inserted
`return` or `goto` statement, even though the last seen location might not have
executed.
  • Loading branch information
@JeffBezanson
JeffBezanson committed Feb 19, 2024
1 parent e460d35 commit b34d26f
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 39 deletions.
4 changes: 3 additions & 1 deletion base/compiler/ssair/inlining.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1818,7 +1818,9 @@ end

function ssa_substitute!(insert_node!::Inserter, subst_inst::Instruction, @nospecialize(val),
ssa_substitute::SSASubstitute)
subst_inst[:line] += ssa_substitute.linetable_offset
if subst_inst[:line] != 0
subst_inst[:line] += ssa_substitute.linetable_offset
end
return ssa_substitute_op!(insert_node!, subst_inst, val, ssa_substitute)
end

Expand Down
6 changes: 5 additions & 1 deletion base/compiler/ssair/passes.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1519,8 +1519,12 @@ function try_inline_finalizer!(ir::IRCode, argexprs::Vector{Any}, idx::Int,
ssa_rename[ssa.id]
end
stmt′ = ssa_substitute_op!(InsertBefore(ir, SSAValue(idx)), inst, stmt′, ssa_substitute)
newline = inst[:line]
if newline != 0
newline += ssa_substitute.linetable_offset
end
ssa_rename[idx′] = insert_node!(ir, idx,
NewInstruction(inst; stmt=stmt′, line=inst[:line]+ssa_substitute.linetable_offset),
NewInstruction(inst; stmt=stmt′, line=newline),
attach_after)
end

Expand Down
83 changes: 48 additions & 35 deletions src/julia-syntax.scm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4371,7 +4371,7 @@ f(x) = yt(x)
(if (eq? (cdr s) dest-tokens)
(cons (car s) l)
(loop (cdr s) (cons (car s) l))))))
(define (emit-return x)
(define (emit-return tail x)
(define (emit- x)
(let* ((tmp (if ((if (null? catch-token-stack) valid-ir-return? simple-atom?) x)
#f
Expand All @@ -4380,8 +4380,12 @@ f(x) = yt(x)
(begin (emit `(= ,tmp ,x)) tmp)
x)))
(define (actually-return x)
(let* ((x (if rett
(compile (convert-for-type-decl (emit- x) rett #t lam) '() #t #f)
(let* ((x (begin0 (emit- x)
;; if we are adding an implicit return then mark it as having no location
(if (not (eq? tail 'explicit))
(emit '(line #f)))))
(x (if rett
(compile (convert-for-type-decl x rett #t lam) '() #t #f)
x))
(x (emit- x)))
(let ((pexc (pop-exc-expr catch-token-stack '())))
Expand Down Expand Up @@ -4531,7 +4535,7 @@ f(x) = yt(x)
(eq? (car e) 'globalref))
(underscore-symbol? (cadr e)))))
(error (string "all-underscore identifiers are write-only and their values cannot be used in expressions" (format-loc current-loc))))
(cond (tail (emit-return e1))
(cond (tail (emit-return tail e1))
(value e1)
((symbol? e1) (emit e1) #f) ;; keep symbols for undefined-var checking
((and (pair? e1) (eq? (car e1) 'outerref)) (emit e1) #f) ;; keep globals for undefined-var checking
Expand Down Expand Up @@ -4577,7 +4581,7 @@ f(x) = yt(x)
(else
(compile-args (cdr e) break-labels))))
(callex (cons (car e) args)))
(cond (tail (emit-return callex))
(cond (tail (emit-return tail callex))
(value callex)
(else (emit callex)))))
((=)
Expand All @@ -4594,7 +4598,7 @@ f(x) = yt(x)
(if (not (eq? rr rhs))
(emit `(= ,rr ,rhs)))
(emit `(= ,lhs ,rr))
(if tail (emit-return rr))
(if tail (emit-return tail rr))
rr)
(emit-assignment lhs rhs))))))
((block)
Expand Down Expand Up @@ -4647,7 +4651,7 @@ f(x) = yt(x)
(if file-diff (set! filename last-fname))
v)))
((return)
(compile (cadr e) break-labels #t #t)
(compile (cadr e) break-labels #t 'explicit)
#f)
((unnecessary)
;; `unnecessary` marks expressions generated by lowering that
Expand All @@ -4662,7 +4666,8 @@ f(x) = yt(x)
(let ((v1 (compile (caddr e) break-labels value tail)))
(if val (emit-assignment val v1))
(if (and (not tail) (or (length> e 3) val))
(emit end-jump))
(begin (emit `(line #f))
(emit end-jump)))
(let ((elselabel (make&mark-label)))
(for-each (lambda (test)
(set-car! (cddr test) elselabel))
Expand All @@ -4674,7 +4679,7 @@ f(x) = yt(x)
(if (not tail)
(set-car! (cdr end-jump) (make&mark-label))
(if (length= e 3)
(emit-return v2)))
(emit-return tail v2)))
val))))
((_while)
(let* ((endl (make-label))
Expand Down Expand Up @@ -4716,7 +4721,7 @@ f(x) = yt(x)
(emit `(label ,m))
(put! label-map (cadr e) (make&mark-label)))
(if tail
(emit-return '(null))
(emit-return tail '(null))
(if value (error "misplaced label")))))
((symbolicgoto)
(let* ((m (get label-map (cadr e) #f))
Expand Down Expand Up @@ -4762,7 +4767,7 @@ f(x) = yt(x)
(begin (if els
(begin (if (and (not val) v1) (emit v1))
(emit `(leave ,handler-token)))
(if v1 (emit-return v1)))
(if v1 (emit-return tail v1)))
(if (not finally) (set! endl #f)))
(begin (emit `(leave ,handler-token))
(emit `(goto ,(or els endl)))))
Expand Down Expand Up @@ -4794,7 +4799,7 @@ f(x) = yt(x)
(emit `(= ,tmp (call (core ===) ,finally ,(caar actions))))
(emit `(gotoifnot ,tmp ,skip))))
(let ((ac (cdar actions)))
(cond ((eq? (car ac) 'return) (emit-return (cadr ac)))
(cond ((eq? (car ac) 'return) (emit-return tail (cadr ac)))
((eq? (car ac) 'break) (emit-break (cadr ac)))
(else ;; assumed to be a rethrow
(emit ac))))
Expand Down Expand Up @@ -4833,8 +4838,8 @@ f(x) = yt(x)
(set! global-const-error current-loc))
(emit e))))
((atomic) (error "misplaced atomic declaration"))
((isdefined) (if tail (emit-return e) e))
((boundscheck) (if tail (emit-return e) e))
((isdefined) (if tail (emit-return tail e) e))
((boundscheck) (if tail (emit-return tail e) e))

((method)
(if (not (null? (cadr lam)))
Expand All @@ -4855,20 +4860,20 @@ f(x) = yt(x)
l))))
(emit `(method ,(or (cadr e) '(false)) ,sig ,lam))
(if value (compile '(null) break-labels value tail)))
(cond (tail (emit-return e))
(cond (tail (emit-return tail e))
(value e)
(else (emit e)))))
((lambda)
(let ((temp (linearize e)))
(cond (tail (emit-return temp))
(cond (tail (emit-return tail temp))
(value temp)
(else (emit temp)))))

;; top level expressions
((thunk module)
(check-top-level e)
(emit e)
(if tail (emit-return '(null)))
(if tail (emit-return tail '(null)))
'(null))
((toplevel-only)
(check-top-level (cdr e))
Expand All @@ -4878,7 +4883,7 @@ f(x) = yt(x)
(check-top-level e)
(let ((val (make-ssavalue)))
(emit `(= ,val ,e))
(if tail (emit-return val))
(if tail (emit-return tail val))
val))

;; other top level expressions
Expand All @@ -4887,7 +4892,7 @@ f(x) = yt(x)
(emit e)
(let ((have-ret? (and (pair? code) (pair? (car code)) (eq? (caar code) 'return))))
(if (and tail (not have-ret?))
(emit-return '(null))))
(emit-return tail '(null))))
'(null))

((gc_preserve_begin)
Expand All @@ -4911,7 +4916,7 @@ f(x) = yt(x)
(else
(emit e)))
(if (and tail (not have-ret?))
(emit-return '(null)))
(emit-return tail '(null)))
'(null)))

;; unsupported assignment operators
Expand Down Expand Up @@ -5027,6 +5032,7 @@ f(x) = yt(x)
(labltable (table))
(ssavtable (table))
(current-loc 0)
(nowhere #f)
(current-file file)
(current-line line)
(locstack '())
Expand All @@ -5040,26 +5046,33 @@ f(x) = yt(x)
(set! current-loc 1)))
(set! code (cons e code))
(set! i (+ i 1))
(set! locs (cons current-loc locs)))
(set! locs (cons (if nowhere 0 current-loc) locs))
(set! nowhere #f))
(let loop ((stmts (cdr body)))
(if (pair? stmts)
(let ((e (car stmts)))
(cond ((atom? e) (emit e))
((eq? (car e) 'line)
(if (and (= current-line 0) (length= e 2) (pair? linetable))
;; (line n) after push_loc just updates the line for the new file
(begin (set-lineno! (car linetable) (cadr e))
(set! current-line (cadr e)))
(begin
(set! current-line (cadr e))
(if (pair? (cddr e))
(set! current-file (caddr e)))
(set! linetable (cons (if (null? locstack)
(make-lineinfo name current-file current-line)
(make-lineinfo name current-file current-line (caar locstack)))
linetable))
(set! linetablelen (+ linetablelen 1))
(set! current-loc linetablelen))))
(cond ((and (length= e 2) (not (cadr e)))
;; (line #f) marks that we are entering a generated statement
;; that should not be counted as belonging to the previous marked location,
;; for example `return` after a not-executed `if` arm in tail position.
(set! nowhere #t))
((and (= current-line 0) (length= e 2) (pair? linetable))
;; (line n) after push_loc just updates the line for the new file
(begin (set-lineno! (car linetable) (cadr e))
(set! current-line (cadr e))))
(else
(begin
(set! current-line (cadr e))
(if (pair? (cddr e))
(set! current-file (caddr e)))
(set! linetable (cons (if (null? locstack)
(make-lineinfo name current-file current-line)
(make-lineinfo name current-file current-line (caar locstack)))
linetable))
(set! linetablelen (+ linetablelen 1))
(set! current-loc linetablelen)))))
((and (length> e 2) (eq? (car e) 'meta) (eq? (cadr e) 'push_loc))
(set! locstack (cons (list current-loc current-line current-file) locstack))
(set! current-file (caddr e))
Expand Down
63 changes: 63 additions & 0 deletions test/cmdlineargs.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,69 @@ let exename = `$(Base.julia_cmd()) --startup-file=no --color=no`
got = read(covfile, String)
@test isempty(got)
rm(covfile)

function coverage_info_for(src::String)
mktemp(dir) do srcfile, io
write(io, src); close(io)
outfile = tempname(dir, cleanup=false)*".info"
run(`$exename --code-coverage=$outfile $srcfile`)
result = read(outfile, String)
rm(outfile, force=true)
result
end
end
@test contains(coverage_info_for("""
function cov_bug(x, p)
if p > 2
print("") # runs
end
if Base.compilerbarrier(:const, false)
println("Does not run")
end
end
function do_test()
cov_bug(5, 3)
end
do_test()
"""), """
DA:1,1
DA:2,1
DA:3,1
DA:5,1
DA:6,0
DA:9,1
DA:10,1
LH:6
LF:7
""")
@test contains(coverage_info_for("""
function cov_bug()
if Base.compilerbarrier(:const, true)
if Base.compilerbarrier(:const, true)
if Base.compilerbarrier(:const, false)
println("Does not run")
end
else
print("Does not run either")
end
else
print("")
end
return nothing
end
cov_bug()
"""), """
DA:1,1
DA:2,1
DA:3,1
DA:4,1
DA:5,0
DA:8,0
DA:11,0
DA:13,1
LH:5
LF:8
""")
end

# --track-allocation
Expand Down
2 changes: 1 addition & 1 deletion test/show.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2106,7 +2106,7 @@ let src = code_typed(my_fun28173, (Int,), debuginfo=:source)[1][1]
io = IOBuffer()
Base.IRShow.show_ir(io, ir, Base.IRShow.default_config(ir; verbose_linetable=true))
seekstart(io)
@test count(contains(r"@ a{80}:\d+ within `my_fun28173"), eachline(io)) == 11
@test count(contains(r"@ a{80}:\d+ within `my_fun28173"), eachline(io)) == 10

# Test that a bad :invoke doesn't cause an error during printing
Core.Compiler.insert_node!(ir, 1, Core.Compiler.NewInstruction(Expr(:invoke, nothing, sin), Any), false)
Expand Down
2 changes: 1 addition & 1 deletion test/syntax.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ m1_exprs = get_expr_list(Meta.lower(@__MODULE__, quote @m1 end))
let low3 = Meta.lower(@__MODULE__, quote @m3 end)
m3_exprs = get_expr_list(low3)
ci = low3.args[1]::Core.CodeInfo
@test ci.codelocs in ([4, 4, 2], [4, 2])
@test ci.codelocs in ([4, 4, 0], [4, 0])
@test is_return_ssavalue(m3_exprs[end])
end

Expand Down

0 comments on commit b34d26f

Please sign in to comment.