[hoisting] Instantiate cost at call instruction

Summary: When instantiating the callee's cost, we have picked up the InferBo memory at the node corresponding to the last instruction. Instead, we should pick up right at the call instruction. Picking it up later might cause arguments to go out of scope.

Reviewed By: skcho

Differential Revision: D27652474

fbshipit-source-id: 5ab35cabb

infer/src/IR/Instrs.ml

@@ -228,3 +228,8 @@ let instrs_get_normal_vars instrs =
            |> Ident.hashqueue_of_sequence ~init:res )
   in
   fold ~init:(Ident.HashQueue.create ()) ~f:do_instr instrs |> Ident.HashQueue.keys
+
+
+let find_instr_index instrs instr =
+  let instrs = get_underlying_not_reversed instrs in
+  Array.findi instrs ~f:(fun _index i -> Sil.equal_instr i instr) |> Option.map ~f:fst

infer/src/IR/Instrs.mli

@@ -65,6 +65,8 @@ val last : _ t -> Sil.instr option
 
 val find_map : _ t -> f:(Sil.instr -> 'a option) -> 'a option
 
+val find_instr_index : not_reversed t -> Sil.instr -> int option
+
 val pp : Pp.env -> Format.formatter -> _ t -> unit
 
 val fold : (_ t, Sil.instr, 'a) Container.fold

infer/src/absint/ProcCfg.ml

@@ -325,6 +325,8 @@ module OneInstrPerNode (Base : S with module Node = DefaultNode) : sig
     S with type t = Base.t and module Node = InstrNode and type instrs_dir = Instrs.not_reversed
 
   val last_of_underlying_node : Procdesc.Node.t -> Node.t
+
+  val of_instr_opt : Procdesc.Node.t -> Sil.instr -> Node.t option
 end = struct
   type t = Base.t
 
@@ -341,6 +343,11 @@ end = struct
 
   let last_of_node node = (node, max 0 (Instrs.count (Base.instrs node) - 1))
 
+  let of_instr_opt node instr =
+    let instrs = Procdesc.Node.get_instrs node in
+    Instrs.find_instr_index instrs instr |> Option.map ~f:(fun index -> (node, index))
+
+
   let last_of_underlying_node = last_of_node
 
   let fold_normal_succs _ _ ~init:_ ~f:_ = (* not used *) assert false

infer/src/absint/ProcCfg.mli

@@ -109,6 +109,8 @@ module OneInstrPerNode (Base : S with module Node = DefaultNode) : sig
     S with type t = Base.t and module Node = InstrNode and type instrs_dir = Instrs.not_reversed
 
   val last_of_underlying_node : Procdesc.Node.t -> Node.t
+
+  val of_instr_opt : Procdesc.Node.t -> Sil.instr -> Node.t option
 end
 
 module NormalOneInstrPerNode : module type of OneInstrPerNode (Normal)

infer/src/cost/hoisting.ml

@@ -11,7 +11,8 @@ module BasicCost = CostDomain.BasicCost
 
 module Call = struct
   type t =
-    { loc: Location.t
+    { instr: Sil.instr
+    ; loc: Location.t
     ; pname: Procname.t
     ; node: Procdesc.Node.t
     ; args: (Exp.t * Typ.t) list
@@ -40,7 +41,7 @@ let add_if_hoistable inv_vars instr node source_nodes idom hoistable_calls =
          List.for_all ~f:(fun source -> Dominators.dominates idom node source) source_nodes
          && (* Check condition (2); id should be invariant already *)
          LoopInvariant.InvariantVars.mem (Var.of_id ret_id) inv_vars ->
-      HoistCalls.add {pname; loc; node; args; ret} hoistable_calls
+      HoistCalls.add {instr; pname; loc; node; args; ret} hoistable_calls
   | _ ->
       hoistable_calls
 
@@ -100,8 +101,8 @@ let do_report extract_cost_if_expensive proc_desc err_log (Call.{pname; loc} as
 
 
 let get_cost_if_expensive tenv integer_type_widths get_callee_cost_summary_and_formals
-    inferbo_invariant_map inferbo_get_summary Call.{pname; node; ret; args} =
+    inferbo_invariant_map inferbo_get_summary Call.{instr; pname; node; ret; args} =
-  let last_node = InstrCFG.last_of_underlying_node node in
+  let last_node = Option.value_exn (InstrCFG.of_instr_opt node instr) in
   let inferbo_mem =
     let instr_node_id = InstrCFG.Node.id last_node in
     Option.value_exn (BufferOverrunAnalysis.extract_pre instr_node_id inferbo_invariant_map)

infer/tests/codetoanalyze/java/hoistingExpensive/HoistExpensive.java

@@ -29,7 +29,7 @@ class HoistExpensive {
   }
 
   // call to cheap_dont_hoist will NOT be hoisted since it is cheap.
-  void instantiated_cheap_hoist(int size) {
+  void instantiated_cheap_dont_hoist(int size) {
     for (int i = 0; i < size; i++) {
       cheap_dont_hoist(1);
     }

infer/tests/codetoanalyze/java/hoistingExpensive/issues.exp

@@ -4,8 +4,8 @@ codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.cheap_i
 codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.cheap_iterator_dont_hoist(java.util.ArrayList):void, 3, INVARIANT_CALL, no_bucket, ERROR, [The call to int HoistExpensive.incr(int) at line 42 is loop-invariant]
 codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.getLeakSummary():java.lang.String, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function String HoistExpensive.getLeakSummary()]
 codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.incr(int):int, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function int HoistExpensive.incr(int)]
-codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.instantiated_cheap_hoist(int):void, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function void HoistExpensive.instantiated_cheap_hoist(int)]
+codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.instantiated_cheap_dont_hoist(int):void, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function void HoistExpensive.instantiated_cheap_dont_hoist(int)]
-codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.instantiated_cheap_hoist(int):void, 2, INVARIANT_CALL, no_bucket, ERROR, [The call to void HoistExpensive.cheap_dont_hoist(int) at line 34 is loop-invariant]
+codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.instantiated_cheap_dont_hoist(int):void, 2, INVARIANT_CALL, no_bucket, ERROR, [The call to void HoistExpensive.cheap_dont_hoist(int) at line 34 is loop-invariant]
 codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.symbolic_expensive_hoist(int):void, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function void HoistExpensive.symbolic_expensive_hoist(int)]
 codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.symbolic_expensive_hoist(int):void, 2, EXPENSIVE_LOOP_INVARIANT_CALL, no_bucket, ERROR, [The call to void HoistExpensive.cheap_dont_hoist(int) at line 27 is loop-invariant,with estimated cost 5 + 10 ⋅ size, degree = 1,{size},Call to void HoistExpensive.cheap_dont_hoist(int),Loop]
 codetoanalyze/java/hoistingExpensive/HoistExpensive.java, HoistExpensive.symbolic_expensive_iterator_hoist(int,java.util.ArrayList):void, 0, PURE_FUNCTION, no_bucket, ERROR, [Side-effect free function void HoistExpensive.symbolic_expensive_iterator_hoist(int,ArrayList)]