From c6d8cdc8eecc8ea91beb83713338087ea218c0a4 Mon Sep 17 00:00:00 2001 From: jrm Date: Wed, 17 Feb 2016 17:56:27 -0800 Subject: [PATCH] Add support for @NoAllocation in the performance critical checker Summary:public Add to the code to detect violation of the `NoAllocation` annotation. This diff adds the code to detect such issue based on the code of the `PerformanceCritical` checker. In the next diff, I will refine the list of acceptable allocations, like new exceptions, etc, and add the list of corresponding tests. Reviewed By: sblackshear Differential Revision: D2938641 fb-gh-sync-id: 9a047dd shipit-source-id: 9a047dd --- .../infer/annotation/NoAllocation.java | 19 ++ infer/src/backend/specs.ml | 7 +- infer/src/backend/specs.mli | 8 +- infer/src/checkers/annotations.ml | 4 + infer/src/checkers/annotations.mli | 2 + infer/src/checkers/performanceCritical.ml | 207 +++++++++++++----- .../java/checkers/NoAllocationExample.java | 43 ++++ .../java/checkers/NoAllocationTest.java | 52 +++++ 8 files changed, 285 insertions(+), 57 deletions(-) create mode 100644 infer/annotations/com/facebook/infer/annotation/NoAllocation.java create mode 100644 infer/tests/codetoanalyze/java/checkers/NoAllocationExample.java create mode 100644 infer/tests/endtoend/java/checkers/NoAllocationTest.java diff --git a/infer/annotations/com/facebook/infer/annotation/NoAllocation.java b/infer/annotations/com/facebook/infer/annotation/NoAllocation.java new file mode 100644 index 000000000..694dd3837 --- /dev/null +++ b/infer/annotations/com/facebook/infer/annotation/NoAllocation.java @@ -0,0 +1,19 @@ +/* + * Copyright (c) 2016 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package com.facebook.infer.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +@Retention(RetentionPolicy.CLASS) +@Target(ElementType.METHOD) +public @interface NoAllocation {} diff --git a/infer/src/backend/specs.ml b/infer/src/backend/specs.ml index eabdc07dc..b90c0060f 100644 --- a/infer/src/backend/specs.ml +++ b/infer/src/backend/specs.ml @@ -301,12 +301,17 @@ type dependency_map_t = int Procname.Map.t type call = Procname.t * Location.t +type call_summary = { + expensive_calls: call list; + allocations: call list +} + (** Payload: results of some analysis *) type payload = { preposts : NormSpec.t list option; (** list of specs *) typestate : unit TypeState.t option; (** final typestate *) - calls: call list option; + calls: call_summary option; } type summary = diff --git a/infer/src/backend/specs.mli b/infer/src/backend/specs.mli index 1b0db6080..d50a62313 100644 --- a/infer/src/backend/specs.mli +++ b/infer/src/backend/specs.mli @@ -117,12 +117,18 @@ type dependency_map_t = int Procname.Map.t (** Type for calls consiting in the name of the callee and the location of the call *) type call = Procname.t * Location.t +(** Collection of first step calls toward expensive call stacks or call stack allocating memory *) +type call_summary = { + expensive_calls: call list; + allocations: call list +} + (** Payload: results of some analysis *) type payload = { preposts : NormSpec.t list option; (** list of specs *) typestate : unit TypeState.t option; (** final typestate *) - calls: call list option; (** list of call tree *) + calls: call_summary option; (** list of calls of the form (call, loc) *) } (** Procedure summary *) diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 95743497d..d1d811c76 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -112,6 +112,7 @@ let true_on_null = "TrueOnNull" let verify_annotation = "com.facebook.infer.annotation.Verify" let expensive = "Expensive" let performance_critical = "PerformanceCritical" +let no_allocation = "NoAllocation" let ia_is_nullable ia = ia_ends_with ia nullable @@ -156,6 +157,9 @@ let ia_is_expensive ia = let ia_is_performance_critical ia = ia_ends_with ia performance_critical +let ia_is_no_allocation ia = + ia_ends_with ia no_allocation + type annotation = | Nullable | Present diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index aba7c6693..538135d58 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -13,6 +13,7 @@ val suppressLint : string val expensive : string val performance_critical : string +val no_allocation : string type annotation = | Nullable @@ -72,6 +73,7 @@ val ia_is_true_on_null : Sil.item_annotation -> bool val ia_is_verify : Sil.item_annotation -> bool val ia_is_expensive : Sil.item_annotation -> bool val ia_is_performance_critical : Sil.item_annotation -> bool +val ia_is_no_allocation : Sil.item_annotation -> bool val ia_iter : (Sil.annotation -> unit) -> Sil.item_annotation -> unit diff --git a/infer/src/checkers/performanceCritical.ml b/infer/src/checkers/performanceCritical.ml index 659f84b70..2feb29d46 100644 --- a/infer/src/checkers/performanceCritical.ml +++ b/infer/src/checkers/performanceCritical.ml @@ -9,11 +9,20 @@ module L = Logging + +let performance_critical_implies_no_allocation = true + + (* Warning name when a performance critical method directly or indirectly calls a method annotatd as expensive *) let calls_expensive_method = "CHECKERS_CALLS_EXPENSIVE_METHOD" +(* Warning name when a performance critical method directly or indirectly + calls a method allocating memory *) +let allocates_memory = + "CHECKERS_ALLOCATES_MEMORY" + (* Warning name for the subtyping rule: method not annotated as expensive cannot be overridden by a method annotated as expensive *) let expensive_overrides_unexpensive = @@ -45,17 +54,31 @@ let method_is_performance_critical pname = check_method Annotations.ia_is_performance_critical pname -let method_overrides_performance_critical tenv pname = +let method_is_no_allcation pname = + (performance_critical_implies_no_allocation + && method_is_performance_critical pname) + || check_method Annotations.ia_is_no_allocation pname + + +let method_overrides is_annotated tenv pname = let overrides () = let found = ref false in PatternMatch.proc_iter_overridden_methods - (fun pn -> found := method_is_performance_critical pn) + (fun pn -> found := is_annotated pn) tenv pname; !found in - method_is_performance_critical pname + is_annotated pname || overrides () +let method_overrides_performance_critical tenv pname = + method_overrides method_is_performance_critical tenv pname + + +let method_overrides_no_allocation tenv pname = + method_overrides method_is_no_allcation tenv pname + + let is_modeled_expensive tenv pname = if SymExec.function_is_builtin pname then false else if Procname.java_get_method pname <> "findViewById" then false @@ -78,20 +101,42 @@ let method_is_expensive tenv pname = || check_method Annotations.ia_is_expensive pname -let lookup_expensive_calls pname = +let lookup_call_summary pname = match Specs.get_summary pname with + | None -> None + | Some summary -> summary.Specs.payload.Specs.calls + + +let lookup_expensive_calls pname = + match lookup_call_summary pname with | None -> [] - | Some summary -> - begin - match summary.Specs.payload.Specs.calls with - | Some calls -> calls - | None -> [] - end + | Some { Specs.expensive_calls } -> expensive_calls + + +let lookup_allocations pname = + match lookup_call_summary pname with + | None -> [] + | Some { Specs.allocations } -> allocations let method_calls_expensive tenv pname = + let calls_expensive () = + match lookup_call_summary pname with + | Some { Specs.expensive_calls } -> + expensive_calls <> [] + | None -> false in method_is_expensive tenv pname - || lookup_expensive_calls pname <> [] + || calls_expensive () + + +let method_allocates pname = + let allocates () = + match lookup_call_summary pname with + | Some { Specs.allocations } -> + allocations <> [] + | None -> false in + Procname.is_constructor pname + || allocates () let lookup_location pname = @@ -100,20 +145,29 @@ let lookup_location pname = | Some summary -> summary.Specs.attributes.ProcAttributes.loc -let collect_expensive_call tenv caller_pdesc checked_pnames call_list (pname, _) = - if Procname.Set.mem pname !checked_pnames then call_list +let collect_calls tenv caller_pdesc checked_pnames call_summary (pname, _) = + if Procname.Set.mem pname !checked_pnames then call_summary else begin Ondemand.do_analysis caller_pdesc pname; checked_pnames := Procname.Set.add pname !checked_pnames; let call_loc = lookup_location pname in - if method_calls_expensive tenv pname then - (pname, call_loc) :: call_list - else - call_list + let updated_expensive_calls = + if method_calls_expensive tenv pname then + (pname, call_loc) :: call_summary.Specs.expensive_calls + else + call_summary.Specs.expensive_calls in + let updated_allocations = + if method_allocates pname then + (pname, call_loc) :: call_summary.Specs.allocations + else + call_summary.Specs.allocations in + { Specs.expensive_calls = updated_expensive_calls; + Specs.allocations = updated_allocations } end -let update_summary calls pname = + +let update_summary call_summary pname = match Specs.get_summary pname with | None -> () | Some summary -> @@ -121,41 +175,65 @@ let update_summary calls pname = { summary with Specs.payload = { summary.Specs.payload with - Specs.calls = Some calls; } + Specs.calls = Some call_summary } } in Specs.add_summary pname updated_summary -let report_expensive_calls tenv pname pdesc loc calls = - let string_of_pname = Procname.to_simplified_string ~withclass:true in - let update_trace trace loc = - if Location.equal loc Location.dummy then trace - else - let trace_elem = { - Errlog.lt_level = 0; - lt_loc = loc; - lt_description = ""; - lt_node_tags = []; - } in - trace_elem :: trace in - let rec report_expensive_call visited_pnames (trace, stack_str) (callee_pname, callee_loc) = - if method_is_expensive tenv callee_pname then - let final_trace = IList.rev (update_trace trace callee_loc) in - let exp_pname_str = string_of_pname callee_pname in - let description = - Printf.sprintf - "Method `%s` annotated with `@%s` calls `%s%s` where `%s` is annotated with `@%s`" - (Procname.to_simplified_string pname) - Annotations.performance_critical - stack_str - exp_pname_str - exp_pname_str - Annotations.expensive in - let exn = - Exceptions.Checkers (calls_expensive_method, Localise.verbatim_desc description) in - Reporting.log_error pname ~loc: (Some loc) ~ltr: (Some final_trace) exn +let string_of_pname = + Procname.to_simplified_string ~withclass:true + +let update_trace trace loc = + if Location.equal loc Location.dummy then trace + else + let trace_elem = { + Errlog.lt_level = 0; + lt_loc = loc; + lt_description = ""; + lt_node_tags = []; + } in + trace_elem :: trace + + +let report_expensive_call_stack pname loc trace stack_str expensive_pname call_loc = + let final_trace = IList.rev (update_trace trace call_loc) in + let exp_pname_str = string_of_pname expensive_pname in + let description = + Printf.sprintf + "Method `%s` annotated with `@%s` calls `%s%s` where `%s` is annotated with `@%s`" + (Procname.to_simplified_string pname) + Annotations.performance_critical + stack_str + exp_pname_str + exp_pname_str + Annotations.expensive in + let exn = + Exceptions.Checkers (calls_expensive_method, Localise.verbatim_desc description) in + Reporting.log_error pname ~loc: (Some loc) ~ltr: (Some final_trace) exn + + +let report_allocation_stack pname loc trace stack_str constructor_pname call_loc = + let final_trace = IList.rev (update_trace trace call_loc) in + let constr_str = string_of_pname constructor_pname in + let description = + Printf.sprintf + "Method `%s` annotated with `@%s` allocates `%s` via `%s%s`" + (Procname.to_simplified_string pname) + Annotations.no_allocation + constr_str + stack_str + ("new "^constr_str) in + let exn = + Exceptions.Checkers (allocates_memory, Localise.verbatim_desc description) in + Reporting.log_error pname ~loc: (Some loc) ~ltr: (Some final_trace) exn + + +let report_call_stack end_of_stack lookup_next_calls report tenv pname pdesc loc calls = + let rec loop visited_pnames (trace, stack_str) (callee_pname, callee_loc) = + if end_of_stack tenv callee_pname then + report pname loc trace stack_str callee_pname callee_loc else - let next_calls = lookup_expensive_calls callee_pname in + let next_calls = lookup_next_calls callee_pname in let callee_pname_str = string_of_pname callee_pname in let new_stack_str = stack_str ^ callee_pname_str ^ " -> " in let new_trace = update_trace trace callee_loc in @@ -165,9 +243,21 @@ let report_expensive_calls tenv pname pdesc loc calls = if Procname.Set.mem p set then (accu, set) else ((p, loc) :: accu, Procname.Set.add p set)) ([], visited_pnames) next_calls in - IList.iter (report_expensive_call updated_visited (new_trace, new_stack_str)) unseen_pnames in + IList.iter (loop updated_visited (new_trace, new_stack_str)) unseen_pnames in let start_trace = update_trace [] (Cfg.Procdesc.get_loc pdesc) in - IList.iter (report_expensive_call Procname.Set.empty (start_trace, "")) calls + IList.iter (loop Procname.Set.empty (start_trace, "")) calls + + +let report_expensive_calls tenv pname pdesc loc calls = + report_call_stack + method_is_expensive lookup_expensive_calls report_expensive_call_stack + tenv pname pdesc loc calls + + +let report_allocations tenv pname pdesc loc calls = + report_call_stack + (fun _ p -> Procname.is_constructor p) lookup_allocations report_allocation_stack + tenv pname pdesc loc calls let check_one_procedure tenv pname pdesc = @@ -176,7 +266,9 @@ let check_one_procedure tenv pname pdesc = let attributes = Cfg.Procdesc.get_attributes pdesc in let expensive = is_expensive attributes and performance_critical = - method_overrides_performance_critical tenv pname in + method_overrides_performance_critical tenv pname + and no_allocation = + method_overrides_no_allocation tenv pname in let check_expensive_subtyping_rules overridden_pname = if not (method_is_expensive tenv overridden_pname) then @@ -195,15 +287,20 @@ let check_one_procedure tenv pname pdesc = PatternMatch.proc_iter_overridden_methods check_expensive_subtyping_rules tenv pname; - let expensive_calls = + let call_summary = let checked_pnames = ref Procname.Set.empty in + let empty_summary = + { Specs.expensive_calls = []; + allocations = [] } in Cfg.Procdesc.fold_calls - (collect_expensive_call tenv pdesc checked_pnames) [] pdesc in + (collect_calls tenv pdesc checked_pnames) empty_summary pdesc in - update_summary expensive_calls pname; + update_summary call_summary pname; if performance_critical then - report_expensive_calls tenv pname pdesc loc expensive_calls + report_expensive_calls tenv pname pdesc loc call_summary.Specs.expensive_calls; + if no_allocation then + report_allocations tenv pname pdesc loc call_summary.Specs.allocations let callback_performance_checker { Callbacks.proc_desc; proc_name; get_proc_desc; tenv } = diff --git a/infer/tests/codetoanalyze/java/checkers/NoAllocationExample.java b/infer/tests/codetoanalyze/java/checkers/NoAllocationExample.java new file mode 100644 index 000000000..d8b131184 --- /dev/null +++ b/infer/tests/codetoanalyze/java/checkers/NoAllocationExample.java @@ -0,0 +1,43 @@ +/* + * Copyright (c) 2013 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package codetoanalyze.java.checkers; + +import com.facebook.infer.annotation.NoAllocation; + +public class NoAllocationExample { + + @NoAllocation + void directlyAllocatingMethod() { + new Object(); + } + + void allocates() { + new Object(); + } + + @NoAllocation + void indirectlyAllocatingMethod() { + allocates(); + } + + void doesNotAllocate() { + // does noting + } + + @NoAllocation + void notAllocatingMethod() { + doesNotAllocate(); + } + + void allocatingIsFine() { + new Object(); + } + +} diff --git a/infer/tests/endtoend/java/checkers/NoAllocationTest.java b/infer/tests/endtoend/java/checkers/NoAllocationTest.java new file mode 100644 index 000000000..cdbf3234b --- /dev/null +++ b/infer/tests/endtoend/java/checkers/NoAllocationTest.java @@ -0,0 +1,52 @@ +/* + * Copyright (c) 2015 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + + +package endtoend.java.checkers; + +import org.junit.BeforeClass; +import org.junit.Test; +import utils.InferException; +import utils.InferResults; + +import java.io.IOException; + +import static org.hamcrest.MatcherAssert.assertThat; +import static utils.matchers.ResultContainsExactly.containsExactly; + +public class NoAllocationTest { + + public static final String SOURCE_FILE = + "infer/tests/codetoanalyze/java/checkers/NoAllocationExample.java"; + + public static final String ALLOCATES_MEMORY = + "CHECKERS_ALLOCATES_MEMORY"; + + private static InferResults inferResults; + + @BeforeClass + public static void loadResults() throws InterruptedException, IOException { + inferResults = InferResults.loadCheckersResults(NoAllocationTest.class, SOURCE_FILE); + } + + @Test + public void matchErrors() + throws IOException, InterruptedException, InferException { + String[] methods = { + "directlyAllocatingMethod", + "indirectlyAllocatingMethod", + }; + assertThat("Results should contain " + ALLOCATES_MEMORY, + inferResults, + containsExactly(ALLOCATES_MEMORY, + SOURCE_FILE, + methods)); + } + +}