From ecfa29b083d1c0ed0e1ffa46c154e777a95e2603 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Thu, 10 May 2018 05:11:18 -0700 Subject: [PATCH] [starvation] dedup deadlock warnings Reviewed By: jvillard Differential Revision: D7933937 fbshipit-source-id: cb1e72a --- infer/src/concurrency/starvation.ml | 89 +++++++++++-------- .../codetoanalyze/java/starvation/issues.exp | 6 -- 2 files changed, 54 insertions(+), 41 deletions(-) diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index 9af39f740..42a8bb504 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -12,14 +12,6 @@ module L = Logging let debug fmt = L.(debug Analysis Verbose fmt) -let is_java_static pname = - match pname with - | Typ.Procname.Java java_pname -> - Typ.Procname.Java.is_static java_pname - | _ -> - false - - let is_on_main_thread pn = RacerDConfig.(match Models.get_thread pn with Models.MainThread -> true | _ -> false) @@ -78,14 +70,14 @@ module TransferFunctions (CFG : ProcCfg.S) = struct get_path actuals |> Option.value_map ~default:astate ~f:(Domain.release astate) | LockedIfTrue -> astate - | NoEffect -> - if Models.may_block tenv callee actuals then - let caller = Procdesc.get_proc_name pdesc in - Domain.blocking_call ~caller ~callee loc astate - else if is_on_main_thread callee then Domain.set_on_main_thread astate - else - Summary.read_summary pdesc callee - |> Option.value_map ~default:astate ~f:(Domain.integrate_summary astate callee loc) ) + | NoEffect when Models.may_block tenv callee actuals -> + let caller = Procdesc.get_proc_name pdesc in + Domain.blocking_call ~caller ~callee loc astate + | NoEffect when is_on_main_thread callee -> + Domain.set_on_main_thread astate + | _ -> + Summary.read_summary pdesc callee + |> Option.value_map ~default:astate ~f:(Domain.integrate_summary astate callee loc) ) | _ -> astate @@ -95,13 +87,6 @@ end module Analyzer = LowerHil.MakeAbstractInterpreter (ProcCfg.Normal) (TransferFunctions) -let get_class_of_pname = function - | Typ.Procname.Java java_pname -> - Some (Typ.Procname.Java.get_class_type_name java_pname) - | _ -> - None - - let die_if_not_java proc_desc = let is_java = Procdesc.get_proc_name proc_desc |> Typ.Procname.get_language |> Language.(equal Java) @@ -119,11 +104,13 @@ let analyze_procedure {Callbacks.proc_desc; tenv; summary} = else let loc = Procdesc.get_loc proc_desc in let lock = - if is_java_static pname then - (* this is crafted so as to match synchronized(CLASSNAME.class) constructs *) - get_class_of_pname pname - |> Option.map ~f:(fun tn -> Typ.Name.name tn |> Ident.string_to_name |> lock_of_class) - else FormalMap.get_formal_base 0 formals |> Option.map ~f:(fun base -> (base, [])) + match pname with + | Typ.Procname.Java java_pname when Typ.Procname.Java.is_static java_pname -> + (* this is crafted so as to match synchronized(CLASSNAME.class) constructs *) + Typ.Procname.Java.get_class_type_name java_pname |> Typ.Name.name + |> Ident.string_to_name |> lock_of_class |> Option.some + | _ -> + FormalMap.get_formal_base 0 formals |> Option.map ~f:(fun base -> (base, [])) in Option.value_map lock ~default:StarvationDomain.empty ~f:(StarvationDomain.acquire StarvationDomain.empty loc) @@ -139,11 +126,6 @@ let analyze_procedure {Callbacks.proc_desc; tenv; summary} = Summary.update_summary lock_order summary ) -let get_summary caller_pdesc callee_pdesc = - Summary.read_summary caller_pdesc (Procdesc.get_proc_name callee_pdesc) - |> Option.map ~f:(fun summary -> (callee_pdesc, summary)) - - let make_trace_with_header ?(header= "") elem start_loc pname = let trace = StarvationDomain.LockOrder.make_loc_trace elem in let trace_descr = Format.asprintf "%s %a" header Typ.Procname.pp pname in @@ -161,13 +143,47 @@ let get_summaries_of_methods_in_class get_proc_desc tenv current_pdesc clazz = Option.value_map tstruct_opt ~default:[] ~f:(fun tstruct -> tstruct.Typ.Struct.methods) in let pdescs = List.rev_filter_map methods ~f:get_proc_desc in - List.rev_filter_map pdescs ~f:(get_summary current_pdesc) + let get_summary callee_pdesc = + Summary.read_summary current_pdesc (Procdesc.get_proc_name callee_pdesc) + |> Option.map ~f:(fun summary -> (callee_pdesc, summary)) + in + List.rev_filter_map pdescs ~f:get_summary let log_issue current_pname current_loc ltr exn = Reporting.log_issue_external current_pname Exceptions.Kerror ~loc:current_loc ~ltr exn +let should_report_deadlock_on_current_proc current_elem endpoint_elem = + let open StarvationDomain in + match (current_elem.LockOrder.first, current_elem.LockOrder.eventually) with + | None, _ | Some {LockEvent.event= MayBlock _}, _ | _, {LockEvent.event= MayBlock _} -> + (* should never happen *) + L.die InternalError "Deadlock cannot occur without two lock events: %a" LockOrder.pp + current_elem + | Some {LockEvent.event= LockAcquire ((Var.LogicalVar _, _), [])}, _ -> + (* first event is a class object (see [lock_of_class]), so always report because the + reverse ordering on the events will not occur *) + true + | Some {LockEvent.event= LockAcquire ((Var.LogicalVar _, _), _ :: _)}, _ + | _, {LockEvent.event= LockAcquire ((Var.LogicalVar _, _), _)} -> + (* first event has an ident root, but has a non-empty access path, which means we are + not filtering out local variables (see [exec_instr]), or, + second event has an ident root, which should not happen if we are filtering locals *) + L.die InternalError "Deadlock cannot occur on these logical variables: %a @." LockOrder.pp + current_elem + | ( Some {LockEvent.event= LockAcquire ((_, typ1), _)} + , {LockEvent.event= LockAcquire ((_, typ2), _)} ) -> + (* use string comparison on types as a stable order to decide whether to report a deadlock *) + let c = String.compare (Typ.to_string typ1) (Typ.to_string typ2) in + c < 0 + || Int.equal 0 c + && (* same class, so choose depending on location *) + Location.compare current_elem.LockOrder.eventually.LockEvent.loc + endpoint_elem.LockOrder.eventually.LockEvent.loc + < 0 + + (* Note about how many times we report a deadlock: normally twice, at each trace starting point. Due to the fact we look for deadlocks in the summaries of the class at the root of a path, this will fail when (a) the lock is of class type (ie as used in static sync methods), because @@ -180,7 +196,10 @@ let report_deadlocks get_proc_desc tenv current_pdesc (summary, current_main) = let current_loc = Procdesc.get_loc current_pdesc in let current_pname = Procdesc.get_proc_name current_pdesc in let report_endpoint_elem current_elem endpoint_pname endpoint_loc elem = - if LockOrder.may_deadlock current_elem elem then + if + LockOrder.may_deadlock current_elem elem + && should_report_deadlock_on_current_proc current_elem elem + then let () = debug "Possible deadlock:@.%a@.%a@." LockOrder.pp current_elem LockOrder.pp elem in match (current_elem.LockOrder.eventually, elem.LockOrder.eventually) with | {LockEvent.event= LockAcquire _}, {LockEvent.event= LockAcquire _} -> diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index 5b95ed888..518cd9e6b 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -11,17 +11,11 @@ codetoanalyze/java/starvation/IndirectBlock.java, void IndirectBlock.takeExpensi codetoanalyze/java/starvation/IndirectBlock.java, void IndirectBlock.takeRemoteExpensiveLockOnUiThreadBad(IndirectInterproc), 34, STARVATION, ERROR, [[Trace 1] void IndirectBlock.takeRemoteExpensiveLockOnUiThreadBad(IndirectInterproc),Method call: void IndirectInterproc.takeLock(),locks `this` in class `IndirectInterproc*`,[Trace 2] void IndirectInterproc.doTransactUnderLock(Binder),locks `this` in class `IndirectInterproc*`,calls boolean Binder.transact(int,Parcel,Parcel,int) from void IndirectInterproc.doTransactUnderLock(Binder)] codetoanalyze/java/starvation/InnerClass.java, InnerClass$InnerClassA.(InnerClass,Object), 49, DEADLOCK, ERROR, [[Trace 1] InnerClass$InnerClassA.(InnerClass,Object),locks `this` in class `InnerClass$InnerClassA*`,Method call: void InnerClass.bar(),locks `this` in class `InnerClass*`,[Trace 2] void InnerClass.outerInnerBad(InnerClass$InnerClassA),locks `this` in class `InnerClass*`,Method call: void InnerClass$InnerClassA.baz(),locks `this` in class `InnerClass$InnerClassA*`] codetoanalyze/java/starvation/InnerClass.java, void InnerClass$InnerClassA.innerOuterBad(), 36, DEADLOCK, ERROR, [[Trace 1] void InnerClass$InnerClassA.innerOuterBad(),locks `this` in class `InnerClass$InnerClassA*`,Method call: void InnerClass.bar(),locks `this` in class `InnerClass*`,[Trace 2] void InnerClass.outerInnerBad(InnerClass$InnerClassA),locks `this` in class `InnerClass*`,Method call: void InnerClass$InnerClassA.baz(),locks `this` in class `InnerClass$InnerClassA*`] -codetoanalyze/java/starvation/InnerClass.java, void InnerClass.outerInnerBad(InnerClass$InnerClassA), 19, DEADLOCK, ERROR, [[Trace 1] void InnerClass.outerInnerBad(InnerClass$InnerClassA),locks `this` in class `InnerClass*`,Method call: void InnerClass$InnerClassA.baz(),locks `this` in class `InnerClass$InnerClassA*`,[Trace 2] InnerClass$InnerClassA.(InnerClass,Object),locks `this` in class `InnerClass$InnerClassA*`,Method call: void InnerClass.bar(),locks `this` in class `InnerClass*`] -codetoanalyze/java/starvation/InnerClass.java, void InnerClass.outerInnerBad(InnerClass$InnerClassA), 19, DEADLOCK, ERROR, [[Trace 1] void InnerClass.outerInnerBad(InnerClass$InnerClassA),locks `this` in class `InnerClass*`,Method call: void InnerClass$InnerClassA.baz(),locks `this` in class `InnerClass$InnerClassA*`,[Trace 2] void InnerClass$InnerClassA.innerOuterBad(),locks `this` in class `InnerClass$InnerClassA*`,Method call: void InnerClass.bar(),locks `this` in class `InnerClass*`] codetoanalyze/java/starvation/Interclass.java, void Interclass.interclass1Bad(InterclassA), 12, DEADLOCK, ERROR, [[Trace 1] void Interclass.interclass1Bad(InterclassA),locks `this` in class `Interclass*`,Method call: void InterclassA.interclass1Bad(),locks `this` in class `InterclassA*`,[Trace 2] void InterclassA.interclass2Bad(Interclass),locks `this` in class `InterclassA*`,Method call: void Interclass.interclass2Bad(),locks `this` in class `Interclass*`] -codetoanalyze/java/starvation/Interclass.java, void InterclassA.interclass2Bad(Interclass), 38, DEADLOCK, ERROR, [[Trace 1] void InterclassA.interclass2Bad(Interclass),locks `this` in class `InterclassA*`,Method call: void Interclass.interclass2Bad(),locks `this` in class `Interclass*`,[Trace 2] void Interclass.interclass1Bad(InterclassA),locks `this` in class `Interclass*`,Method call: void InterclassA.interclass1Bad(),locks `this` in class `InterclassA*`] codetoanalyze/java/starvation/Interproc.java, void Interproc.interproc1Bad(InterprocA), 11, DEADLOCK, ERROR, [[Trace 1] void Interproc.interproc1Bad(InterprocA),locks `this` in class `Interproc*`,Method call: void Interproc.interproc2Bad(InterprocA),locks `b` in class `InterprocA*`,[Trace 2] void InterprocA.interproc1Bad(Interproc),locks `this` in class `InterprocA*`,Method call: void InterprocA.interproc2Bad(Interproc),locks `d` in class `Interproc*`] -codetoanalyze/java/starvation/Interproc.java, void InterprocA.interproc1Bad(Interproc), 39, DEADLOCK, ERROR, [[Trace 1] void InterprocA.interproc1Bad(Interproc),locks `this` in class `InterprocA*`,Method call: void InterprocA.interproc2Bad(Interproc),locks `d` in class `Interproc*`,[Trace 2] void Interproc.interproc1Bad(InterprocA),locks `this` in class `Interproc*`,Method call: void Interproc.interproc2Bad(InterprocA),locks `b` in class `InterprocA*`] codetoanalyze/java/starvation/Intraproc.java, void Intraproc.intraBad(IntraprocA), 11, DEADLOCK, ERROR, [[Trace 1] void Intraproc.intraBad(IntraprocA),locks `this` in class `Intraproc*`,locks `o` in class `IntraprocA*`,[Trace 2] void IntraprocA.intraBad(Intraproc),locks `this` in class `IntraprocA*`,locks `o` in class `Intraproc*`] -codetoanalyze/java/starvation/Intraproc.java, void IntraprocA.intraBad(Intraproc), 33, DEADLOCK, ERROR, [[Trace 1] void IntraprocA.intraBad(Intraproc),locks `this` in class `IntraprocA*`,locks `o` in class `Intraproc*`,[Trace 2] void Intraproc.intraBad(IntraprocA),locks `this` in class `Intraproc*`,locks `o` in class `IntraprocA*`] codetoanalyze/java/starvation/JavaIO.java, void JavaIO.fileReadBad(), 32, STARVATION, ERROR, [ void JavaIO.fileReadBad(),Method call: int JavaIO.doFileRead(),calls int InputStreamReader.read() from int JavaIO.doFileRead()] codetoanalyze/java/starvation/JavaIO.java, void JavaIO.streamReadBad(), 37, STARVATION, ERROR, [ void JavaIO.streamReadBad(),Method call: String JavaIO.doStreamRead(),calls String DataInputStream.readUTF() from String JavaIO.doStreamRead()] codetoanalyze/java/starvation/StaticLock.java, void StaticLock.lockOtherClassOneWayBad(), 24, DEADLOCK, ERROR, [[Trace 1] void StaticLock.lockOtherClassOneWayBad(),locks `StaticLock$0` in class `java.lang.Class*`,locks `this` in class `StaticLock*`,[Trace 2] void StaticLock.lockOtherClassAnotherWayNad(),locks `this` in class `StaticLock*`,Method call: void StaticLock.staticSynced(),locks `StaticLock$0` in class `java.lang.Class*`] -codetoanalyze/java/starvation/UIDeadlock.java, void UIDeadlock.notOnUIThreadBad(), 32, DEADLOCK, ERROR, [[Trace 1] void UIDeadlock.notOnUIThreadBad(),locks `this.UIDeadlock.lockB` in class `UIDeadlock*`,locks `this` in class `UIDeadlock*`,[Trace 2] void UIDeadlock.onUIThreadBad(),locks `this` in class `UIDeadlock*`,locks `this.UIDeadlock.lockB` in class `UIDeadlock*`] codetoanalyze/java/starvation/UIDeadlock.java, void UIDeadlock.onUIThreadBad(), 28, DEADLOCK, ERROR, [[Trace 1] void UIDeadlock.onUIThreadBad(),locks `this` in class `UIDeadlock*`,locks `this.UIDeadlock.lockB` in class `UIDeadlock*`,[Trace 2] void UIDeadlock.notOnUIThreadBad(),locks `this.UIDeadlock.lockB` in class `UIDeadlock*`,locks `this` in class `UIDeadlock*`] codetoanalyze/java/starvation/VisDispFrame.java, void VisDispFrame.callsGetVisibleDisplayFrameOnUiThreadBad(), 19, STARVATION, ERROR, [ void VisDispFrame.callsGetVisibleDisplayFrameOnUiThreadBad(),calls void View.getWindowVisibleDisplayFrame(Rect) from void VisDispFrame.callsGetVisibleDisplayFrameOnUiThreadBad()]