From dcaabe8c60690ff6ab37dbdf9f4bbc576da323d1 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. (cherry picked from commit 61fc907a225eb642fd180257a02e5951336dabe4) --- base/compiler/ssair/inlining.jl | 4 +- base/compiler/ssair/passes.jl | 6 ++- src/julia-syntax.scm | 82 +++++++++++++++++++-------------- test/cmdlineargs.jl | 63 +++++++++++++++++++++++++ test/syntax.jl | 2 +- 5 files changed, 120 insertions(+), 37 deletions(-) diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index 019df328ab9a12..75e63f5c6b2e4d 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -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 diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index 3df55e38b35120..39f1445d8c02da 100644 --- a/base/compiler/ssair/passes.jl +++ b/base/compiler/ssair/passes.jl @@ -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 diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm index b5705c55b6e615..c6c411d2a21f27 100644 --- a/src/julia-syntax.scm +++ b/src/julia-syntax.scm @@ -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 @@ -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 '()))) @@ -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 @@ -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))))) ((=) @@ -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) @@ -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 @@ -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)) @@ -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)) @@ -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)) @@ -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))))) @@ -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)))) @@ -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))) @@ -4805,12 +4810,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))))) @@ -4818,7 +4823,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)) @@ -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 @@ -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) @@ -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 @@ -4979,6 +4984,7 @@ f(x) = yt(x) (labltable (table)) (ssavtable (table)) (current-loc 0) + (nowhere #f) (current-file file) (current-line line) (locstack '()) @@ -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)) diff --git a/test/cmdlineargs.jl b/test/cmdlineargs.jl index 1730fe8179857f..12e0c1fe2d5025 100644 --- a/test/cmdlineargs.jl +++ b/test/cmdlineargs.jl @@ -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 diff --git a/test/syntax.jl b/test/syntax.jl index 3516c306c42d40..e529e0f292df1c 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 == [4, 2] + @test ci.codelocs == [4, 0] @test is_return_ssavalue(m3_exprs[end]) end