[cost] If the modeled cost is Top, underestimate the cost

Summary:
**Existing heuristic**: If we have a call `foo(n)` that has no model and summary for `foo`, we underestimate its cost as constant[1].

However, if we have a model for `foo` (e.g.with modeled cost O(n)) but applying the model to arguments causes the cost to be Top (e.g if `n` has Top size), then we could have Top-poisoning where all the callers up the call chain will have Top costs [2].

To prevent these unintended Top-poisioning when adding models, this diff applies *the same heuristic* to modeled calls with Top cost and gives them constant cost. This way, when adding models, we wouldn't be introducing more Tops than if we were to have no models in the first place.

[1] This is problematic in itself and causes many FPs at diff time, but otherwise we would be getting Tops everywhere and would not be able to give any meaningful cost. E.g. for fblite, if we were to give unknown calls Top cost, #procedures with Top cost increases form 5% to 38% and #procedures with linear cost reduces by 99.75%.
[2] This was observed for `containsValue` for Instagram where %Tops increased by 88% :(

Reviewed By: skcho

Differential Revision: D26174644

fbshipit-source-id: 232354923

infer/src/cost/cost.ml

@@ -62,12 +62,22 @@ module InstrBasicCostWithReason = struct
         && Procdesc.is_objc_arc_on callee_pdesc )
 
 
+  let get_modeled_cost_unless_top ~default modeled_cost =
+    if BasicCost.is_top modeled_cost then
+      (* If we get Top modeled cost, this could cause
+         Top-poisoning up the call chain, so instead, we keep
+         underestimating the cost as we do with the case where we
+         have no summary below.*)
+      default
+    else BasicCostWithReason.of_basic_cost modeled_cost
+
+
   let dispatch_operation tenv callee_pname callee_cost_opt fun_arg_list get_summary model_env ret
       inferbo_mem =
     match CostModels.Call.dispatch tenv callee_pname fun_arg_list with
     | Some model ->
-        BasicCostWithReason.of_basic_cost
-          (model {get_summary; model_env= Lazy.force model_env} ~ret inferbo_mem)
+        let modeled_cost = model {get_summary; model_env= Lazy.force model_env} ~ret inferbo_mem in
+        get_modeled_cost_unless_top ~default:(BasicCostWithReason.one ()) modeled_cost
     | None -> (
       match callee_cost_opt with
       | Some callee_cost ->
@@ -83,31 +93,33 @@ module InstrBasicCostWithReason = struct
   let dispatch_autoreleasepool tenv callee_pname callee_cost_opt fun_arg_list
       ({get_summary} as extras) model_env ((_, ret_typ) as ret) cfg loc inferbo_mem :
       BasicCostWithReason.t =
+    let fun_cost =
+      if
+        is_objc_call_from_no_arc_to_arc extras cfg callee_pname
+        && Typ.is_pointer_to_objc_non_tagged_class ret_typ
+        && not (return_object_owned_by_caller callee_pname)
+      then
+        let autoreleasepool_trace =
+          Bounds.BoundTrace.of_arc_from_non_arc (Procname.to_string callee_pname) loc
+        in
+        BasicCostWithReason.one ~autoreleasepool_trace ()
+      else BasicCostWithReason.zero
+    in
     match CostAutoreleaseModels.Call.dispatch tenv callee_pname fun_arg_list with
     | Some model ->
         let autoreleasepool_size =
           model {get_summary; model_env= Lazy.force model_env} ~ret inferbo_mem
         in
-        BasicCostWithReason.of_basic_cost autoreleasepool_size
+        get_modeled_cost_unless_top ~default:fun_cost autoreleasepool_size
     | None ->
-        let fun_cost =
-          if
-            is_objc_call_from_no_arc_to_arc extras cfg callee_pname
-            && Typ.is_pointer_to_objc_non_tagged_class ret_typ
-            && not (return_object_owned_by_caller callee_pname)
-          then
-            let autoreleasepool_trace =
-              Bounds.BoundTrace.of_arc_from_non_arc (Procname.to_string callee_pname) loc
-            in
-            BasicCostWithReason.one ~autoreleasepool_trace ()
-          else BasicCostWithReason.zero
-        in
         Option.value_map ~default:fun_cost ~f:(BasicCostWithReason.plus fun_cost) callee_cost_opt
 
 
   let dispatch_allocation tenv callee_pname callee_cost_opt : BasicCostWithReason.t =
     CostAllocationModels.ProcName.dispatch tenv callee_pname
-    |> Option.value ~default:(Option.value callee_cost_opt ~default:BasicCostWithReason.zero)
+    |> Option.value_map ~default:(Option.value callee_cost_opt ~default:BasicCostWithReason.zero)
+         ~f:(fun modeled_cost ->
+           get_modeled_cost_unless_top ~default:BasicCostWithReason.zero modeled_cost )
 
 
   let dispatch_func_ptr_call {inferbo_invariant_map; integer_type_widths} instr_node fun_exp

infer/src/cost/costAllocationModels.ml

@@ -6,16 +6,16 @@
  *)
 
 open! IStd
-module BasicCostWithReason = CostDomain.BasicCostWithReason
+module BasicCost = CostDomain.BasicCost
 
 module ProcName = struct
-  let dispatch : (Tenv.t, BasicCostWithReason.t, unit) ProcnameDispatcher.ProcName.dispatcher =
+  let dispatch : (Tenv.t, BasicCost.t, unit) ProcnameDispatcher.ProcName.dispatcher =
     let open ProcnameDispatcher.ProcName in
     let match_builtin builtin _ s = String.equal s (Procname.get_method builtin) in
     make_dispatcher
