Skip to content

Commit

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

(cherry picked from commit 61fc907)
  • Loading branch information
@JeffBezanson
JeffBezanson committed Feb 21, 2024
1 parent 988f154 commit dcaabe8
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 37 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 @@ -1811,7 +1811,9 @@ function ssa_substitute!(insert_node!::Inserter,
spvals_ssa::Union{Nothing, SSAValue},
linetable_offset::Int32, boundscheck::Symbol)
subst_inst[:flag] &= ~IR_FLAG_INBOUNDS
subst_inst[:line] += linetable_offset
if subst_inst[:line] != 0
subst_inst[:line] += linetable_offset
end
return ssa_substitute_op!(insert_node!, subst_inst,
val, arg_replacements, spsig, spvals, spvals_ssa, boundscheck)
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 @@ -1205,8 +1205,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′, argexprs, mi.specTypes, mi.sparam_vals, sp_ssa, :default)
newline = inst[:line]
if newline != 0
newline += linetable_offset
end
ssa_rename[idx′] = insert_node!(ir, idx,
NewInstruction(inst; stmt=stmt′, line=inst[:line]+linetable_offset),
NewInstruction(inst; stmt=stmt′, line=newline),
attach_after)
end

Expand Down
82 changes: 48 additions & 34 deletions src/julia-syntax.scm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4338,7 +4338,7 @@ f(x) = yt(x)
(car s)
(loop (cdr s))))))
`(pop_exception ,restore-token))))
(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 @@ -4347,8 +4347,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 @@ -4487,7 +4491,7 @@ f(x) = yt(x)
(eq? (car e) 'globalref))
(underscore-symbol? (cadr e)))))
(error (string "all-underscore identifier used as rvalue" (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 @@ -4533,7 +4537,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 @@ -4550,7 +4554,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 @@ -4603,7 +4607,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 @@ -4618,7 +4622,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 @@ -4630,7 +4635,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 @@ -4672,7 +4677,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 @@ -4712,7 +4717,7 @@ f(x) = yt(x)
(begin (if els
(begin (if (and (not val) v1) (emit v1))
(emit '(leave 1)))
(if v1 (emit-return v1)))
(if v1 (emit-return tail v1)))
(if (not finally) (set! endl #f)))
(begin (emit '(leave 1))
(emit `(goto ,(or els endl)))))
Expand Down Expand Up @@ -4744,7 +4749,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 @@ -4783,8 +4788,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 @@ -4805,20 +4810,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 @@ -4828,7 +4833,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 @@ -4837,7 +4842,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 @@ -4861,7 +4866,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 @@ -4979,6 +4984,7 @@ f(x) = yt(x)
(labltable (table))
(ssavtable (table))
(current-loc 0)
(nowhere #f)
(current-file file)
(current-line line)
(locstack '())
Expand All @@ -4991,25 +4997,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! current-loc (- (length linetable) 1)))))
(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 (- (length linetable) 1))))))
((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 @@ -520,6 +520,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/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 == [4, 2]
@test ci.codelocs == [4, 0]
@test is_return_ssavalue(m3_exprs[end])
end

Expand Down

0 comments on commit dcaabe8

Please sign in to comment.