tests: thread-safe test suites (#353)

This makes steps towards making test suites thread-safe. That is, multiple suites can be executed in parallel, but test cases within the same suite are still run sequentially.

* add locks around systemtests

* DataStructureAnalysis: avoid global NodeCounter

this replaces the global singleton NodeCounter with
a new instance per DSA execution. this allows multiple
DSA runs to run in parallel.

the NodeCounter is constructed by the DataStructureAnalysis class,
then passed through an implicit "using" parameter to the classes
which require it.

* remove reset method from NodeCounter

* refactor to LockManager class

* lock around testResults

* scalafmt

* greatly simplify Lockmanager by not using RWLock

LockManager now uses a single lock around the map and this is shared
for reads and writes. this will increase lock contention. however,
the locked duration will be extremely small since it only wraps
a single hashmap operation. this should have no discernible performance
impact.

* Apply suggestions from code review

* SystemTests: shuffle tests

honestly, i think this might be annoying, especially if you try
to run the tests locally to reproduce an issue and the order keeps
changing.

* SystemTests: insert suite name into output file names

this should avoid clashes where test suites try to write
to the same output file.

SystemTestsBAP and SystemTestsGTIRB are overriden to have
no suffix, to maintain compatibility with the update-expected
sbt/mill jobs.

* Revert "SystemTests: shuffle tests"

This reverts commit f4b3a53.

* ci: run 3 suites in parallel

-T is used to re-sort the test cases into the original order
before printing them in batches for each suite to avoid interleaving
individual cases from multiple suites running in parallel.

* RegionTimer: use atomic long instead of instance vars

* ExtraSpecTests: retry on failure

Author: katrinafyi

.github/workflows/run-examples.yml

@@ -65,7 +65,7 @@ jobs:
       - run: dotnet tool install --global boogie --version '3.4.3'
 
       - run: ./mill compile
-      - run: ./scripts/scalatest.sh -oID -n test_util.tags.StandardSystemTest
+      - run: ./scripts/scalatest.sh -oID -PS3 -T 1000000 -n test_util.tags.StandardSystemTest
 
       - uses: actions/upload-artifact@v4
         with:
@@ -116,7 +116,7 @@ jobs:
       - run: dotnet tool install --global boogie --version '3.4.3'
 
       - run: ./mill test.compile
-      - run: ./scripts/scalatest.sh -oID -n test_util.tags.UnitTest
+      - run: ./scripts/scalatest.sh -oID -PS3 -T 1000000 -n test_util.tags.UnitTest
 
         # every test with package prefix:
         # sbt "show test:definedTests"
@@ -144,4 +144,4 @@ jobs:
       - run: echo "All systemtest suites:" && ./mill test.testOnly '*SystemTests*' -- -z 'xxxx'
 
       - run: ./mill compile
-      - run: ./scripts/scalatest.sh -oID -n test_util.tags.AnalysisSystemTest
+      - run: ./scripts/scalatest.sh -oID -PS3 -T 1000000 -n test_util.tags.AnalysisSystemTest

src/main/scala/analysis/data_structure_analysis/DataStructureAnalysis.scala

@@ -35,6 +35,9 @@ class DataStructureAnalysis(
   params: Map[Procedure, Set[Variable]]
 ) extends Analysis[Map[Procedure, Graph]] {
 
+  private val nodeCounter = NodeCounter()
+  given NodeCounter = nodeCounter
+
   val local: mutable.Map[Procedure, Graph] = mutable.Map()
   val bottomUp: mutable.Map[Procedure, Graph] = mutable.Map()
   val topDown: mutable.Map[Procedure, Graph] = mutable.Map()
@@ -58,7 +61,6 @@ class DataStructureAnalysis(
   private val queue = mutable.Queue[Procedure]()
 
   override def analyze(): Map[Procedure, Graph] = {
-    NodeCounter.reset()
     var domain: Set[Procedure] = Set(program.mainProcedure)
     val stack: mutable.Stack[Procedure] = mutable.Stack()
     stack.pushAll(program.mainProcedure.calls)

src/main/scala/analysis/data_structure_analysis/Graph.scala

@@ -26,7 +26,7 @@ import scala.util.control.Breaks.{break, breakable}
   * @param writesTo
   * @param params
   */
-class Graph(
+class Graph(using NodeCounter)(
   val proc: Procedure,
   constProp: Map[CFGPosition, Map[Variable, FlatElement[BitVecLiteral]]],
   varToSym: Map[CFGPosition, Map[Variable, Set[SymbolicAddress]]],

src/main/scala/analysis/data_structure_analysis/LocalPhase.scala

@@ -26,7 +26,7 @@ import scala.util.control.Breaks.{break, breakable}
   * @param params
   *   mapping from procedures to their parameters
   */
-class LocalPhase(
+class LocalPhase(using NodeCounter)(
   proc: Procedure,
   symResults: Map[CFGPosition, Map[SymbolicAddress, TwoElement]],
   constProp: Map[CFGPosition, Map[Variable, FlatElement[BitVecLiteral]]],

src/main/scala/analysis/data_structure_analysis/Utility.scala

@@ -12,17 +12,24 @@ import scala.collection.mutable
 import scala.collection.mutable.ArrayBuffer
 import scala.util.control.Breaks.{break, breakable}
 
-object NodeCounter {
+/**
+ * An incrementing counter for generating node IDs.
+ * An instance of this should be constructed and managed
+ * by the entry point of the DSA. A NodeCounter instance
+ * cannot be reset and should not be re-used for multiple
+ * analysis runs (a new instance should be constructed if
+ * needed).
+ *
+ * Generally, this is injected to the classes which require it
+ * by a "using" parameter.
+ */
+class NodeCounter {
   private var counter: Int = 0
 
   def getCounter: Int = {
     counter = counter + 1
     counter
   }
-
-  def reset(): Unit = {
-    counter = 0
-  }
 }
 
 class Flags() {
@@ -57,7 +64,11 @@ class Flags() {
 
 /** a Data structure Node
   */
-class Node(val graph: Option[Graph], var size: BigInt = 0, val id: Int = NodeCounter.getCounter) {
+class Node(using nodeCounter: NodeCounter)(
+  val graph: Option[Graph],
+  var size: BigInt = 0,
+  val id: Int = nodeCounter.getCounter
+) {
 
   val term: DSAUniTerm = DSAUniTerm(this)
   val children: mutable.Map[Node, BigInt] = mutable.Map()
@@ -174,7 +185,7 @@ class Node(val graph: Option[Graph], var size: BigInt = 0, val id: Int = NodeCou
   * @param offset
   *   the offset of the cell
   */
-class Cell(val node: Option[Node], val offset: BigInt) {
+class Cell(using NodeCounter)(val node: Option[Node], val offset: BigInt) {
   var largestAccessedSize: Int = 0
 
   // the cell's pointee
@@ -220,7 +231,7 @@ case class Slice(cell: Cell, internalOffset: BigInt) {
   * @param graph
   *   caller's DSG
   */
-class CallSite(val call: DirectCall, val graph: Graph) {
+class CallSite(using NodeCounter)(val call: DirectCall, val graph: Graph) {
   val proc: Procedure = call.target
   val paramCells: mutable.Map[Variable, Slice] =
     graph.params(proc).foldLeft(mutable.Map[Variable, Slice]()) { (m, reg) =>

src/main/scala/util/PerformanceTimer.scala

@@ -1,32 +1,20 @@
 package util
 import scala.collection.mutable
 import scala.collection
+import java.util.concurrent.atomic.AtomicLong
 
 case class RegionTimer(name: String) {
-  private var total: Long = 0
-  private var entered: Long = 0
-  private var inside: Boolean = false
+  private val total: AtomicLong = AtomicLong(0)
 
   def within[T](body: => T): T = {
-    enter()
+    val begin = System.currentTimeMillis()
     val result = body
-    exit()
+    val finish = System.currentTimeMillis()
+    val _ = total.addAndGet(finish - begin)
     result
   }
 
-  def enter() = {
-    require(!inside)
-    inside = true
-    entered = System.currentTimeMillis()
-  }
-
-  def exit() = {
-    require(inside)
-    inside = false
-    total += (System.currentTimeMillis() - entered)
-  }
-
-  def getTotal() = total
+  def getTotal(): Long = total.get
 
   override def toString() = {
     s"$name : ${getTotal()} (ms)"

src/test/scala/SystemTests.scala

@@ -11,13 +11,26 @@ import test_util.BASILTest
 import test_util.BASILTest.*
 import test_util.Histogram
 import test_util.TestConfig
+import test_util.LockManager
 import test_util.TestCustomisation
 
 /** Add more tests by simply adding them to the programs directory. Refer to the existing tests for the expected
   * directory structure and file-name patterns.
   */
 
-trait SystemTests extends AnyFunSuite, BASILTest, Retries, TestCustomisation {
+object SystemTests {
+
+  /** Locks are shared by all SystemTests instances. */
+  val locks = LockManager[String]()
+}
+
+trait SystemTests extends AnyFunSuite, BASILTest, TestCustomisation {
+
+  /**
+   * A suffix appended to output file names, in order to avoid clashes between test suites.
+   */
+  def testSuiteSuffix = "_" + this.getClass.getSimpleName
+
   case class TestResult(
     name: String,
     passed: Boolean,
@@ -154,15 +167,23 @@ trait SystemTests extends AnyFunSuite, BASILTest, Retries, TestCustomisation {
   def runTest(path: String, name: String, variation: String, conf: TestConfig): Unit = {
     val directoryPath = path + "/" + name + "/"
     val variationPath = directoryPath + variation + "/" + name
-    val inputPath = if conf.useBAPFrontend then variationPath + ".adt" else variationPath + ".gts"
-    val BPLPath = if conf.useBAPFrontend then variationPath + "_bap.bpl" else variationPath + "_gtirb.bpl"
+    val suiteSuffix = testSuiteSuffix
+
+    // input files:
+    val inputPath = variationPath + (if conf.useBAPFrontend then ".adt" else ".gts")
     val specPath = directoryPath + name + ".spec"
     val RELFPath = variationPath + ".relf"
-    val resultPath =
-      if conf.useBAPFrontend then variationPath + "_bap_result.txt" else variationPath + "_gtirb_result.txt"
-    val testSuffix = if conf.useBAPFrontend then ":BAP" else ":GTIRB"
+
+    // output files:
+    val lifterString = if conf.useBAPFrontend then s"_bap" else s"_gtirb"
+    val BPLPath = variationPath + lifterString + suiteSuffix + ".bpl"
+    val resultPath = variationPath + lifterString + suiteSuffix + "_result.txt"
+
+    // reference file:
     val expectedOutPath = if conf.useBAPFrontend then variationPath + ".expected" else variationPath + "_gtirb.expected"
 
+    val testSuffix = if conf.useBAPFrontend then ":BAP" else ":GTIRB"
+
     Logger.info(s"$name/$variation$testSuffix")
     val timer = PerformanceTimer(s"test $name/$variation$testSuffix")
     runBASIL(inputPath, RELFPath, Some(specPath), BPLPath, conf.staticAnalysisConfig, conf.simplify)
@@ -227,7 +248,9 @@ trait SystemTests extends AnyFunSuite, BASILTest, Retries, TestCustomisation {
         translateTime,
         verifyTime
       )
-      testResults.append(result)
+      testResults.synchronized {
+        testResults.append(result)
+      }
     }
     if (!passed) fail(boogieFailureMsg.get)
   }
@@ -250,6 +273,7 @@ trait SystemTests extends AnyFunSuite, BASILTest, Retries, TestCustomisation {
 
 @test_util.tags.StandardSystemTest
 class SystemTestsBAP extends SystemTests {
+  override def testSuiteSuffix = ""
   runTests("correct", TestConfig(useBAPFrontend = true, expectVerify = true, checkExpected = true, logResults = true))
   runTests(
     "incorrect",
@@ -262,6 +286,7 @@ class SystemTestsBAP extends SystemTests {
 
 @test_util.tags.StandardSystemTest
 class SystemTestsGTIRB extends SystemTests {
+  override def testSuiteSuffix = ""
   runTests("correct", TestConfig(useBAPFrontend = false, expectVerify = true, checkExpected = true, logResults = true))
   runTests(
     "incorrect",
@@ -276,6 +301,12 @@ class SystemTestsGTIRB extends SystemTests {
 @test_util.tags.StandardSystemTest
 class ExtraSpecTests extends SystemTests {
 
+  override def customiseTestsByName(name: String) = super.customiseTestsByName(name).orElse {
+    name match {
+      case _ => Mode.Retry("timeout issues")
+    }
+  }
+
   // some of these tests have time out issues so they need more time, but some still time out even with this for unclear reasons
   val boogieFlags = Seq("/timeLimit:30", "/proverOpt:O:smt.array.extensional=false")
   runTests(

src/test/scala/test_util/LockManager.scala

@@ -0,0 +1,49 @@
+package test_util
+
+import scala.collection.mutable.HashMap
+
+/**
+ * Manages a number of mutex locks, each identified by a key.
+ * Users should construct an instance of this class and place
+ * it in a shared location. The `withLock` method takes a lock
+ * name and executes the body while holding the specified lock.
+ *
+ * The key values will be used as HashMap keys, so they must be
+ * hashable with sensible equality.
+ */
+class LockManager[T] {
+
+  /**
+   * The map containing the locks. The "locks" are simply AnyRef
+   * objects which act as locks through the `synchronized` method.
+   * Additionally, the map object itself is used to synchronise
+   * reads/writes to the map.
+   *
+   * This is a mutable map.
+   */
+  private val locksMap: HashMap[T, AnyRef] = HashMap()
+
+  /**
+   * Executes the given body while holding the given lock.
+   * The named lock will be created if needed.
+   *
+   * Usage:
+   *
+   *     val manager = LockManager[String]()
+   *
+   *     // ...
+   *
+   *     manager.withLock("lock name") {
+   *       println("this code will hold the lock!")
+   *     }
+   */
+  def withLock(key: T)(body: => Unit): Unit = {
+    val lock = locksMap.synchronized {
+      locksMap.getOrElseUpdate(key, Object())
+    }
+
+    lock.synchronized {
+      body
+    }
+  }
+}