[starvation] dedup deadlocks

Summary: Deadlocks can be very noisy, so dedup reports on same line by showing only the one with the shortest trace and a count of the suppressed ones.

Reviewed By: mbouaziz

Differential Revision: D8351148

fbshipit-source-id: 8913db2

infer/src/concurrency/starvation.ml

@@ -168,44 +168,55 @@ let get_summaries_of_methods_in_class tenv clazz =
 
 
 (** per-procedure report map, which takes care of deduplication *)
-module ReportMap = struct
-  type issue_t = Starvation of StarvationDomain.Event.severity_t | Deadlock [@@deriving compare]
+module ReportMap : sig
+  type t
 
-  type report_t =
-    { issue: issue_t
-    ; pname: Typ.Procname.t [@compare.ignore]
-    ; ltr: Errlog.loc_trace [@compare.ignore]
-    ; message: string [@compare.ignore] }
-  [@@deriving compare]
+  val empty : t
 
-  module LocMap = PrettyPrintable.MakePPMap (Location)
+  val add_deadlock : Tenv.t -> Procdesc.t -> Location.t -> Errlog.loc_trace -> string -> t -> t
+
+  val add_starvation :
+    Tenv.t -> StarvationDomain.Event.severity_t -> Procdesc.t -> Location.t -> Errlog.loc_trace
+    -> string -> t -> t
+
+  val log : t -> unit
+end = struct
+  type starvation_t = StarvationDomain.Event.severity_t
 
-  let empty : report_t list LocMap.t = LocMap.empty
+  type deadlock_t = int
+
+  type 'weight_t report_t =
+    {weight: 'weight_t; pname: Typ.Procname.t; ltr: Errlog.loc_trace; message: string}
+
+  module LocMap = PrettyPrintable.MakePPMap (Location)
 
-  let issue_type issue =
-    match issue with Deadlock -> IssueType.deadlock | Starvation _ -> IssueType.starvation
+  type t = (deadlock_t report_t list * starvation_t report_t list) LocMap.t
 
+  let empty : t = LocMap.empty
 
-  let add tenv issue pdesc loc ltr message map =
+  let add_deadlock tenv pdesc loc ltr message (map: t) =
     let pname = Procdesc.get_proc_name pdesc in
-    let issue_type = issue_type issue in
-    if Reporting.is_suppressed tenv pdesc issue_type ~field_name:None then map
+    if Reporting.is_suppressed tenv pdesc IssueType.deadlock ~field_name:None then map
     else
-      let rep = {issue; pname; ltr; message} in
-      let preexisting = try LocMap.find loc map with Caml.Not_found -> [] in
-      LocMap.add loc (rep :: preexisting) map
+      let rep = {weight= -List.length ltr; pname; ltr; message} in
+      let deadlocks, starvations = try LocMap.find loc map with Caml.Not_found -> ([], []) in
+      let new_reports = (rep :: deadlocks, starvations) in
+      LocMap.add loc new_reports map
 
 
-  let add_deadlock tenv pdesc loc ltr exn map = add tenv Deadlock pdesc loc ltr exn map
-
-  let add_starvation tenv sev pdesc loc ltr exn map =
-    add tenv (Starvation sev) pdesc loc ltr exn map
+  let add_starvation tenv sev pdesc loc ltr message map =
+    let pname = Procdesc.get_proc_name pdesc in
+    if Reporting.is_suppressed tenv pdesc IssueType.starvation ~field_name:None then map
+    else
+      let rep = {weight= sev; pname; ltr; message} in
+      let deadlocks, starvations = try LocMap.find loc map with Caml.Not_found -> ([], []) in
+      let new_reports = (deadlocks, rep :: starvations) in
+      LocMap.add loc new_reports map
 
 
   let log map =
-    let log_report loc {issue; pname; ltr; message} =
-      let issue_type = issue_type issue in
-      let exn = Exceptions.Checkers (issue_type, Localise.verbatim_desc message) in
+    let log_report issuetype loc {pname; ltr; message} =
+      let exn = Exceptions.Checkers (issuetype, Localise.verbatim_desc message) in
       Reporting.log_issue_external pname Exceptions.Kerror ~loc ~ltr exn
     in
     let mk_deduped_report num_of_reports ({message} as report) =
@@ -214,21 +225,20 @@ module ReportMap = struct
           Printf.sprintf "%s %d more starvation report(s) on the same line suppressed." message
             (num_of_reports - 1) }
     in
