Skip to content

Commit dcaabe8

Browse files
JeffBezansonJeffBezanson
committed
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. (cherry picked from commit 61fc907)
1 parent 988f154 commit dcaabe8

File tree

5 files changed

+120
-37
lines changed

5 files changed

+120
-37
lines changed

‎base/compiler/ssair/inlining.jl

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1811,7 +1811,9 @@ function ssa_substitute!(insert_node!::Inserter,
18111811
spvals_ssa::Union{Nothing, SSAValue},
18121812
linetable_offset::Int32, boundscheck::Symbol)
18131813
subst_inst[:flag] &= ~IR_FLAG_INBOUNDS
1814-
subst_inst[:line] += linetable_offset
1814+
if subst_inst[:line] != 0
1815+
subst_inst[:line] += linetable_offset
1816+
end
18151817
return ssa_substitute_op!(insert_node!, subst_inst,
18161818
val, arg_replacements, spsig, spvals, spvals_ssa, boundscheck)
18171819
end

‎base/compiler/ssair/passes.jl

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1205,8 +1205,12 @@ function try_inline_finalizer!(ir::IRCode, argexprs::Vector{Any}, idx::Int,
12051205
ssa_rename[ssa.id]
12061206
end
12071207
stmt′ = ssa_substitute_op!(InsertBefore(ir, SSAValue(idx)), inst, stmt′, argexprs, mi.specTypes, mi.sparam_vals, sp_ssa, :default)
1208+
newline = inst[:line]
1209+
if newline != 0
1210+
newline += linetable_offset
1211+
end
12081212
ssa_rename[idx′] = insert_node!(ir, idx,
1209-
NewInstruction(inst; stmt=stmt′, line=inst[:line]+linetable_offset),
1213+
NewInstruction(inst; stmt=stmt′, line=newline),
12101214
attach_after)
12111215
end
12121216

‎src/julia-syntax.scm

Lines changed: 48 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -4338,7 +4338,7 @@ f(x) = yt(x)
43384338
(car s)
43394339
(loop (cdr s))))))
43404340
`(pop_exception ,restore-token))))
4341-
(define (emit-return x)
4341+
(define (emit-return tail x)
43424342
(define (emit- x)
43434343
(let* ((tmp (if ((if (null? catch-token-stack) valid-ir-return? simple-atom?) x)
43444344
#f
@@ -4347,8 +4347,12 @@ f(x) = yt(x)
43474347
(begin (emit `(= ,tmp ,x)) tmp)
43484348
x)))
43494349
(define (actually-return x)
4350-
(let* ((x (if rett
4351-
(compile (convert-for-type-decl (emit- x) rett #t lam) '() #t #f)
4350+
(let* ((x (begin0 (emit- x)
4351+
;; if we are adding an implicit return then mark it as having no location
4352+
(if (not (eq? tail 'explicit))
4353+
(emit '(line #f)))))
4354+
(x (if rett
4355+
(compile (convert-for-type-decl x rett #t lam) '() #t #f)
43524356
x))
43534357
(x (emit- x)))
43544358
(let ((pexc (pop-exc-expr catch-token-stack '())))
@@ -4487,7 +4491,7 @@ f(x) = yt(x)
44874491
(eq? (car e) 'globalref))
44884492
(underscore-symbol? (cadr e)))))
44894493
(error (string "all-underscore identifier used as rvalue" (format-loc current-loc))))
4490-
(cond (tail (emit-return e1))
4494+
(cond (tail (emit-return tail e1))
44914495
(value e1)
44924496
((symbol? e1) (emit e1) #f) ;; keep symbols for undefined-var checking
44934497
((and (pair? e1) (eq? (car e1) 'outerref)) (emit e1) #f) ;; keep globals for undefined-var checking
@@ -4533,7 +4537,7 @@ f(x) = yt(x)
45334537
(else
45344538
(compile-args (cdr e) break-labels))))
45354539
(callex (cons (car e) args)))
4536-
(cond (tail (emit-return callex))
4540+
(cond (tail (emit-return tail callex))
45374541
(value callex)
45384542
(else (emit callex)))))
45394543
((=)
@@ -4550,7 +4554,7 @@ f(x) = yt(x)
45504554
(if (not (eq? rr rhs))
45514555
(emit `(= ,rr ,rhs)))
45524556
(emit `(= ,lhs ,rr))
4553-
(if tail (emit-return rr))
4557+
(if tail (emit-return tail rr))
45544558
rr)
45554559
(emit-assignment lhs rhs))))))
45564560
((block)
@@ -4603,7 +4607,7 @@ f(x) = yt(x)
46034607
(if file-diff (set! filename last-fname))
46044608
v)))
46054609
((return)
4606-
(compile (cadr e) break-labels #t #t)
4610+
(compile (cadr e) break-labels #t 'explicit)
46074611
#f)
46084612
((unnecessary)
46094613
;; `unnecessary` marks expressions generated by lowering that
@@ -4618,7 +4622,8 @@ f(x) = yt(x)
46184622
(let ((v1 (compile (caddr e) break-labels value tail)))
46194623
(if val (emit-assignment val v1))
46204624
(if (and (not tail) (or (length> e 3) val))
4621-
(emit end-jump))
4625+
(begin (emit `(line #f))
4626+
(emit end-jump)))
46224627
(let ((elselabel (make&mark-label)))
46234628
(for-each (lambda (test)
46244629
(set-car! (cddr test) elselabel))
@@ -4630,7 +4635,7 @@ f(x) = yt(x)
46304635
(if (not tail)
46314636
(set-car! (cdr end-jump) (make&mark-label))
46324637
(if (length= e 3)
4633-
(emit-return v2)))
4638+
(emit-return tail v2)))
46344639
val))))
46354640
((_while)
46364641
(let* ((endl (make-label))
@@ -4672,7 +4677,7 @@ f(x) = yt(x)
46724677
(emit `(label ,m))
46734678
(put! label-map (cadr e) (make&mark-label)))
46744679
(if tail
4675-
(emit-return '(null))
4680+
(emit-return tail '(null))
46764681
(if value (error "misplaced label")))))
46774682
((symbolicgoto)
46784683
(let* ((m (get label-map (cadr e) #f))
@@ -4712,7 +4717,7 @@ f(x) = yt(x)
47124717
(begin (if els
47134718
(begin (if (and (not val) v1) (emit v1))
47144719
(emit '(leave 1)))
4715-
(if v1 (emit-return v1)))
4720+
(if v1 (emit-return tail v1)))
47164721
(if (not finally) (set! endl #f)))
47174722
(begin (emit '(leave 1))
47184723
(emit `(goto ,(or els endl)))))
@@ -4744,7 +4749,7 @@ f(x) = yt(x)
47444749
(emit `(= ,tmp (call (core ===) ,finally ,(caar actions))))
47454750
(emit `(gotoifnot ,tmp ,skip))))
47464751
(let ((ac (cdar actions)))
4747-
(cond ((eq? (car ac) 'return) (emit-return (cadr ac)))
4752+
(cond ((eq? (car ac) 'return) (emit-return tail (cadr ac)))
47484753
((eq? (car ac) 'break) (emit-break (cadr ac)))
47494754
(else ;; assumed to be a rethrow
47504755
(emit ac))))
@@ -4783,8 +4788,8 @@ f(x) = yt(x)
47834788
(set! global-const-error current-loc))
47844789
(emit e))))
47854790
((atomic) (error "misplaced atomic declaration"))
4786-
((isdefined) (if tail (emit-return e) e))
4787-
((boundscheck) (if tail (emit-return e) e))
4791+
((isdefined) (if tail (emit-return tail e) e))
4792+
((boundscheck) (if tail (emit-return tail e) e))
47884793

47894794
((method)
47904795
(if (not (null? (cadr lam)))
@@ -4805,20 +4810,20 @@ f(x) = yt(x)
48054810
l))))
48064811
(emit `(method ,(or (cadr e) '(false)) ,sig ,lam))
48074812
(if value (compile '(null) break-labels value tail)))
4808-
(cond (tail (emit-return e))
4813+
(cond (tail (emit-return tail e))
48094814
(value e)
48104815
(else (emit e)))))
48114816
((lambda)
48124817
(let ((temp (linearize e)))
4813-
(cond (tail (emit-return temp))
4818+
(cond (tail (emit-return tail temp))
48144819
(value temp)
48154820
(else (emit temp)))))
48164821

48174822
;; top level expressions
48184823
((thunk module)
48194824
(check-top-level e)
48204825
(emit e)
4821-
(if tail (emit-return '(null)))
4826+
(if tail (emit-return tail '(null)))
48224827
'(null))
48234828
((toplevel-only)
48244829
(check-top-level (cdr e))
@@ -4828,7 +4833,7 @@ f(x) = yt(x)
48284833
(check-top-level e)
48294834
(let ((val (make-ssavalue)))
48304835
(emit `(= ,val ,e))
4831-
(if tail (emit-return val))
4836+
(if tail (emit-return tail val))
48324837
val))
48334838

48344839
;; other top level expressions
@@ -4837,7 +4842,7 @@ f(x) = yt(x)
48374842
(emit e)
48384843
(let ((have-ret? (and (pair? code) (pair? (car code)) (eq? (caar code) 'return))))
48394844
(if (and tail (not have-ret?))
4840-
(emit-return '(null))))
4845+
(emit-return tail '(null))))
48414846
'(null))
48424847

48434848
((gc_preserve_begin)
@@ -4861,7 +4866,7 @@ f(x) = yt(x)
48614866
(else
48624867
(emit e)))
48634868
(if (and tail (not have-ret?))
4864-
(emit-return '(null)))
4869+
(emit-return tail '(null)))
48654870
'(null)))
48664871

48674872
;; unsupported assignment operators
@@ -4979,6 +4984,7 @@ f(x) = yt(x)
49794984
(labltable (table))
49804985
(ssavtable (table))
49814986
(current-loc 0)
4987+
(nowhere #f)
49824988
(current-file file)
49834989
(current-line line)
49844990
(locstack '())
@@ -4991,25 +4997,33 @@ f(x) = yt(x)
49914997
(set! current-loc 1)))
49924998
(set! code (cons e code))
49934999
(set! i (+ i 1))
4994-
(set! locs (cons current-loc locs)))
5000+
(set! locs (cons (if nowhere 0 current-loc) locs))
5001+
(set! nowhere #f))
49955002
(let loop ((stmts (cdr body)))
49965003
(if (pair? stmts)
49975004
(let ((e (car stmts)))
49985005
(cond ((atom? e) (emit e))
49995006
((eq? (car e) 'line)
5000-
(if (and (= current-line 0) (length= e 2) (pair? linetable))
5001-
;; (line n) after push_loc just updates the line for the new file
5002-
(begin (set-lineno! (car linetable) (cadr e))
5003-
(set! current-line (cadr e)))
5004-
(begin
5005-
(set! current-line (cadr e))
5006-
(if (pair? (cddr e))
5007-
(set! current-file (caddr e)))
5008-
(set! linetable (cons (if (null? locstack)
5009-
(make-lineinfo name current-file current-line)
5010-
(make-lineinfo name current-file current-line (caar locstack)))
5011-
linetable))
5012-
(set! current-loc (- (length linetable) 1)))))
5007+
(cond ((and (length= e 2) (not (cadr e)))
5008+
;; (line #f) marks that we are entering a generated statement
5009+
;; that should not be counted as belonging to the previous marked location,
5010+
;; for example `return` after a not-executed `if` arm in tail position.
5011+
(set! nowhere #t))
5012+
((and (= current-line 0) (length= e 2) (pair? linetable))
5013+
;; (line n) after push_loc just updates the line for the new file
5014+
(begin (set-lineno! (car linetable) (cadr e))
5015+
(set! current-line (cadr e))))
5016+
(else
5017+
(begin
5018+
(set! current-line (cadr e))
5019+
(if (pair? (cddr e))
5020+
(set! current-file (caddr e)))
5021+
(set! linetable (cons (if (null? locstack)
5022+
(make-lineinfo name current-file current-line)
5023+
(make-lineinfo name current-file current-line (caar locstack)))
5024+
linetable))
5025+
(set! linetablelen (+ linetablelen 1))
5026+
(set! current-loc (- (length linetable) 1))))))
50135027
((and (length> e 2) (eq? (car e) 'meta) (eq? (cadr e) 'push_loc))
50145028
(set! locstack (cons (list current-loc current-line current-file) locstack))
50155029
(set! current-file (caddr e))

‎test/cmdlineargs.jl

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -520,6 +520,69 @@ let exename = `$(Base.julia_cmd()) --startup-file=no --color=no`
520520
got = read(covfile, String)
521521
@test isempty(got)
522522
rm(covfile)
523+
524+
function coverage_info_for(src::String)
525+
mktemp(dir) do srcfile, io
526+
write(io, src); close(io)
527+
outfile = tempname(dir, cleanup=false)*".info"
528+
run(`$exename --code-coverage=$outfile $srcfile`)
529+
result = read(outfile, String)
530+
rm(outfile, force=true)
531+
result
532+
end
533+
end
534+
@test contains(coverage_info_for("""
535+
function cov_bug(x, p)
536+
if p > 2
537+
print("") # runs
538+
end
539+
if Base.compilerbarrier(:const, false)
540+
println("Does not run")
541+
end
542+
end
543+
function do_test()
544+
cov_bug(5, 3)
545+
end
546+
do_test()
547+
"""), """
548+
DA:1,1
549+
DA:2,1
550+
DA:3,1
551+
DA:5,1
552+
DA:6,0
553+
DA:9,1
554+
DA:10,1
555+
LH:6
556+
LF:7
557+
""")
558+
@test contains(coverage_info_for("""
559+
function cov_bug()
560+
if Base.compilerbarrier(:const, true)
561+
if Base.compilerbarrier(:const, true)
562+
if Base.compilerbarrier(:const, false)
563+
println("Does not run")
564+
end
565+
else
566+
print("Does not run either")
567+
end
568+
else
569+
print("")
570+
end
571+
return nothing
572+
end
573+
cov_bug()
574+
"""), """
575+
DA:1,1
576+
DA:2,1
577+
DA:3,1
578+
DA:4,1
579+
DA:5,0
580+
DA:8,0
581+
DA:11,0
582+
DA:13,1
583+
LH:5
584+
LF:8
585+
""")
523586
end
524587

525588
# --track-allocation

‎test/syntax.jl

Lines changed: 1 addition & 1 deletion
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 == [4, 2]
716+
@test ci.codelocs == [4, 0]
717717
@test is_return_ssavalue(m3_exprs[end])
718718
end
719719

0 commit comments

Comments
 (0)