[liveness] don't report on dead stores of variables captured by reference in a lambda

Summary:
You can capture a variable by reference in a lambda, assign to it, and then invoke the lambda.
This looks like a dead store from the perspective of the current analysis.

This diff mitigates the problem by computing an additional analysis that tracks variables captured by ref at each program point.
It refuses to report a dead store on a variable that has already been captured by reference.

Later, we might want to incorporate the results of this analysis directly into the liveness analysis instead of just using it to gate reporting.

Reviewed By: jeremydubreil

Differential Revision: D7090291

fbshipit-source-id: 25eeffa

infer/src/IR/Exp.ml

@@ -197,6 +197,23 @@ let get_vars exp =
   get_vars_ exp ([], [])
 
 
+let fold_captured ~f exp acc =
+  let rec fold_captured_ exp captured_acc =
+    match exp with
+    | Cast (_, e) | UnOp (_, e, _) | Lfield (e, _, _) | Exn e | Sizeof {dynamic_length= Some e} ->
+        fold_captured_ e captured_acc
+    | BinOp (_, e1, e2) | Lindex (e1, e2) ->
+        fold_captured_ e1 captured_acc |> fold_captured_ e2
+    | Closure {captured_vars} ->
+        List.fold captured_vars
+          ~f:(fun acc (_, captured_pvar, _) -> f acc captured_pvar)
+          ~init:captured_acc
+    | Const _ | Lvar _ | Var _ | Sizeof _ ->
+        captured_acc
+  in
+  fold_captured_ exp acc
+
+
 (** Pretty print an expression. *)
 let rec pp_ pe pp_t f e =
   let pp_exp = pp_ pe pp_t in

infer/src/IR/Exp.mli

@@ -119,6 +119,9 @@ val lt : t -> t -> t
 val get_vars : t -> Ident.t list * Pvar.t list
 (** Extract the ids and pvars from an expression *)
 
+val fold_captured : f:('a -> Pvar.t -> 'a) -> t -> 'a -> 'a
+(** Extract the program variables captured by this expression *)
+
 val pp_printenv : Pp.env -> (Pp.env -> F.formatter -> Typ.t -> unit) -> F.formatter -> t -> unit
 
 val pp : F.formatter -> t -> unit

infer/src/checkers/liveness.ml

@@ -13,7 +13,8 @@ module L = Logging
 
 (** backward analysis for computing set of maybe-live variables at each program point *)
 
-module Domain = AbstractDomain.FiniteSet (Var)
+module VarSet = AbstractDomain.FiniteSet (Var)
+module Domain = VarSet
 
 (* compilers 101-style backward transfer functions for liveness analysis. gen a variable when it is
    read, kill the variable when it is assigned *)
@@ -95,11 +96,32 @@ let matcher_scope_guard =
   |> QualifiedCppName.Match.of_fuzzy_qual_names
 
 
+module CapturedByRefTransferFunctions (CFG : ProcCfg.S) = struct
+  module CFG = CFG
+  module Domain = VarSet
+
+  type extras = ProcData.no_extras
+
+  let exec_instr astate _ _ instr =
+    List.fold (Sil.instr_get_exps instr)
+      ~f:(fun acc exp ->
+        Exp.fold_captured ~f:(fun acc pvar -> Domain.add (Var.of_pvar pvar) acc) exp acc )
+      ~init:astate
+end
+
+module CapturedByRefAnalyzer =
+  AbstractInterpreter.Make (ProcCfg.Exceptional) (CapturedByRefTransferFunctions)
+
+let get_captured_by_ref_invariant_map proc_desc proc_data =
+  let cfg = ProcCfg.Exceptional.from_pdesc proc_desc in
+  CapturedByRefAnalyzer.exec_cfg cfg proc_data ~initial:VarSet.empty ~debug:false
+
+
 let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary =
+  let proc_data = ProcData.make_default proc_desc tenv in
+  let captured_by_ref_invariant_map = get_captured_by_ref_invariant_map proc_desc proc_data in
   let cfg = CFG.from_pdesc proc_desc in
-  let invariant_map =
-    Analyzer.exec_cfg cfg (ProcData.make_default proc_desc tenv) ~initial:Domain.empty ~debug:true
-  in
+  let invariant_map = Analyzer.exec_cfg cfg proc_data ~initial:Domain.empty ~debug:false in
   (* we don't want to report in harmless cases like int i = 0; if (...) { i = ... } else { i = ... }
      that create an intentional dead store as an attempt to imitate default value semantics.
      use dead stores to a "sentinel" value as a heuristic for ignoring this case *)
