tests: thread-safe test suites

.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
+    }
+  }
+}