Skip to content

Commit

Permalink
fix code coverage bug in tail position and else (JuliaLang#53354)
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.

Also fixes inliner handling of the sentinel `0` value for code
locations.
  • Loading branch information
@JeffBezanson @tecosaur
JeffBezanson authored and tecosaur committed Mar 4, 2024
1 parent 1263560 commit aa0716a
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 aa0716a

Please sign in to comment.