@@ -119,9 +141,10 @@ let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary =
     | _ ->
         false
   in
-  let should_report pvar typ live_vars =
+  let should_report pvar typ live_vars captured_by_ref_vars =
     not
       ( Pvar.is_frontend_tmp pvar || Pvar.is_return pvar || Pvar.is_global pvar
+      || VarSet.mem (Var.of_pvar pvar) captured_by_ref_vars
       || Domain.mem (Var.of_pvar pvar) live_vars || Procdesc.is_captured_var proc_desc pvar
       || is_scope_guard typ || Procdesc.has_modify_in_block_attr proc_desc pvar )
   in
@@ -134,24 +157,36 @@ let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary =
     let exn = Exceptions.Checkers (IssueType.dead_store, Localise.verbatim_desc message) in
     Reporting.log_error summary ~loc ~ltr exn
   in
-  let report_dead_store live_vars = function
+  let report_dead_store live_vars captured_by_ref_vars = function
     | Sil.Store (Lvar pvar, typ, rhs_exp, loc)
-      when should_report pvar typ live_vars && not (is_sentinel_exp rhs_exp) ->
+      when should_report pvar typ live_vars captured_by_ref_vars && not (is_sentinel_exp rhs_exp) ->
         log_report pvar typ loc
     | Sil.Call
         (None, Exp.Const Cfun (Typ.Procname.ObjC_Cpp _ as pname), (Exp.Lvar pvar, typ) :: _, loc, _)
-      when Typ.Procname.is_constructor pname && should_report pvar typ live_vars ->
+      when Typ.Procname.is_constructor pname
+           && should_report pvar typ live_vars captured_by_ref_vars ->
         log_report pvar typ loc
     | _ ->
         ()
   in
   let report_on_node node =
+    let captured_by_ref_vars =
+      match
+        CapturedByRefAnalyzer.extract_post
+          (ProcCfg.Exceptional.id (CFG.underlying_node node))
+          captured_by_ref_invariant_map
+      with
+      | Some post ->
+          post
+      | None ->
+          VarSet.empty
+    in
     List.iter (CFG.instr_ids node) ~f:(fun (instr, node_id_opt) ->
         match node_id_opt with
         | Some node_id -> (
           match Analyzer.extract_pre node_id invariant_map with
           | Some live_vars ->
-              report_dead_store live_vars instr
+              report_dead_store live_vars captured_by_ref_vars instr
           | None ->
               () )
         | None ->

infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp

@@ -130,12 +130,6 @@ void by_ref1_ok(int& ref) { ref = 7; }
 
 void by_ref2_ok(int& ref) { ref++; }
 
-int capture_by_ref3_ok() {
-  int x = 1;
-  [&](auto y) { x += y; }(3);
-  return x;
-}
-
 int plus_plus_ok() {
   int x = 1;
   return ++x;
@@ -183,6 +177,37 @@ int capture_by_ref2_ok() {
   return x + y;
 }
 
+int capture_by_ref3_ok() {
+  int x = 1;
+  [&](auto y) { x += y; }(3);
+  return x;
+}
+
+int capture_by_ref4_ok() {
+  int x = 1;
+  auto lambda = [&] { return x; };
+  x = 2; // not a dead store; updates captured x
+  return lambda();
+}
+
+int dead_store_before_capture_by_ref_bad() {
+  int x = 1; // this is dead. should report it even though x is captured by ref
+             // later on
+  x = 2;
+  auto lambda = [&] { return x; };
+  x = 2;
+  return lambda();
+}
+
+// frontend can't tell the difference between capture by ref and capture by
+// value yet.
+int FN_capture_by_value_bad() {
+  int x = 1;
+  auto lambda = [=] { return x; };
+  x = 2; // this is dead
+  return lambda();
+}
+
 int FN_capture_by_ref_reuseBad() {
   int x = 1;
   [&x]() {

infer/tests/codetoanalyze/cpp/liveness/issues.exp

@@ -1,12 +1,13 @@
 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_store_before_capture_by_ref_bad, 1, DEAD_STORE, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_struct_no_destructor_bad, 0, 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_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]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::init_capture_reassign_bad, 1, DEAD_STORE, [Write of unused value]
-codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::lambda_bad::lambda_dead_stores.cpp:153:11_operator(), 1, DEAD_STORE, [Write of unused value]
+codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::lambda_bad::lambda_dead_stores.cpp:147:11_operator(), 1, DEAD_STORE, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus1_bad, 2, DEAD_STORE, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus2_bad, 2, DEAD_STORE, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus3_bad, 2, DEAD_STORE, [Write of unused value]