-    let log_location loc reports =
-      let deadlocks, starvations =
-        List.partition_tf ~f:(function {issue= Deadlock} -> true | _ -> false) reports
-      in
-      (* report all deadlocks *)
-      List.iter ~f:(log_report loc) deadlocks ;
-      match starvations with
+    let log_loc_reports issuetype compare loc = function
       | [] ->
           ()
       | [report] ->
-          log_report loc report
-      | _ ->
-          List.max_elt ~compare:compare_report_t starvations
+          log_report issuetype loc report
+      | reports ->
+          List.max_elt ~compare:(fun {weight} {weight= weight'} -> compare weight weight') reports
           |> Option.iter ~f:(fun rep ->
-                 mk_deduped_report (List.length starvations) rep |> log_report loc )
+                 mk_deduped_report (List.length reports) rep |> log_report issuetype loc )
+    in
+    let log_location loc (deadlocks, starvations) =
+      log_loc_reports IssueType.deadlock Int.compare loc deadlocks ;
+      log_loc_reports IssueType.starvation StarvationDomain.Event.compare_severity_t loc
+        starvations
     in
     LocMap.iter log_location map
 end

infer/tests/codetoanalyze/java/starvation/Dedup.java

@@ -28,4 +28,24 @@ class Dedup {
     latch.await();
     future.get();
   }
+
+  Object lockA, lockB;
+
+  // deadlock should be reported between oneWayBad and anotherWayBad only
+  void oneWayBad() {
+    synchronized(lockA) {
+      synchronized(lockB) {}
+    }
+  }
+
+  void anotherWayBad() {
+    synchronized(lockB) {
+      synchronized(lockA) {}
+    }
+  }
+
+  // this is creating a longer trace than anotherWayBad, so should be suppressed
+  void thirdLongerWayBad() {
+    anotherWayBad();
+  }
 }

infer/tests/codetoanalyze/java/starvation/issues.exp

@@ -11,6 +11,7 @@ codetoanalyze/java/starvation/Dedup.java, void Dedup.callMethodWithMultipleBlock
 codetoanalyze/java/starvation/Dedup.java, void Dedup.callMethodWithMultipleBlocksBad(), 28, STARVATION, no_bucket, ERROR, [`void Dedup.callMethodWithMultipleBlocksBad()`,calls `void CountDownLatch.await()` from `void Dedup.callMethodWithMultipleBlocksBad()`]
 codetoanalyze/java/starvation/Dedup.java, void Dedup.callMethodWithMultipleBlocksBad(), 29, STARVATION, no_bucket, ERROR, [`void Dedup.callMethodWithMultipleBlocksBad()`,calls `Object Future.get()` from `void Dedup.callMethodWithMultipleBlocksBad()`]
 codetoanalyze/java/starvation/Dedup.java, void Dedup.onUiThreadBad(), 21, STARVATION, no_bucket, ERROR, [`void Dedup.onUiThreadBad()`,Method call: `void Dedup.callMethodWithMultipleBlocksBad()`,calls `Object Future.get()` from `void Dedup.callMethodWithMultipleBlocksBad()`]
+codetoanalyze/java/starvation/Dedup.java, void Dedup.oneWayBad(), 36, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void Dedup.oneWayBad()`,locks `this.Dedup.lockA` in class `Dedup*`,locks `this.Dedup.lockB` in class `Dedup*`,[Trace 2] `void Dedup.anotherWayBad()`,locks `this.Dedup.lockB` in class `Dedup*`,locks `this.Dedup.lockA` in class `Dedup*`]
 codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getDirectBad(), 20, STARVATION, no_bucket, ERROR, [`void FutureGet.getDirectBad()`,calls `Object Future.get()` from `void FutureGet.getDirectBad()`]
 codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getIndirectBad(), 25, STARVATION, no_bucket, ERROR, [[Trace 1] `void FutureGet.getIndirectBad()`,locks `this.FutureGet.lock` in class `FutureGet*`,[Trace 2] `void FutureGet.getUnderLock()`,locks `this.FutureGet.lock` in class `FutureGet*`,calls `Object Future.get()` from `void FutureGet.getUnderLock()`]
 codetoanalyze/java/starvation/FutureGet.java, void FutureGet.getTimeout50000001MicroSecondsBad(), 76, STARVATION, no_bucket, ERROR, [`void FutureGet.getTimeout50000001MicroSecondsBad()`,calls `Object Future.get(long,TimeUnit)` from `void FutureGet.getTimeout50000001MicroSecondsBad()`]