Repository: intel/rohd (dart)

Issue:
Improve error message of `Signal changed its value after ...`
### Motivation

I am getting this error message but have no idea what signal has the issue. 

```
Signal changed its value after being used within one `Combinational` execution. This can 
lead to a mismatch between simulation and synthesis. You may be able to use 
`Combinational.ssa` to correct your design with minimal refactoring.
```


### Desired solution

Indicate what signal/object has the problem

### Alternatives considered

_No response_

### Additional details

This was seen when using signals inside a FiniteStateMachine.

Pull request:
<!-- Please make sure you check out the contribution guidelines before submitting a pull request! -->

## Description & Motivation

Debugging `WriteAfterReadException` is difficult because the message does not provide sufficient context as to what caused the error.  Adding an indication on the endpoint where the `guard` was violated has two downsides:
- It only gives limited information on the signal that was a problem, but not how it got there
- Adding that information would cause a measurable performance impact in general `Combinational` execution since it would allocate more in each execution.

This PR introduces a `try`/`catch` block that appends a sort of stack trace of information on where the write-after-read occurred and how it got there, which dramatically helps with debug in a useful way.  This approach also has a negligible performance overhead (confirmed in benchmarks).

Also, added a new test which mimics a relatively complicated write-after-read error to help protect functionality and verify the error message is helpful.

Also, made it so that `Case` and `If` had slightly more informative `toString()` implementations.

## Related Issue(s)

Fix #569

## Testing

Added new tests -- existing tests cover nothing broke.

## Backwards-compatibility

> Is this a breaking change that will not be backwards-compatible? If yes, how so?

No

## Documentation

> Does the change require any updates to documentation? If so, where? Are they included?

No

Patch:
diff --git a/lib/src/exceptions/conditionals/write_after_read_exception.dart b/lib/src/exceptions/conditionals/write_after_read_exception.dart
index 757966625..ea46edfe4 100644
--- a/lib/src/exceptions/conditionals/write_after_read_exception.dart
+++ b/lib/src/exceptions/conditionals/write_after_read_exception.dart
@@ -1,4 +1,4 @@
-// Copyright (C) 2023 Intel Corporation
+// Copyright (C) 2023-2025 Intel Corporation
 // SPDX-License-Identifier: BSD-3-Clause
 //
 // write_after_read_exception.dart
@@ -7,17 +7,43 @@
 // 2023 April 13
 // Author: Max Korbel <max.korbel@intel.com>
 
+import 'package:meta/meta.dart';
 import 'package:rohd/src/exceptions/rohd_exception.dart';
 
 /// An exception that is thrown when a "write after read" violation occurs.
 ///
 /// This is also sometimes called a "read before write" violation.
 class WriteAfterReadException extends RohdException {
+  /// The path of things that caused the exception.
+  final List<String>? _path;
+
+  /// Creates a [WriteAfterReadException].
+  WriteAfterReadException() : this._();
+
   /// Creates a [WriteAfterReadException].
-  WriteAfterReadException()
-      : super('Signal changed its value after being used'
+  WriteAfterReadException._([this._path])
+      : super(_appendPath(
+            'Signal changed its value after being used'
             ' within one `Combinational` execution.'
             ' This can lead to a mismatch between simulation and synthesis.'
             ' You may be able to use `Combinational.ssa` to correct your'
-            ' design with minimal refactoring.');
+            ' design with minimal refactoring.',
+            _path));
+
+  /// Appends a [path] to the [message], if it exists.
+  static String _appendPath(String message, List<String>? path) {
+    if (path == null || path.isEmpty) {
+      return message;
+    }
+    return '$message\n${path.join('\n')}';
+  }
+
+  /// Clones this [WriteAfterReadException] with an added [pathItem] item to the
+  /// top of the path.
+  @internal
+  WriteAfterReadException cloneWithAddedPath(String pathItem) =>
+      WriteAfterReadException._([
+        pathItem,
+        ...?_path,
+      ]);
 }
diff --git a/lib/src/modules/conditionals/case.dart b/lib/src/modules/conditionals/case.dart
index 590437391..34e39a24f 100644
--- a/lib/src/modules/conditionals/case.dart
+++ b/lib/src/modules/conditionals/case.dart
@@ -1,4 +1,4 @@
-// Copyright (C) 2021-2024 Intel Corporation
+// Copyright (C) 2021-2025 Intel Corporation
 // SPDX-License-Identifier: BSD-3-Clause
 //
 // case.dart
@@ -364,6 +364,16 @@ ${subPadding}end
 
     return newMappings;
   }
