Skip to content

Commit fa78aab

Browse files
JeffBezansonJeffBezanson
committed
fix code coverage bug in tail position and else
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.
1 parent e460d35 commit fa78aab

File tree

5 files changed

+120
-38
lines changed

5 files changed

+120
-38
lines changed

‎base/compiler/ssair/inlining.jl‎

Lines changed: 3 additions & 1 deletion
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‎

Lines changed: 5 additions & 1 deletion
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‎

Lines changed: 48 additions & 35 deletions
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‎

Lines changed: 63 additions & 0 deletions
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‎

Lines changed: 1 addition & 1 deletion
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)

0 commit comments

Comments
 (0)