From debeba5860fe2c97b2dd7e0f7f7a8b8b35000f8c Mon Sep 17 00:00:00 2001 From: Qianyi Shu Date: Thu, 27 Aug 2020 01:38:52 -0700 Subject: [PATCH] [cost] make the size of collection more accurate when modifying in loop Summary: Two issues are fixed 1. For this diff, when the condition `curr_langauge_is Java` is removed in some part of the analysis, some c bufferoverrun tests are broken. This is fixed in this diff by inspecting if we are dealing with a `Size` alias referring to an `objc_internal_collection_array`. 2. Previously, when we are modifying mutable array in a loop, e.g. adding element to the array or removing element from the array, we are unable to give an estimable size of the array after exiting the loop. This is now fixed, and the corresponding FPs are resolved. Reviewed By: skcho Differential Revision: D23342350 fbshipit-source-id: 200f5261c --- infer/src/bufferoverrun/absLoc.ml | 12 +++++++++++- infer/src/bufferoverrun/absLoc.mli | 2 ++ infer/src/bufferoverrun/bufferOverrunDomain.ml | 16 +++++++++++++++- .../objc/performance/NSMutableArray.m | 9 +++------ .../objc/performance/cost-issues.exp | 6 +++--- .../codetoanalyze/objc/performance/issues.exp | 10 ++-------- 6 files changed, 36 insertions(+), 19 deletions(-) diff --git a/infer/src/bufferoverrun/absLoc.ml b/infer/src/bufferoverrun/absLoc.ml index 546b1961f..8360916a3 100644 --- a/infer/src/bufferoverrun/absLoc.ml +++ b/infer/src/bufferoverrun/absLoc.ml @@ -231,6 +231,13 @@ module Loc = struct false + let is_objc_collection_internal_array = function + | BoField.Field {fn} -> + Fieldname.equal fn BoField.objc_collection_internal_array + | _ -> + false + + let is_objc_iterator_offset = function | BoField.Field {fn} -> Fieldname.equal fn BoField.objc_iterator_offset @@ -362,7 +369,10 @@ module Loc = struct | BoField.Prim (Allocsite allocsite) -> Allocsite.represents_multiple_values allocsite | BoField.Field _ as x - when is_c_strlen x || is_java_collection_internal_array x || is_objc_iterator_offset x -> + when is_c_strlen x + || is_java_collection_internal_array x + || is_objc_iterator_offset x + || is_objc_collection_internal_array x -> false | BoField.Field {prefix= l} -> represents_multiple_values l diff --git a/infer/src/bufferoverrun/absLoc.mli b/infer/src/bufferoverrun/absLoc.mli index 17a14644d..cddde06dd 100644 --- a/infer/src/bufferoverrun/absLoc.mli +++ b/infer/src/bufferoverrun/absLoc.mli @@ -96,6 +96,8 @@ module Loc : sig val represents_multiple_values : t -> bool + val is_objc_collection_internal_array : t -> bool + val append_field : ?typ:Typ.typ -> t -> Fieldname.t -> t (** It appends field. [typ] is the type of [fn]. *) end diff --git a/infer/src/bufferoverrun/bufferOverrunDomain.ml b/infer/src/bufferoverrun/bufferOverrunDomain.ml index c7d3ac105..fdd0dc3b1 100644 --- a/infer/src/bufferoverrun/bufferOverrunDomain.ml +++ b/infer/src/bufferoverrun/bufferOverrunDomain.ml @@ -1115,6 +1115,12 @@ module AliasTargets = struct () in match iter is_simple_zero x with () -> None | exception Found rhs -> Some rhs + + + let find_size_alias x = + let exception Found of KeyRhs.t in + let is_size rhs = function AliasTarget.Size _ -> raise (Found rhs) | _ -> () in + match iter is_size x with () -> None | exception Found rhs -> Some rhs end module AliasMap = struct @@ -1163,13 +1169,21 @@ module AliasMap = struct fun loc x -> find (KeyLhs.LocKey loc) x |> AliasTargets.map (AliasTarget.set_java_tmp loc) + let has_objc_collection_size_alias : Loc.t -> t -> bool = + fun loc x -> + AliasTargets.find_size_alias (find_loc loc x) + |> Option.value_map ~default:false ~f:Loc.is_objc_collection_internal_array + + let load : Ident.t -> Loc.t -> AliasTarget.t -> t -> t = fun id loc tgt x -> if Loc.is_unknown loc || AliasTarget.is_unknown tgt then x else let tgts = match tgt with - | AliasTarget.Simple {i} when IntLit.iszero i && Language.curr_language_is Java -> + | AliasTarget.Simple {i} + when IntLit.iszero i + && (Language.curr_language_is Java || has_objc_collection_size_alias loc x) -> find_loc loc x |> AliasTargets.add loc tgt | _ -> AliasTargets.singleton loc tgt diff --git a/infer/tests/codetoanalyze/objc/performance/NSMutableArray.m b/infer/tests/codetoanalyze/objc/performance/NSMutableArray.m index 4a49c193b..23db0d07d 100644 --- a/infer/tests/codetoanalyze/objc/performance/NSMutableArray.m +++ b/infer/tests/codetoanalyze/objc/performance/NSMutableArray.m @@ -23,10 +23,7 @@ void nsmarray_empty_ok_costant() { [array insertObject:@1 atIndex:0]; } -// top cost caused by not able to estimate the -// length of array after adding elements to the -// array in a loop -void nsmarray_add_in_loop_constant_FP() { +void nsmarray_add_in_loop_constant() { NSMutableArray* array = [[NSMutableArray alloc] init]; for (int i = 0; i < 10; i++) { [array addObject:[NSNumber numberWithInt:i]]; @@ -35,7 +32,7 @@ void nsmarray_add_in_loop_constant_FP() { } } -void nsmarray_add_in_loop_linear_FP(NSUInteger n) { +void nsmarray_add_in_loop_linear(NSUInteger n) { NSMutableArray* array = [[NSMutableArray alloc] init]; for (int i = 0; i < n; i++) { [array addObject:[NSNumber numberWithInt:i]]; @@ -113,7 +110,7 @@ id nsmarray_remove_constant() { return array[0]; } -void nsmarray_remove_in_loop_constant_FP() { +void nsmarray_remove_in_loop_constant() { NSMutableArray* array = [[NSMutableArray alloc] init]; for (int i = 0; i < 10; i++) { [array addObject:[NSNumber numberWithInt:i]]; diff --git a/infer/tests/codetoanalyze/objc/performance/cost-issues.exp b/infer/tests/codetoanalyze/objc/performance/cost-issues.exp index f63c87653..7c1cc8ad2 100644 --- a/infer/tests/codetoanalyze/objc/performance/cost-issues.exp +++ b/infer/tests/codetoanalyze/objc/performance/cost-issues.exp @@ -39,15 +39,15 @@ codetoanalyze/objc/performance/NSDictionary.m, nsdictionary_init_with_dictionary codetoanalyze/objc/performance/NSInteger.m, nsinteger_value_linear, 3 + 3 ⋅ integer + 2 ⋅ (1+max(0, integer)), OnUIThread:false, [{1+max(0, integer)},Loop,{integer},Loop] codetoanalyze/objc/performance/NSInteger.m, nsnumber_number_with_int_integer_value_constant, 34, OnUIThread:false, [] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_all_constant, 23, OnUIThread:false, [] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_constant_FP, ⊤, OnUIThread:false, [Unbounded loop,Loop] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_linear_FP, ⊤, OnUIThread:false, [Unbounded loop,Loop] +codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_constant, 152, OnUIThread:false, [] +codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_linear, 8 + 7 ⋅ n + 3 ⋅ n + 2 ⋅ (n + 1) + 3 ⋅ (n + 1), OnUIThread:false, [{n + 1},Loop,{n + 1},Loop,{n},Loop,{n},Loop] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_quadratic, 6 + 5 ⋅ n + 8 ⋅ n × m + 2 ⋅ n × (m + 1) + 2 ⋅ (n + 1), OnUIThread:false, [{n + 1},Loop,{m + 1},Loop,{m},Loop,{n},Loop] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_then_loop_constant, 108, OnUIThread:false, [] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_empty_ok_costant, 7, OnUIThread:false, [] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_init_with_capacity_constant_FP, ⊤, OnUIThread:false, [Unbounded loop,Loop] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_all_linear, 4 + 3 ⋅ array->elements.length.ub + array->elements.length.ub + 3 ⋅ (array->elements.length.ub + 1), OnUIThread:false, [{array->elements.length.ub + 1},Loop,{array->elements.length.ub},Modeled call to NSArray.removeAllObjects,{array->elements.length.ub},Loop] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_constant, 17, OnUIThread:false, [] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_in_loop_constant_FP, ⊤, OnUIThread:false, [Unbounded loop,Loop] +codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_in_loop_constant, 182, OnUIThread:false, [] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_set_in_loop_constant, 51, OnUIThread:false, [] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_set_linear, 3 + 11 ⋅ array->elements.length.ub + 3 ⋅ (array->elements.length.ub + 1), OnUIThread:false, [{array->elements.length.ub + 1},Loop,{array->elements.length.ub},Loop] codetoanalyze/objc/performance/NSMutableDictionary.m, nsmutabledictionary_set_element_in_loop_linear_FN, 14, OnUIThread:false, [] diff --git a/infer/tests/codetoanalyze/objc/performance/issues.exp b/infer/tests/codetoanalyze/objc/performance/issues.exp index cddeafeac..26fec12d9 100644 --- a/infer/tests/codetoanalyze/objc/performance/issues.exp +++ b/infer/tests/codetoanalyze/objc/performance/issues.exp @@ -2,16 +2,10 @@ codetoanalyze/objc/performance/NSArray.m, nsarray_empty_array_constant, 3, CONDI codetoanalyze/objc/performance/NSArray.m, nsarray_init_constant, 3, CONDITION_ALWAYS_FALSE, no_bucket, WARNING, [Here] codetoanalyze/objc/performance/NSDictionary.m, nsdictionary_init_with_dictionary_linear_FP, 0, INFINITE_EXECUTION_TIME, no_bucket, ERROR, [Unbounded loop,Loop] codetoanalyze/objc/performance/NSDictionary.m, nsdictionary_init_with_dictionary_linear_FP, 2, INTEGER_OVERFLOW_U5, no_bucket, ERROR, [,Unknown value from: NSDictionary.initWithDictionary:,Binary operation: ([0, +oo] + 1):signed32] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_constant_FP, 0, INFINITE_EXECUTION_TIME, no_bucket, ERROR, [Unbounded loop,Loop] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_constant_FP, 5, INTEGER_OVERFLOW_L5, no_bucket, ERROR, [,Array declaration,Binary operation: ([0, +oo] + 1):signed32] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_linear_FP, 0, INFINITE_EXECUTION_TIME, no_bucket, ERROR, [Unbounded loop,Loop] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_add_in_loop_linear_FP, 5, INTEGER_OVERFLOW_L5, no_bucket, ERROR, [,Array declaration,Binary operation: ([0, +oo] + 1):signed32] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_init_with_capacity_constant_FP, 0, INFINITE_EXECUTION_TIME, no_bucket, ERROR, [Unbounded loop,Loop] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_init_with_capacity_constant_FP, 3, INTEGER_OVERFLOW_U5, no_bucket, ERROR, [,Unknown value from: NSMutableArray.initWithCapacity:,Binary operation: ([0, +oo] + 1):signed32] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_constant, 6, BUFFER_OVERRUN_L3, no_bucket, ERROR, [,Array declaration,Array access: Offset: 0 Size: [0, 2]] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_in_loop_constant_FP, 0, INFINITE_EXECUTION_TIME, no_bucket, ERROR, [Unbounded loop,Loop] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_in_loop_constant_FP, 5, INTEGER_OVERFLOW_L5, no_bucket, ERROR, [,Array declaration,Binary operation: ([0, +oo] + 1):signed32] -codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_in_loop_constant_FP, 6, BUFFER_OVERRUN_L5, no_bucket, ERROR, [,Array declaration,,Array declaration,Array access: Offset: [0, +oo] Size: [0, +oo]] +codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_constant, 6, BUFFER_OVERRUN_L1, no_bucket, ERROR, [,Array declaration,Through,Through,Through,Through,Array access: Offset: 0 Size: 0] +codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_remove_in_loop_constant, 6, BUFFER_OVERRUN_L3, no_bucket, ERROR, [,Set array size,,Assignment,Set array size,Array access: Offset: [0, 9] Size: [1, 10]] codetoanalyze/objc/performance/NSMutableArray.m, nsmarray_set_linear, 2, INTEGER_OVERFLOW_U5, no_bucket, ERROR, [,Unknown value from: NSNumber.intValue,Binary operation: ([-oo, +oo] + 1):signed32] 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]