+
+  @override
+  String toString() {
+    final buffer = StringBuffer()..writeln('$caseType($expression, ');
+    for (final item in items) {
+      buffer.writeln('  $item');
+    }
+    buffer.writeln(')');
+    return buffer.toString();
+  }
 }
 
 /// A special version of [Case] which can do wildcard matching via `z` in
diff --git a/lib/src/modules/conditionals/conditional.dart b/lib/src/modules/conditionals/conditional.dart
index 05653d5b6..0bc6fc410 100644
--- a/lib/src/modules/conditionals/conditional.dart
+++ b/lib/src/modules/conditionals/conditional.dart
@@ -193,8 +193,12 @@ abstract class Conditional {
   /// Drives X to all receivers.
   @protected
   void driveX(Set<Logic>? drivenSignals) {
-    for (final receiverOutput in _receiverOutputs) {
-      receiverOutput.put(LogicValue.x);
+    try {
+      for (final receiverOutput in _receiverOutputs) {
+        receiverOutput.put(LogicValue.x);
+      }
+    } on WriteAfterReadException catch (e) {
+      throw e.cloneWithAddedPath('  at (driving X) $this');
     }
 
     drivenSignals?.addAll(receivers);
diff --git a/lib/src/modules/conditionals/conditional_assign.dart b/lib/src/modules/conditionals/conditional_assign.dart
index 3e783bba0..524f46462 100644
--- a/lib/src/modules/conditionals/conditional_assign.dart
+++ b/lib/src/modules/conditionals/conditional_assign.dart
@@ -56,12 +56,17 @@ class ConditionalAssign extends Conditional {
     }
 
     final currentValue = driverValue(driver);
-    if (!currentValue.isValid) {
-      // Use bitwise & to turn Z's into X's, but keep valid signals as-is.
-      // It's too pessimistic to convert the whole bus to X.
-      _receiverOutput.put(currentValue & currentValue);
-    } else {
-      _receiverOutput.put(currentValue);
+
+    try {
+      if (!currentValue.isValid) {
+        // Use bitwise & to turn Z's into X's, but keep valid signals as-is.
+        // It's too pessimistic to convert the whole bus to X.
+        _receiverOutput.put(currentValue & currentValue);
+      } else {
+        _receiverOutput.put(currentValue);
+      }
+    } on WriteAfterReadException catch (e) {
+      throw e.cloneWithAddedPath('  at $this');
     }
 
     if (drivenSignals != null &&
diff --git a/lib/src/modules/conditionals/if.dart b/lib/src/modules/conditionals/if.dart
index 064a20a43..4d0776982 100644
--- a/lib/src/modules/conditionals/if.dart
+++ b/lib/src/modules/conditionals/if.dart
@@ -1,4 +1,4 @@
-// Copyright (C) 2021-2024 Intel Corporation
+// Copyright (C) 2021-2025 Intel Corporation
 // SPDX-License-Identifier: BSD-3-Clause
 //
 // if.dart
@@ -272,4 +272,18 @@ ${padding}end ''');
 
     return newMappings;
   }
+
+  @override
+  String toString() {
+    final buffer = StringBuffer()..writeln('If(');
+    for (final iff in iffs) {
+      if (iff is Else) {
+        buffer.writeln('  Else  ...');
+      } else {
+        buffer.writeln('  ${iff.condition.name} ...');
+      }
+    }
+    buffer.writeln(')');
+    return buffer.toString();
+  }
 }


Test patch:
diff --git a/test/comb_mod_test.dart b/test/comb_mod_test.dart
index bd09ceb94..a4df8a47d 100644
--- a/test/comb_mod_test.dart
+++ b/test/comb_mod_test.dart
@@ -1,4 +1,4 @@
-// Copyright (C) 2021-2023 Intel Corporation
+// Copyright (C) 2021-2025 Intel Corporation
 // SPDX-License-Identifier: BSD-3-Clause
 //
 // comb_mod_test.dart
@@ -92,11 +92,165 @@ class DuplicateExampleSsa extends Module {
   }
 }
 
+class SingleReadAndWriteRf extends Module {
+  SingleReadAndWriteRf(
+      {required Logic reset,
+      required Logic writeEn,
+      required Logic writeData,
+      required Logic writeAddr,
+      required Logic readEn,
+      required Logic readAddr,
+      bool useSsa = false})
+      : super(name: 'singlereadandwriterf') {
+    final clk = SimpleClockGenerator(10).clk;
+
+    const numEntries = 16;
+    const dataWidth = 8;
+    const addrWidth = 4;
+
+    reset = addInput('reset', reset);
+    writeEn = addInput('writeEn', writeEn);
+    writeData = addInput('writeData', writeData, width: dataWidth);
+    writeAddr = addInput('writeAddr', writeAddr, width: addrWidth);
+    readEn = addInput('readEn', readEn);
+    readAddr = addInput('readAddr', readAddr, width: addrWidth);
+
+    final readData = addOutput('readData', width: dataWidth);
+
+    final storageBank = List<Logic>.generate(
+        numEntries, (i) => Logic(name: 'storageBank_$i', width: dataWidth));
+
+    final internalReadEn = Logic(name: 'internalReadEn');
+    final internalReadAddr = Logic(name: 'internalReadAddr', width: addrWidth);
+    final internalReadData = Logic(name: 'internalReadData', width: dataWidth);
+
+    Sequential(clk, [
+      If(reset, then: [
+        ...storageBank.map((e) => e < 0)
+      ], orElse: [
+        for (var entry = 0; entry < numEntries; entry++)
+          If(writeEn & writeAddr.eq(entry), then: [
+            storageBank[entry] < writeData,
+          ]),
+      ]),
+    ]);
+
+    Combinational(name: 'rf_read', [
+      If(~internalReadEn, then: [
+        internalReadData < Const(0, width: dataWidth)
+      ], orElse: [
+        Case(internalReadAddr, [
+          for (var entry = 0; entry < numEntries; entry++)
+            CaseItem(Const(LogicValue.ofInt(entry, addrWidth)),
+                [internalReadData < storageBank[entry]])
+        ], defaultItem: [
+          internalReadData < Const(0, width: dataWidth)
+        ])
+      ])
+    ]);
+
+    if (useSsa) {
+      Combinational.ssa(
+          name: 'accessor',
+          (s) => [
+                readData < 0,
+                s(internalReadEn) < 0,
+                If(readEn, then: [
+                  s(internalReadEn) < readEn,
+                  internalReadAddr < readAddr,
+                  readData < internalReadData,
+                ])
+              ]);
+    } else {
+      Combinational(name: 'accessor', [
+        readData < 0,
+        internalReadEn < 0,
+        If(readEn, then: [
+          internalReadEn < readEn,
+          internalReadAddr < readAddr,
+          readData < internalReadData,
+        ])
+      ]);
+    }
+  }
+}
+
 void main() {
   tearDown(() async {
     await Simulator.reset();
   });
 
+  group('simulatenous read and write rf', () {
+    for (final useSsa in [false, true]) {
+      test('useSsa = $useSsa', () async {
+        final dut = SingleReadAndWriteRf(
+            reset: Logic(),
+            writeEn: Logic(),
+            writeData: Logic(width: 8),
+            writeAddr: Logic(width: 4),
+            readEn: Logic(),
+            readAddr: Logic(width: 4),
+            useSsa: useSsa);
+
+        await dut.build();
+
+        final vectors = [
+          Vector({
+            'reset': 1,
+            'writeEn': 0,
+            'writeData': 0,
+            'writeAddr': 0,
+            'readEn': 0,
+            'readAddr': 0
+          }, {}),
+          Vector({
+            'reset': 0,
+            'writeEn': 0,
+            'writeData': 0,
+            'writeAddr': 0,
+            'readEn': 1,
+            'readAddr': 3
+          }, {
+            'readData': 0
+          }),
+          Vector({
+            'reset': 0,
+            'writeEn': 1,
+            'writeData': 5,
+            'writeAddr': 3,
+            'readEn': 1,
+            'readAddr': 3
+          }, {
+            'readData': 0
+          }),
+          Vector({
+            'reset': 0,
+            'writeEn': 0,
+            'writeData': 0,
+            'writeAddr': 0,
+            'readEn': 1,
+            'readAddr': 3
+          }, {
+            'readData': 5
+          }),
+        ];
+
+        if (useSsa) {
+          await SimCompare.checkFunctionalVector(dut, vectors);
+          SimCompare.checkIverilogVector(dut, vectors);
+        } else {
+          try {
+            await SimCompare.checkFunctionalVector(dut, vectors);
+            fail('Expected a write after read exception!');
+          } on Exception catch (e) {
+            expect(e, isA<WriteAfterReadException>());
+            expect(e.toString(), contains('internalReadEn '));
+          }
+        }
+      });
+    }
+  });
+
   group('module reuse', () {
     test('should fail normally', () async {
       try {