-      [ +match_builtin BuiltinDecl.__new <>--> BasicCostWithReason.one ()
-      ; +match_builtin BuiltinDecl.__new_array <>--> BasicCostWithReason.one ()
-      ; +match_builtin BuiltinDecl.__objc_alloc_no_fail <>--> BasicCostWithReason.one ()
-      ; +match_builtin BuiltinDecl.malloc <>--> BasicCostWithReason.one ()
-      ; +match_builtin BuiltinDecl.malloc_no_fail <>--> BasicCostWithReason.one () ]
+      [ +match_builtin BuiltinDecl.__new <>--> BasicCost.one ()
+      ; +match_builtin BuiltinDecl.__new_array <>--> BasicCost.one ()
+      ; +match_builtin BuiltinDecl.__objc_alloc_no_fail <>--> BasicCost.one ()
+      ; +match_builtin BuiltinDecl.malloc <>--> BasicCost.one ()
+      ; +match_builtin BuiltinDecl.malloc_no_fail <>--> BasicCost.one () ]
 end

infer/src/cost/costAllocationModels.mli

@@ -8,6 +8,5 @@
 open! IStd
 
 module ProcName : sig
-  val dispatch :
-    (Tenv.t, CostDomain.BasicCostWithReason.t, unit) ProcnameDispatcher.ProcName.dispatcher
+  val dispatch : (Tenv.t, CostDomain.BasicCost.t, unit) ProcnameDispatcher.ProcName.dispatcher
 end

infer/tests/codetoanalyze/objc/performance/NSString.m

@@ -129,7 +129,7 @@ bool string_has_prefix_linear(NSString* str, NSString* prefix) {
 
 @implementation DummyClass
 
-+ (void)call_string_by_appending_string_constant_FP {
++ (void)call_string_by_appending_string_constant {
   NSString* s = [NSStringFromClass(self) stringByAppendingString:@"abc"];
 }
 

infer/tests/codetoanalyze/objc/performance/cost-issues.exp

@@ -90,7 +90,7 @@ codetoanalyze/objc/performance/NSSet.m, nsset_init_with_array_linear, 6 + 3 ⋅
 codetoanalyze/objc/performance/NSSet.m, nsset_init_with_set_constant, 5,  OnUIThread:false, []
 codetoanalyze/objc/performance/NSSet.m, nsset_iterate_linear, 5 + 8 ⋅ (set->elements.length.ub + 1),  OnUIThread:false, [{set->elements.length.ub + 1},Loop]
 codetoanalyze/objc/performance/NSSet.m, nsset_next_object_linear, 4 + 5 ⋅ (set->elements.length.ub + 1),  OnUIThread:false, [{set->elements.length.ub + 1},Loop]
-codetoanalyze/objc/performance/NSString.m, DummyClass.call_string_by_appending_string_constant_FP, ⊤,  OnUIThread:false, [Unbounded loop,Modeled call to NSString.stringByAppendingString:]
+codetoanalyze/objc/performance/NSString.m, DummyClass.call_string_by_appending_string_constant, 7,  OnUIThread:false, []
 codetoanalyze/objc/performance/NSString.m, DummyClass.dealloc, 0,  OnUIThread:false, []
 codetoanalyze/objc/performance/NSString.m, call_component_separated_by_char_constant, 44,  OnUIThread:false, []
 codetoanalyze/objc/performance/NSString.m, call_init_with_string_constant, 13,  OnUIThread:false, []

infer/tests/codetoanalyze/objc/performance/issues.exp

@@ -11,7 +11,6 @@ codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_set_linear, 2, INTEGER
 codetoanalyze/objc/performance/NSSet.m, nsset_enumerator_linear, 7, INTEGER_OVERFLOW_L5, no_bucket, ERROR, [<LHS trace>,Assignment,<RHS trace>,Assignment,Binary operation: ([0, +oo] + [1, set->elements.length.ub + 1]):signed64]
 codetoanalyze/objc/performance/NSSet.m, nsset_init_constant, 3, CONDITION_ALWAYS_FALSE, no_bucket, WARNING, [Here]
 codetoanalyze/objc/performance/NSSet.m, nsset_iterate_linear, 3, INTEGER_OVERFLOW_L5, no_bucket, ERROR, [<LHS trace>,Assignment,<RHS trace>,Assignment,Binary operation: ([0, +oo] + [1, set->elements.length.ub + 1]):signed64]
-codetoanalyze/objc/performance/NSString.m, DummyClass.call_string_by_appending_string_constant_FP, 0, INFINITE_EXECUTION_TIME, no_bucket, ERROR, [Unbounded loop,Modeled call to NSString.stringByAppendingString:]
 codetoanalyze/objc/performance/NSString.m, init_string_constant, 2, CONDITION_ALWAYS_FALSE, no_bucket, WARNING, [Here]
 codetoanalyze/objc/performance/NSString.m, replace_linear_FP, 0, INFINITE_EXECUTION_TIME, no_bucket, ERROR, [Unbounded loop,Loop]
 codetoanalyze/objc/performance/NSString.m, replace_linear_FP, 2, INTEGER_OVERFLOW_U5, no_bucket, ERROR, [<LHS trace>,Unknown value from: NSString.stringByReplacingOccurrencesOfString:withString:,Binary operation: ([0, +oo] + 1):signed32]