Increase CycleElement lookback for Cycles UI (#306)

* Add (failing) unit test for UI bug in #304

* Remove max cycles checks in E2E tests

The timeouts are a more elegant way of achieving the same goal.

* Increase CycleElement lookback

This fixes #304, but may not work in all cases.

* Remove unused variable

* Add check for valid cycles at runtime

It only emits a SEVERE log statement.

Author: lupino3

src/main/java/org/edumips64/ui/swing/CPUSwingWorker.java

@@ -128,6 +128,13 @@ private synchronized void haltCPU() {
     logger.info("Halting the CPU.");
     front.updateComponents();
     cpu.setStatus(CPU.CPUStatus.HALTED);
+
+    // Check if the transactions in the CycleBuilder are all valid.
+    for (CycleElement el : builder.getElementsList()) {
+      if (!el.isValid()) {
+        logger.severe("Invalid CycleElement for instruction " + el.getName());
+      }
+    }
     haltCallback.run();
   }
 

src/main/java/org/edumips64/utils/CycleBuilder.java

@@ -97,10 +97,18 @@ public void step() {
     // View into the last N elements of the list. This list is used to search for CycleElements
     // corresponding to a given instruction in the getElementToUpdate() method.
     //
+    // N is equal to the number of instructions in the pipeline plus the magic number 11, which represents
+    // the maximum number of stages an instruction will traverse from IF to WB.
+    //
+    // TODO: this is not a permanent fix, as there is still potential for bugs if an instruction gets stuck in
+    // the pipeline for too long (which may happen in case of MEM stalls, since the FP pipelines and EX have a
+    // deterministic precedence ordering. A permanent fix would probably look into the maximum age of the instructions
+    // in the pipeline and do something to enforce that ordering in the list of CycleElements.
+    //
     // The max() statement is needed to avoid negative indexing (and therefore errors), in the case when the
     // size of the list of elements is smaller than the number of stages in the pipeline, which happens in the
     // first cycles of a program.
-    int lookback = Math.max(elementsList.size() - instrInPipelineCount, 0);
+    int lookback = Math.max(elementsList.size() - (instrInPipelineCount + 11), 0);
     lastElements = new ArrayList<>(elementsList.subList(lookback, elementsList.size()));
 
     // If there are multiple elements for a given instruction (e.g., a fetched instruction that will not run)

src/test/java/org/edumips64/EndToEndTests.java

@@ -143,27 +143,21 @@ public void testTearDown() {
    * @param testPath path of the test code.
    */
   private CpuTestStatus runMipsTest(String testPath) throws Exception {
-    return runMipsTest(testPath, false, 0);
+    return runMipsTest(testPath, false);
   }
 
   // Version of runMipsTest that allows to specify whether to write a trace file.
   // Writing the trace file can unnecessarily slow down the tests, so by default they are not written (see previous
   // override of this method).
-  //
-  // maxCycles is the maximum number of cycles to allow being executed before considering
-  // the test failed. Necessary for test cases for bugs that throw the CPU in an infinite loop,
-  // such as for example disappearing instructions that set a read/write semaphore, making
-  // it impossible to test their values. If > 0, an exception will be thrown.
-  private CpuTestStatus runMipsTest(String testPath, boolean writeTracefile, int maxCycles) throws Exception {
+  private CpuTestStatus runMipsTest(String testPath, boolean writeTracefile) throws Exception {
     log.warning("================================= Starting test " + testPath + " (forwarding: " +
-        config.getBoolean(ConfigKey.FORWARDING) + ") (max cycles: " + maxCycles + ")");
+        config.getBoolean(ConfigKey.FORWARDING)+ ")");
     cpu.reset();
     dinero.reset();
     symTab.reset();
     builder.reset();
     testPath = testsLocation + testPath;
     String tracefile = null;
-    int curCycles = 0;
 
     try {
       try {
@@ -181,11 +175,6 @@ private CpuTestStatus runMipsTest(String testPath, boolean writeTracefile, int m
       cpu.setStatus(CPU.CPUStatus.RUNNING);
 
       while (true) {
-        curCycles++;
-        if (maxCycles > 0 && curCycles > maxCycles) {
-          log.severe("Exceeded maximum number of cycles allowed: " + maxCycles);
-          throw new HaltException();
-        }
         cpu.step();
         builder.step();
       }
@@ -202,11 +191,6 @@ private CpuTestStatus runMipsTest(String testPath, boolean writeTracefile, int m
         w.close();
       }
 
-      // Check if the execution exceeded the maximum allowed number of cycles.
-      if (maxCycles > 0 && curCycles > maxCycles) {
-        throw new TestTookTooLongException();
-      }
-
       // Check if the transactions in the CycleBuilder are all valid.
       boolean allValid = true;
       for (CycleElement el : builder.getElementsList()) {
@@ -258,7 +242,7 @@ private void runForwardingTest(String path, int expected_cycles_with_forwarding,
   }
 
   private void runTestAndCompareTracefileWithGolden(String path) throws Exception {
-    CpuTestStatus status = runMipsTest(path, true, 0);
+    CpuTestStatus status = runMipsTest(path, true);
     String goldenTrace = testsLocation + path + ".xdin.golden";
 
     String golden, trace;
@@ -615,6 +599,13 @@ public void testLdLargeImmediate() throws Exception {
   */
   @Test(timeout=2000)
   public void testIssue304() throws Exception {
-    runMipsTest("issue304.s", false, 30);
+    runMipsTest("issue304.s", false);
+  }
+
+  /* Issue #304: Missing MEM/WB in Cycles UI for some FPU instructions.
+  */
+  @Test(timeout=2000)
+  public void testIssue304UI() throws Exception {
+    runMipsTest("infinite-bug-304.s", false);
   }
 }

src/test/resources/infinite-bug-304.s

@@ -0,0 +1,58 @@
+;--------------------------------------------------------------------------
+;
+;
+; THIS CODE demonstrates an apparent bug in EDUMIPS.
+; Tested using EduMIPS64 v1.2.6 (standalone) and v1.2.5
+;
+;--------------------------------------------------------------------------
+               .data         ; start of data space
+
+scalara:       .double  -3.7 ; first scalar
+scalarb:       .double  5.1  ; second scalar
+vectorX:       .double  -5.78;
+vectorXnext:   .double  10.26;
+vectorXnext2:  .double  -1.9,    0.001,   3.002, -1257.72,  1009.235,    899.0001,  9.9999, 0.000, -30.567, -10020.5  ; first vector
+vectorY:       .double  -4.88;
+vectorYnext:   .double  202.21;
+vectorYnext2:  .double  31.8, -206.112, 100.7,     -88.12,      0.77,  10031.4,    -3.02,   0.000,  15.5,      622.80 ; second vector
+vectorZ:       .space  96    ; space set aside for result
+
+;--------------------------------------------------------------------------
+; CODE SPACE
+;--------------------------------------------------------------------------
+               .text        ; start of code space
+
+main:
+          L.D f1, scalara(r0) 
+          L.D f2, scalarb(r0)
+          DADDI r3, r0, 0
+
+          DADDI r1, r0, #88 ; 
+
+          L.D f3, vectorX(r3) 
+          L.D f4, vectorY(r3) 
+          MUL.D f5, f1, f3     
+          MUL.D f6, f2, f4   
+
+          L.D f3, vectorXnext(r3) 
+          L.D f4, vectorYnext(r3) 
+
+          ADD.D f7, f5, f6
+ 
+          MUL.D f5, f1, f3   ; THIS INSTRUCTION'S WB HAPPENS, but the CYCLE WINDOW does not show the MEM and WB stages.
+          MUL.D f6, f2, f4   ; THIS INSTRUCTION'S WB HAPPENS, but the CYCLE WINDOW does not show the M7, MEM and WB stages.
+
+loop:      S.D f7, vectorZ(r3) 
+
+           L.D f3, vectorXnext2(r3) 
+           L.D f4, vectorYnext2(r3) 
+           ;DADDI r5, r0, 0   ; DUMMY INSTRUCTION INSERTED TO CIRCUMVENT BUG.
+           ADDI r3, r3, 8     ; THIS INSTRUCTION'S WB NEVER HAPPENS.  CYCLE WINDOW shows no MEM OR WB stages.
+           ADD.D f7, f5, f6   
+           MUL.D f5, f1, f3   
+           MUL.D f6, f2, f4   
+           BNE r3, r1, loop   ; CONSEQUENTLY, THIS INSTRUCTION INFINITELY RAW STALLS.
+
+           SYSCALL 0
+
+