From 8d0f141974f2404c2398860e3c0fd7257178f9c2 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Wed, 29 Nov 2017 14:25:44 -0800 Subject: [PATCH] [dead stores] identify dead stores involving struct values Summary: As da319 points out, we did not handle this case correctly before. There were a few reasons why: (1) An assignment like `struct S s = mk_s()` gets translated as `tmp = mk_s(); S(&s, tmp)`, so we didn't see the write to `s`. (2) We counted uses of variables in destructors and dummy `_ = *s` assignments as reads, which meant that any struct values were considered as live. This diff fixes these limitations so we can report on dead stores of struct values. Reviewed By: da319 Differential Revision: D6327564 fbshipit-source-id: 2ead4be --- infer/src/checkers/liveness.ml | 57 +++++++++++++++---- .../cpp/liveness/dead_stores.cpp | 32 +++++++++++ .../codetoanalyze/cpp/liveness/issues.exp | 2 + 3 files changed, 79 insertions(+), 12 deletions(-) diff --git a/infer/src/checkers/liveness.ml b/infer/src/checkers/liveness.ml index 5e3797dca..a5f1afef9 100644 --- a/infer/src/checkers/liveness.ml +++ b/infer/src/checkers/liveness.ml @@ -34,7 +34,32 @@ module TransferFunctions (CFG : ProcCfg.S) = struct ~init:astate' pvars + let add_live_actuals actuals call_exp live_acc = + let add_live_actuals_ exps acc = + List.fold exps ~f:(fun acc_ (exp, _) -> exp_add_live exp acc_) ~init:acc + in + match call_exp with + | Exp.Const Cfun (Typ.Procname.ObjC_Cpp _ as pname) when Typ.Procname.is_constructor pname -> ( + match + (* first actual passed to a C++ constructor is actually written, not read *) + actuals + with + | (Exp.Lvar pvar, _) :: exps -> + Domain.remove (Var.of_pvar pvar) live_acc |> add_live_actuals_ exps + | exps -> + add_live_actuals_ exps live_acc ) + | _ -> + add_live_actuals_ actuals live_acc + + let exec_instr astate _ _ = function + | Sil.Load (lhs_id, _, _, _) when Ident.is_none lhs_id -> + (* dummy deref inserted by frontend--don't count as a read *) + astate + | Sil.Call (_, Exp.Const Cfun (Typ.Procname.ObjC_Cpp _ as pname), _, _, _) + when Typ.Procname.is_destructor pname -> + (* don't count destructor calls as a read *) + astate | Sil.Load (lhs_id, rhs_exp, _, _) -> Domain.remove (Var.of_id lhs_id) astate |> exp_add_live rhs_exp | Sil.Store (Lvar lhs_pvar, _, rhs_exp, _) -> @@ -47,12 +72,11 @@ module TransferFunctions (CFG : ProcCfg.S) = struct exp_add_live lhs_exp astate |> exp_add_live rhs_exp | Sil.Prune (exp, _, _, _) -> exp_add_live exp astate - | Sil.Call (ret_id, call_exp, params, _, _) -> + | Sil.Call (ret_id, call_exp, actuals, _, _) -> Option.value_map ~f:(fun (ret_id, _) -> Domain.remove (Var.of_id ret_id) astate) ~default:astate ret_id - |> exp_add_live call_exp - |> fun x -> List.fold_right ~f:exp_add_live (List.map ~f:fst params) ~init:x + |> exp_add_live call_exp |> add_live_actuals actuals call_exp | Sil.Declare_locals _ | Remove_temps _ | Abstract _ | Nullify _ -> astate @@ -77,17 +101,26 @@ let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary = | _ -> false in + let should_report pvar live_vars = + not + ( Pvar.is_frontend_tmp pvar || Pvar.is_return pvar || Pvar.is_global pvar + || Domain.mem (Var.of_pvar pvar) live_vars || Procdesc.is_captured_var proc_desc pvar ) + in + let log_report pvar loc = + let issue_id = IssueType.dead_store.unique_id in + let message = F.asprintf "The value written to %a is never used" (Pvar.pp Pp.text) pvar in + let ltr = [Errlog.make_trace_element 0 loc "Write of unused value" []] in + let exn = Exceptions.Checkers (issue_id, Localise.verbatim_desc message) in + Reporting.log_error summary ~loc ~ltr exn + in let report_dead_store live_vars = function | Sil.Store (Lvar pvar, _, rhs_exp, loc) - when not - ( Pvar.is_frontend_tmp pvar || Pvar.is_return pvar || Pvar.is_global pvar - || Domain.mem (Var.of_pvar pvar) live_vars || Procdesc.is_captured_var proc_desc pvar - || is_sentinel_exp rhs_exp ) -> - let issue_id = IssueType.dead_store.unique_id in - let message = F.asprintf "The value written to %a is never used" (Pvar.pp Pp.text) pvar in - let ltr = [Errlog.make_trace_element 0 loc "Write of unused value" []] in - let exn = Exceptions.Checkers (issue_id, Localise.verbatim_desc message) in - Reporting.log_error summary ~loc ~ltr exn + when should_report pvar live_vars && not (is_sentinel_exp rhs_exp) -> + log_report pvar loc + | Sil.Call + (None, Exp.Const Cfun (Typ.Procname.ObjC_Cpp _ as pname), (Exp.Lvar pvar, _) :: _, loc, _) + when Typ.Procname.is_constructor pname && should_report pvar live_vars -> + log_report pvar loc | _ -> () in diff --git a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp index 273b1b067..19f6eaefd 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp +++ b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp @@ -280,4 +280,36 @@ int* sentinel_ptr_ok(int* j) { return i; } +struct S { + ~S() {} +}; + +typedef S&& B; + +S mk_s() { + S s; + return s; +}; + +// s gets read by the destructor for S +void dead_struct_value1_bad() { S s = mk_s(); } + +// need to handle operator= in order to detect this case +void FN_dead_struct_value2_bad() { + S s = mk_s(); + s = mk_s(); +} + +void dead_struct_rvalue_ref_bad() { B b = mk_s(); } + +S struct_value_used_ok() { + S s = mk_s(); + return s; +} + +B& struct_rvalue_ref_used_ok() { + B b = mk_s(); + return b; +} + } diff --git a/infer/tests/codetoanalyze/cpp/liveness/issues.exp b/infer/tests/codetoanalyze/cpp/liveness/issues.exp index 70468c247..3859fd733 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/issues.exp +++ b/infer/tests/codetoanalyze/cpp/liveness/issues.exp @@ -1,5 +1,7 @@ codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::FP_assign_array_tricky_ok, 3, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_pointer_bad, 2, DEAD_STORE, [Write of unused value] +codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_struct_rvalue_ref_bad, 0, DEAD_STORE, [Write of unused value] +codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_struct_value1_bad, 0, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_then_live_bad, 1, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::easy_bad, 0, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::init_capture_no_call_bad, 1, DEAD_STORE, [Write of unused value]