Skip to content

Commit 61fc907

Browse files
JeffBezansonJeffBezanson
authored
fix code coverage bug in tail position and else (#53354)
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.
1 parent d12a620 commit 61fc907

File tree

6 files changed

+121
-39
lines changed

6 files changed

+121
-39
lines changed

‎base/compiler/ssair/inlining.jl

+3-1
Original file line numberDiff line numberDiff line change
@@ -1818,7 +1818,9 @@ end
18181818

18191819
function ssa_substitute!(insert_node!::Inserter, subst_inst::Instruction, @nospecialize(val),
18201820
ssa_substitute::SSASubstitute)
1821-
subst_inst[:line] += ssa_substitute.linetable_offset
1821+
if subst_inst[:line] != 0
1822+
subst_inst[:line] += ssa_substitute.linetable_offset
1823+
end
18221824
return ssa_substitute_op!(insert_node!, subst_inst, val, ssa_substitute)
18231825
end
18241826

‎base/compiler/ssair/passes.jl

+5-1
Original file line numberDiff line numberDiff line change
@@ -1519,8 +1519,12 @@ function try_inline_finalizer!(ir::IRCode, argexprs::Vector{Any}, idx::Int,
15191519
ssa_rename[ssa.id]
15201520
end
15211521
stmt′ = ssa_substitute_op!(InsertBefore(ir, SSAValue(idx)), inst, stmt′, ssa_substitute)
1522+
newline = inst[:line]
1523+
if newline != 0
1524+
newline += ssa_substitute.linetable_offset
1525+
end
15221526
ssa_rename[idx′] = insert_node!(ir, idx,
1523-
NewInstruction(inst; stmt=stmt′, line=inst[:line]+ssa_substitute.linetable_offset),
1527+
NewInstruction(inst; stmt=stmt′, line=newline),
15241528
attach_after)
15251529
end
15261530

‎src/julia-syntax.scm

+48-35
Original file line numberDiff line numberDiff line change
@@ -4371,7 +4371,7 @@ f(x) = yt(x)
43714371
(if (eq? (cdr s) dest-tokens)
43724372
(cons (car s) l)
43734373
(loop (cdr s) (cons (car s) l))))))
4374-
(define (emit-return x)
4374+
(define (emit-return tail x)
43754375
(define (emit- x)
43764376
(let* ((tmp (if ((if (null? catch-token-stack) valid-ir-return? simple-atom?) x)
43774377
#f
@@ -4380,8 +4380,12 @@ f(x) = yt(x)
43804380
(begin (emit `(= ,tmp ,x)) tmp)
43814381
x)))
43824382
(define (actually-return x)
4383-
(let* ((x (if rett
4384-
(compile (convert-for-type-decl (emit- x) rett #t lam) '() #t #f)
4383+
(let* ((x (begin0 (emit- x)
4384+
;; if we are adding an implicit return then mark it as having no location
4385+
(if (not (eq? tail 'explicit))
4386+
(emit '(line #f)))))
4387+
(x (if rett
4388+
(compile (convert-for-type-decl x rett #t lam) '() #t #f)
43854389
x))
43864390
(x (emit- x)))
43874391
(let ((pexc (pop-exc-expr catch-token-stack '())))
@@ -4531,7 +4535,7 @@ f(x) = yt(x)
45314535
(eq? (car e) 'globalref))
45324536
(underscore-symbol? (cadr e)))))
45334537
(error (string "all-underscore identifiers are write-only and their values cannot be used in expressions" (format-loc current-loc))))
4534-
(cond (tail (emit-return e1))
4538+
(cond (tail (emit-return tail e1))
45354539
(value e1)
45364540
((symbol? e1) (emit e1) #f) ;; keep symbols for undefined-var checking
45374541
((and (pair? e1) (eq? (car e1) 'outerref)) (emit e1) #f) ;; keep globals for undefined-var checking
@@ -4577,7 +4581,7 @@ f(x) = yt(x)
45774581
(else
45784582
(compile-args (cdr e) break-labels))))
45794583
(callex (cons (car e) args)))
4580-
(cond (tail (emit-return callex))
4584+
(cond (tail (emit-return tail callex))
45814585
(value callex)
45824586
(else (emit callex)))))
45834587
((=)
@@ -4594,7 +4598,7 @@ f(x) = yt(x)
45944598
(if (not (eq? rr rhs))
45954599
(emit `(= ,rr ,rhs)))
45964600
(emit `(= ,lhs ,rr))
4597-
(if tail (emit-return rr))
4601+
(if tail (emit-return tail rr))
45984602
rr)
45994603
(emit-assignment lhs rhs))))))
46004604
((block)
@@ -4647,7 +4651,7 @@ f(x) = yt(x)
46474651
(if file-diff (set! filename last-fname))
46484652
v)))
46494653
((return)
4650-
(compile (cadr e) break-labels #t #t)
4654+
(compile (cadr e) break-labels #t 'explicit)
46514655
#f)
46524656
((unnecessary)
46534657
;; `unnecessary` marks expressions generated by lowering that
@@ -4662,7 +4666,8 @@ f(x) = yt(x)
46624666
(let ((v1 (compile (caddr e) break-labels value tail)))
46634667
(if val (emit-assignment val v1))
46644668
(if (and (not tail) (or (length> e 3) val))
4665-
(emit end-jump))
4669+
(begin (emit `(line #f))
4670+
(emit end-jump)))
46664671
(let ((elselabel (make&mark-label)))
46674672
(for-each (lambda (test)
46684673
(set-car! (cddr test) elselabel))
@@ -4674,7 +4679,7 @@ f(x) = yt(x)
46744679
(if (not tail)
46754680
(set-car! (cdr end-jump) (make&mark-label))
46764681
(if (length= e 3)
4677-
(emit-return v2)))
4682+
(emit-return tail v2)))
46784683
val))))
46794684
((_while)
46804685
(let* ((endl (make-label))
@@ -4716,7 +4721,7 @@ f(x) = yt(x)
47164721
(emit `(label ,m))
47174722
(put! label-map (cadr e) (make&mark-label)))
47184723
(if tail
4719-
(emit-return '(null))
4724+
(emit-return tail '(null))
47204725
(if value (error "misplaced label")))))
47214726
((symbolicgoto)
47224727
(let* ((m (get label-map (cadr e) #f))
@@ -4762,7 +4767,7 @@ f(x) = yt(x)
47624767
(begin (if els
47634768
(begin (if (and (not val) v1) (emit v1))
47644769
(emit `(leave ,handler-token)))
4765-
(if v1 (emit-return v1)))
4770+
(if v1 (emit-return tail v1)))
47664771
(if (not finally) (set! endl #f)))
47674772
(begin (emit `(leave ,handler-token))
47684773
(emit `(goto ,(or els endl)))))
@@ -4794,7 +4799,7 @@ f(x) = yt(x)
47944799
(emit `(= ,tmp (call (core ===) ,finally ,(caar actions))))
47954800
(emit `(gotoifnot ,tmp ,skip))))
47964801
(let ((ac (cdar actions)))
4797-
(cond ((eq? (car ac) 'return) (emit-return (cadr ac)))
4802+
(cond ((eq? (car ac) 'return) (emit-return tail (cadr ac)))
47984803
((eq? (car ac) 'break) (emit-break (cadr ac)))
47994804
(else ;; assumed to be a rethrow
48004805
(emit ac))))
@@ -4833,8 +4838,8 @@ f(x) = yt(x)
48334838
(set! global-const-error current-loc))
48344839
(emit e))))
48354840
((atomic) (error "misplaced atomic declaration"))
4836-
((isdefined) (if tail (emit-return e) e))
4837-
((boundscheck) (if tail (emit-return e) e))
4841+
((isdefined) (if tail (emit-return tail e) e))
4842+
((boundscheck) (if tail (emit-return tail e) e))
48384843

48394844
((method)
48404845
(if (not (null? (cadr lam)))
@@ -4855,20 +4860,20 @@ f(x) = yt(x)
48554860
l))))
48564861
(emit `(method ,(or (cadr e) '(false)) ,sig ,lam))
48574862
(if value (compile '(null) break-labels value tail)))
4858-
(cond (tail (emit-return e))
4863+
(cond (tail (emit-return tail e))
48594864
(value e)
48604865
(else (emit e)))))
48614866
((lambda)
48624867
(let ((temp (linearize e)))
4863-
(cond (tail (emit-return temp))
4868+
(cond (tail (emit-return tail temp))
48644869
(value temp)
48654870
(else (emit temp)))))
48664871

48674872
;; top level expressions
48684873
((thunk module)
48694874
(check-top-level e)
48704875
(emit e)
4871-
(if tail (emit-return '(null)))
4876+
(if tail (emit-return tail '(null)))
48724877
'(null))
48734878
((toplevel-only)
48744879
(check-top-level (cdr e))
@@ -4878,7 +4883,7 @@ f(x) = yt(x)
48784883
(check-top-level e)
48794884
(let ((val (make-ssavalue)))
48804885
(emit `(= ,val ,e))
4881-
(if tail (emit-return val))
4886+
(if tail (emit-return tail val))
48824887
val))
48834888

48844889
;; other top level expressions
@@ -4887,7 +4892,7 @@ f(x) = yt(x)
48874892
(emit e)
48884893
(let ((have-ret? (and (pair? code) (pair? (car code)) (eq? (caar code) 'return))))
48894894
(if (and tail (not have-ret?))
4890-
(emit-return '(null))))
4895+
(emit-return tail '(null))))
48914896
'(null))
48924897

48934898
((gc_preserve_begin)
@@ -4911,7 +4916,7 @@ f(x) = yt(x)
49114916
(else
49124917
(emit e)))
49134918
(if (and tail (not have-ret?))
4914-
(emit-return '(null)))
4919+
(emit-return tail '(null)))
49154920
'(null)))
49164921

49174922
;; unsupported assignment operators
@@ -5027,6 +5032,7 @@ f(x) = yt(x)
50275032
(labltable (table))
50285033
(ssavtable (table))
50295034
(current-loc 0)
5035+
(nowhere #f)
50305036
(current-file file)
50315037
(current-line line)
50325038
(locstack '())
@@ -5040,26 +5046,33 @@ f(x) = yt(x)
50405046
(set! current-loc 1)))
50415047
(set! code (cons e code))
50425048
(set! i (+ i 1))
5043-
(set! locs (cons current-loc locs)))
5049+
(set! locs (cons (if nowhere 0 current-loc) locs))
5050+
(set! nowhere #f))
50445051
(let loop ((stmts (cdr body)))
50455052
(if (pair? stmts)
50465053
(let ((e (car stmts)))
50475054
(cond ((atom? e) (emit e))
50485055
((eq? (car e) 'line)
5049-
(if (and (= current-line 0) (length= e 2) (pair? linetable))
5050-
;; (line n) after push_loc just updates the line for the new file
5051-
(begin (set-lineno! (car linetable) (cadr e))
5052-
(set! current-line (cadr e)))
5053-
(begin
5054-
(set! current-line (cadr e))
5055-
(if (pair? (cddr e))
5056-
(set! current-file (caddr e)))
5057-
(set! linetable (cons (if (null? locstack)
5058-
(make-lineinfo name current-file current-line)
5059-
(make-lineinfo name current-file current-line (caar locstack)))
5060-
linetable))
5061-
(set! linetablelen (+ linetablelen 1))
5062-
(set! current-loc linetablelen))))
5056+
(cond ((and (length= e 2) (not (cadr e)))
5057+
;; (line #f) marks that we are entering a generated statement
5058+
;; that should not be counted as belonging to the previous marked location,
5059+
;; for example `return` after a not-executed `if` arm in tail position.
5060+
(set! nowhere #t))
5061+
((and (= current-line 0) (length= e 2) (pair? linetable))
5062+
;; (line n) after push_loc just updates the line for the new file
5063+
(begin (set-lineno! (car linetable) (cadr e))
5064+
(set! current-line (cadr e))))
5065+
(else
5066+
(begin
5067+
(set! current-line (cadr e))
5068+
(if (pair? (cddr e))
5069+
(set! current-file (caddr e)))
5070+
(set! linetable (cons (if (null? locstack)
5071+
(make-lineinfo name current-file current-line)
5072+
(make-lineinfo name current-file current-line (caar locstack)))
5073+
linetable))
5074+
(set! linetablelen (+ linetablelen 1))
5075+
(set! current-loc linetablelen)))))
50635076
((and (length> e 2) (eq? (car e) 'meta) (eq? (cadr e) 'push_loc))
50645077
(set! locstack (cons (list current-loc current-line current-file) locstack))
50655078
(set! current-file (caddr e))

‎test/cmdlineargs.jl

+63
Original file line numberDiff line numberDiff line change
@@ -545,6 +545,69 @@ let exename = `$(Base.julia_cmd()) --startup-file=no --color=no`
545545
got = read(covfile, String)
546546
@test isempty(got)
547547
rm(covfile)
548+
549+
function coverage_info_for(src::String)
550+
mktemp(dir) do srcfile, io
551+
write(io, src); close(io)
552+
outfile = tempname(dir, cleanup=false)*".info"
553+
run(`$exename --code-coverage=$outfile $srcfile`)
554+
result = read(outfile, String)
555+
rm(outfile, force=true)
556+
result
557+
end
558+
end
559+
@test contains(coverage_info_for("""
560+
function cov_bug(x, p)
561+
if p > 2
562+
print("") # runs
563+
end
564+
if Base.compilerbarrier(:const, false)
565+
println("Does not run")
566+
end
567+
end
568+
function do_test()
569+
cov_bug(5, 3)
570+
end
571+
do_test()
572+
"""), """
573+
DA:1,1
574+
DA:2,1
575+
DA:3,1
576+
DA:5,1
577+
DA:6,0
578+
DA:9,1
579+
DA:10,1
580+
LH:6
581+
LF:7
582+
""")
583+
@test contains(coverage_info_for("""
584+
function cov_bug()
585+
if Base.compilerbarrier(:const, true)
586+
if Base.compilerbarrier(:const, true)
587+
if Base.compilerbarrier(:const, false)
588+
println("Does not run")
589+
end
590+
else
591+
print("Does not run either")
592+
end
593+
else
594+
print("")
595+
end
596+
return nothing
597+
end
598+
cov_bug()
599+
"""), """
600+
DA:1,1
601+
DA:2,1
602+
DA:3,1
603+
DA:4,1
604+
DA:5,0
605+
DA:8,0
606+
DA:11,0
607+
DA:13,1
608+
LH:5
609+
LF:8
610+
""")
548611
end
549612

550613
# --track-allocation

‎test/show.jl

+1-1
Original file line numberDiff line numberDiff line change
@@ -2106,7 +2106,7 @@ let src = code_typed(my_fun28173, (Int,), debuginfo=:source)[1][1]
21062106
io = IOBuffer()
21072107
Base.IRShow.show_ir(io, ir, Base.IRShow.default_config(ir; verbose_linetable=true))
21082108
seekstart(io)
2109-
@test count(contains(r"@ a{80}:\d+ within `my_fun28173"), eachline(io)) == 11
2109+
@test count(contains(r"@ a{80}:\d+ within `my_fun28173"), eachline(io)) == 10
21102110

21112111
# Test that a bad :invoke doesn't cause an error during printing
21122112
Core.Compiler.insert_node!(ir, 1, Core.Compiler.NewInstruction(Expr(:invoke, nothing, sin), Any), false)

‎test/syntax.jl

+1-1
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,7 @@ m1_exprs = get_expr_list(Meta.lower(@__MODULE__, quote @m1 end))
713713
let low3 = Meta.lower(@__MODULE__, quote @m3 end)
714714
m3_exprs = get_expr_list(low3)
715715
ci = low3.args[1]::Core.CodeInfo
716-
@test ci.codelocs in ([4, 4, 2], [4, 2])
716+
@test ci.codelocs in ([4, 4, 0], [4, 0])
717717
@test is_return_ssavalue(m3_exprs[end])
718718
end
719719

0 commit comments

Comments
 (0)