[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

@@ -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

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;
+}
+
 }

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]