From aa0716a19ae87c38621d8450c9738089bcc8c6c0 Mon Sep 17 00:00:00 2001 From: Jeff Bezanson Date: Tue, 20 Feb 2024 16:42:30 -0500 Subject: [PATCH] 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. --- base/compiler/ssair/inlining.jl | 4 +- base/compiler/ssair/passes.jl | 6 ++- src/julia-syntax.scm | 83 +++++++++++++++++++-------------- test/cmdlineargs.jl | 63 +++++++++++++++++++++++++ test/show.jl | 2 +- test/syntax.jl | 2 +- 6 files changed, 121 insertions(+), 39 deletions(-) diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index 74d3987ee02572..d4228cf3c4454b 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -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 diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index fe587f82a9def1..5ea21c85427be1 100644 --- a/base/compiler/ssair/passes.jl +++ b/base/compiler/ssair/passes.jl @@ -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 diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm index 674a04cb1e5635..7d9f8711ec7146 100644 --- a/src/julia-syntax.scm +++ b/src/julia-syntax.scm @@ -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 @@ -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 '()))) @@ -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 @@ -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))))) ((=) @@ -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) @@ -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 @@ -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)) @@ -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)) @@ -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)) @@ -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))))) @@ -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)))) @@ -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))) @@ -4855,12 +4860,12 @@ 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))))) @@ -4868,7 +4873,7 @@ f(x) = yt(x) ((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)) @@ -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 @@ -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) @@ -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 @@ -5027,6 +5032,7 @@ f(x) = yt(x) (labltable (table)) (ssavtable (table)) (current-loc 0) + (nowhere #f) (current-file file) (current-line line) (locstack '()) @@ -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)) diff --git a/test/cmdlineargs.jl b/test/cmdlineargs.jl index 434f6be0d8b059..3af31965e63fc0 100644 --- a/test/cmdlineargs.jl +++ b/test/cmdlineargs.jl @@ -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 diff --git a/test/show.jl b/test/show.jl index d6a691029d60a8..66286f4df12ba8 100644 --- a/test/show.jl +++ b/test/show.jl @@ -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) diff --git a/test/syntax.jl b/test/syntax.jl index a0021864da0e07..25d3e7282818ac 100644 --- a/test/syntax.jl +++ b/test/syntax.jl @@ -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