From 913dfb8c378d236c24895d9683733f70f3178055 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ezgi=20=C3=87i=C3=A7ek?= Date: Mon, 12 Apr 2021 06:10:41 -0700 Subject: [PATCH] [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 | 5 +++++ infer/src/IR/Instrs.mli | 2 ++ infer/src/absint/ProcCfg.ml | 7 +++++++ infer/src/absint/ProcCfg.mli | 2 ++ infer/src/cost/hoisting.ml | 9 +++++---- .../java/hoistingExpensive/HoistExpensive.java | 2 +- .../codetoanalyze/java/hoistingExpensive/issues.exp | 4 ++-- 7 files changed, 24 insertions(+), 7 deletions(-) diff --git a/infer/src/IR/Instrs.ml b/infer/src/IR/Instrs.ml index e38d277ca..2cfc3d672 100644 --- a/infer/src/IR/Instrs.ml +++ b/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 diff --git a/infer/src/IR/Instrs.mli b/infer/src/IR/Instrs.mli index 9fd7dd213..bf19728ee 100644 --- a/infer/src/IR/Instrs.mli +++ b/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 diff --git a/infer/src/absint/ProcCfg.ml b/infer/src/absint/ProcCfg.ml index 04521663f..c423d2da7 100644 --- a/infer/src/absint/ProcCfg.ml +++ b/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 diff --git a/infer/src/absint/ProcCfg.mli b/infer/src/absint/ProcCfg.mli index 99bb94e3e..a059bdc5e 100644 --- a/infer/src/absint/ProcCfg.mli +++ b/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) diff --git a/infer/src/cost/hoisting.ml b/infer/src/cost/hoisting.ml index 831c63d3e..9f1aaf808 100644 --- a/infer/src/cost/hoisting.ml +++ b/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} = - let last_node = InstrCFG.last_of_underlying_node node in + inferbo_invariant_map inferbo_get_summary Call.{instr; pname; node; ret; args} = + 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) diff --git a/infer/tests/codetoanalyze/java/hoistingExpensive/HoistExpensive.java b/infer/tests/codetoanalyze/java/hoistingExpensive/HoistExpensive.java index b62497f7f..96cc8c91e 100644 --- a/infer/tests/codetoanalyze/java/hoistingExpensive/HoistExpensive.java +++ b/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); } diff --git a/infer/tests/codetoanalyze/java/hoistingExpensive/issues.exp b/infer/tests/codetoanalyze/java/hoistingExpensive/issues.exp index 7dadbc818..c80f03c72 100644 --- a/infer/tests/codetoanalyze/java/hoistingExpensive/issues.exp +++ b/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_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, 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_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)]