From 646c9dbb61f565f83d9f7090b150d01152b2e0f9 Mon Sep 17 00:00:00 2001 From: jrm Date: Mon, 23 Nov 2015 12:23:45 -0800 Subject: [PATCH] Report error message with call stacks for @PerformanceCritical checker Summary: public Use the analysis summary to store call stacks from PerformanceCritical-annotated methods to Expensive-annotated methods. This use the on demand scheduling in order to make sure that the summary of the callee is always analyzed before the callers. Reviewed By: cristianoc Differential Revision: D2685347 fb-gh-sync-id: ab403d9 --- infer/src/backend/callTree.ml | 13 ++ infer/src/backend/callTree.mli | 13 ++ infer/src/backend/specs.ml | 9 +- infer/src/backend/specs.mli | 1 + infer/src/checkers/annotations.ml | 9 -- infer/src/checkers/annotations.mli | 3 - infer/src/checkers/performanceCritical.ml | 140 ++++++++++++------- infer/tests/codetoanalyze/java/checkers/BUCK | 3 +- 8 files changed, 121 insertions(+), 70 deletions(-) create mode 100644 infer/src/backend/callTree.ml create mode 100644 infer/src/backend/callTree.mli diff --git a/infer/src/backend/callTree.ml b/infer/src/backend/callTree.ml new file mode 100644 index 000000000..c1e030ffe --- /dev/null +++ b/infer/src/backend/callTree.ml @@ -0,0 +1,13 @@ +(* + * 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. + *) + + +type t = + | Direct of Procname.t + | Indirect of Procname.t * t list diff --git a/infer/src/backend/callTree.mli b/infer/src/backend/callTree.mli new file mode 100644 index 000000000..c58f9105f --- /dev/null +++ b/infer/src/backend/callTree.mli @@ -0,0 +1,13 @@ +(* + * 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. + *) + +(** data-structure to represent call stacks in a compact tree form *) +type t = + | Direct of Procname.t + | Indirect of Procname.t * t list diff --git a/infer/src/backend/specs.ml b/infer/src/backend/specs.ml index b360179e5..5ded8535e 100644 --- a/infer/src/backend/specs.ml +++ b/infer/src/backend/specs.ml @@ -309,6 +309,7 @@ type is_library = Source | Library type payload = | PrePosts of NormSpec.t list (** list of specs *) | TypeState of unit TypeState.t option (** final typestate *) + | Calls of CallTree.t list type summary = { dependency_map: dependency_map_t; (** maps children procs to timestamp as last seen at the start of an analysys phase for this proc *) @@ -427,7 +428,7 @@ let pp_stats_html err_log fmt stats = let get_specs_from_payload summary = match summary.payload with | PrePosts specs -> NormSpec.tospecs specs - | TypeState _ -> [] + | TypeState _ | Calls _ -> [] (** Print the summary *) let pp_summary pe whole_seconds fmt summary = @@ -481,7 +482,8 @@ let rec post_equal pl1 pl2 = match pl1, pl2 with let payload_compact sh payload = match payload with | PrePosts specs -> PrePosts (IList.map (NormSpec.compact sh) specs) - | TypeState _ -> payload + | TypeState _ as p -> p + | Calls _ as p -> p (** Return a compact representation of the summary *) let summary_compact sh summary = @@ -529,7 +531,8 @@ let summary_serializer : summary Serialization.serializer = let store_summary pname (summ: summary) = let process_payload = function | PrePosts specs -> PrePosts (IList.map NormSpec.erase_join_info_pre specs) - | TypeState typestate_opt -> TypeState typestate_opt in + | TypeState _ as p -> p + | Calls _ as p -> p in let summ1 = { summ with payload = process_payload summ.payload } in let summ2 = if !Config.save_compact_summaries then summary_compact (Sil.create_sharing_env ()) summ1 diff --git a/infer/src/backend/specs.mli b/infer/src/backend/specs.mli index e373a0eec..8d7eb6178 100644 --- a/infer/src/backend/specs.mli +++ b/infer/src/backend/specs.mli @@ -121,6 +121,7 @@ type dependency_map_t = int Procname.Map.t type payload = | PrePosts of NormSpec.t list (** list of specs *) | TypeState of unit TypeState.t option (** final typestate *) + | Calls of CallTree.t list (** list of call tree *) (** Procedure summary *) type summary = diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 43a3e4fa9..0e519cb98 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -110,14 +110,8 @@ let strict = "com.facebook.infer.annotation.Strict" let true_on_null = "TrueOnNull" let verify_annotation = "com.facebook.infer.annotation.Verify" let expensive = "Expensive" -let calls_expensive = "CallsExpensive" let performance_critical = "PerformanceCritical" -let calls_expensive_annotation = { - Sil.class_name = calls_expensive; - Sil.parameters = [] -} - let ia_is_nullable ia = ia_ends_with ia nullable @@ -158,9 +152,6 @@ let ia_is_verify ia = let ia_is_expensive ia = ia_ends_with ia expensive -let ia_calls_expensive ia = - ia_is_expensive ia || ia_ends_with ia calls_expensive - let ia_is_performance_critical ia = ia_ends_with ia performance_critical diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index 75eaab7ae..d450fe7fc 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -14,8 +14,6 @@ val suppressLint : string val expensive : string val performance_critical : string -val calls_expensive_annotation : Sil.annotation - type annotation = | Nullable | Present @@ -71,7 +69,6 @@ val ia_is_present : Sil.item_annotation -> bool 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_calls_expensive : Sil.item_annotation -> bool val ia_is_performance_critical : 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 b777314c2..93c39fc6a 100644 --- a/infer/src/checkers/performanceCritical.ml +++ b/infer/src/checkers/performanceCritical.ml @@ -20,26 +20,6 @@ let unannotated_overrides_performance_critical = "CHECKERS_UNANNOTATED_OVERRIDES_PERFOMANCE_CRITICAL" -let search_expensive_call checked_pnames expensive_callee (pname, _) = - match expensive_callee with - | Some callee_pname -> Some callee_pname - | None -> - if Procname.Set.mem pname !checked_pnames then None - else - begin - checked_pnames := Procname.Set.add pname !checked_pnames; - match Specs.proc_resolve_attributes pname with - | None -> None - | Some attributes -> - let annotated_signature = Annotations.get_annotated_signature attributes in - let ret_annotation, _ = annotated_signature.Annotations.ret in - if Annotations.ia_calls_expensive ret_annotation then - Some pname - else - None - end - - let check_attributes check attributes = let annotated_signature = Annotations.get_annotated_signature attributes in let ret_annotation, _ = annotated_signature.Annotations.ret in @@ -69,39 +49,77 @@ let method_is_expensive pname = check_method Annotations.ia_is_expensive pname -let update_summary_attributes pname = +let lookup_call_trees pname = + match Specs.get_summary pname with + | None -> [] + | Some summary -> + begin + match summary.Specs.payload with + | Specs.Calls tree -> tree + | _ -> [] + end + + +let collect_expensive_call caller_pdesc checked_pnames call_trees (pname, _) = + if Procname.Set.mem pname !checked_pnames then call_trees + else + begin + Ondemand.do_analysis caller_pdesc pname; + checked_pnames := Procname.Set.add pname !checked_pnames; + if method_is_expensive pname then + (CallTree.Direct pname) :: call_trees + else + match lookup_call_trees pname with + | [] -> call_trees + | calls -> (CallTree.Indirect (pname, calls)) :: call_trees + end + + +let update_summary call_trees pname = match Specs.get_summary pname with | None -> () | Some summary -> - let attributes = Specs.get_attributes summary in - let ret_annot, param_annot = attributes.ProcAttributes.method_annotation in - let updated_method_annot = - (Annotations.calls_expensive_annotation, true) :: ret_annot, param_annot in - let updated_attributes = - { attributes with ProcAttributes.method_annotation = updated_method_annot } in let updated_summary = - { summary with Specs.attributes = updated_attributes } in + { summary with Specs.payload = Specs.Calls call_trees } in Specs.add_summary pname updated_summary -let callback_performance_checker _ _ _ tenv pname pdesc = +let report_expensive_calls pname pdesc loc call_trees = + let string_of_pname = Procname.to_simplified_string ~withclass:true in + let rec report_call_tree stack_str call_tree = + match call_tree with + | CallTree.Direct expensive_pname -> + 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 + Checkers.ST.report_error + pname pdesc calls_expensive_method loc description + | CallTree.Indirect (callee_pname, sub_trees) -> + let callee_pname_str = string_of_pname callee_pname in + let new_stack_str = stack_str ^ callee_pname_str ^ " -> " in + IList.iter (report_call_tree new_stack_str) sub_trees in + IList.iter (report_call_tree "") call_trees + + +let check_one_procedure tenv pname pdesc = let loc = Cfg.Procdesc.get_loc pdesc in let attributes = Cfg.Procdesc.get_attributes pdesc in let expensive = is_expensive attributes and performance_critical = is_performance_critical attributes in - let expensive_call_found = - let checked_pnames = ref Procname.Set.empty in - Cfg.Procdesc.fold_calls - (search_expensive_call checked_pnames) - None - pdesc in let check_expensive_subtyping_rules overridden_pname = if not (method_is_expensive overridden_pname) then let description = Printf.sprintf - "Method %s overrides unannotated method %s and cannot be annotated with @%s" + "Method `%s` overrides unannotated method `%s` and cannot be annotated with `@%s`" (Procname.to_string pname) (Procname.to_string overridden_pname) Annotations.expensive in @@ -112,7 +130,7 @@ let callback_performance_checker _ _ _ tenv pname pdesc = if method_is_performance_critical overridden_pname then let description = Printf.sprintf - "Method %s overrides method %s annotated with %s and should also be annotated" + "Method `%s` overrides method `%s` annotated with `%s` and should also be annotated" (Procname.to_string pname) (Procname.to_string overridden_pname) Annotations.performance_critical in @@ -120,22 +138,36 @@ let callback_performance_checker _ _ _ tenv pname pdesc = pname pdesc unannotated_overrides_performance_critical loc description in if expensive then - PatternMatch.proc_iter_overridden_methods check_expensive_subtyping_rules tenv pname; + PatternMatch.proc_iter_overridden_methods + check_expensive_subtyping_rules tenv pname; if not performance_critical then - PatternMatch.proc_iter_overridden_methods check_performance_critical_subtyping_rules tenv pname; - - match expensive_call_found with - | None -> () - | Some callee_pname when performance_critical -> - let description = - Printf.sprintf "Method %s annotated with @%s calls method %s annotated with @%s" - (Procname.to_simplified_string pname) - Annotations.performance_critical - (Procname.to_string callee_pname) - Annotations.expensive in - Checkers.ST.report_error - pname pdesc calls_expensive_method loc description - | Some _ when not expensive -> - update_summary_attributes pname + PatternMatch.proc_iter_overridden_methods + check_performance_critical_subtyping_rules tenv pname; - | Some _ -> () (* Nothing to do if method already annotated with @Expensive *) + let expensive_call_trees = + let checked_pnames = ref Procname.Set.empty in + Cfg.Procdesc.fold_calls + (collect_expensive_call pdesc checked_pnames) [] pdesc in + match expensive_call_trees with + | [] -> () + | call_trees when performance_critical -> + report_expensive_calls pname pdesc loc call_trees + | call_trees -> + update_summary call_trees pname + + +let callback_performance_checker _ get_proc_desc _ tenv pname proc_desc = + let callbacks = + let analyze_ondemand pn = + match get_proc_desc pn with + | None -> () + | Some pd -> check_one_procedure tenv pn pd in + { Ondemand.analyze_ondemand; get_proc_desc; } in + if !Config.ondemand_enabled + || Ondemand.procedure_should_be_analyzed proc_desc pname + then + begin + Ondemand.set_callbacks callbacks; + check_one_procedure tenv pname proc_desc; + Ondemand.unset_callbacks () + end diff --git a/infer/tests/codetoanalyze/java/checkers/BUCK b/infer/tests/codetoanalyze/java/checkers/BUCK index ffe3077e5..bbc4de4c2 100644 --- a/infer/tests/codetoanalyze/java/checkers/BUCK +++ b/infer/tests/codetoanalyze/java/checkers/BUCK @@ -18,6 +18,7 @@ out = 'out' clean_cmd = ' '.join(['rm', '-rf', out]) classpath = ':'.join([('$(classpath ' + path + ')') for path in dependencies]) env_cmd = ' '.join(['export', 'INFER_REPORT_EXPENSIVE_CALLS=1']) +ondemand_mode = ' '.join(['export', 'INFER_ONDEMAND=1']) infer_cmd = ' '.join([ 'infer', '--no-progress-bar', @@ -30,7 +31,7 @@ infer_cmd = ' '.join([ '$SRCS', ]) copy_cmd = ' '.join(['cp', out + '/report.csv', '$OUT']) -command = ' && '.join([clean_cmd, env_cmd, infer_cmd, copy_cmd]) +command = ' && '.join([clean_cmd, ondemand_mode, env_cmd, infer_cmd, copy_cmd]) genrule( name = 'analyze',