Test fails in rti.dart due to failed null check when run on node, passes in VM

[mkorbel1]

I'm working on upgrading the ROHD package to run properly (passing test suite) when compiled to JavaScript (dart test -p node) (intel/rohd#445)

There's one test in the suite failing when run on node with what appears to be a null check somewhere inside Dart's js libraries. It's not really doing something significantly different than many other tests in the suite, so I'm not sure how to reduce it to a smaller reproduction. The same test passes fine in the Dart VM.

I'm running on WSL2 Ubuntu 22, with Dart version Dart SDK version: 3.2.3 (stable) (None) on "linux_x64" and node version v20.10.0.

The failure signature is:

  Null check operator used on a null value
  org-dartlang-sdk:///lib/_internal/js_shared/lib/rti.dart 883:10  Object._instanceType
  package:rohd/src/modules/conditional.dart 782:5                  If._driveX
  package:rohd/src/modules/conditional.dart 1443:9                 If.execute
  package:rohd/src/modules/conditional.dart 1438:23                If.execute
  package:rohd/src/modules/conditional.dart 572:19                 Sequential._execute
  package:rohd/src/modules/conditional.dart 524:13                 Sequential._setup.<fn>.<fn>
  org-dartlang-sdk:///lib/async/zone.dart 1407:46                  StaticClosure._rootRunUnary
  org-dartlang-sdk:///lib/async/zone.dart 1307:34                  _CustomZone.runUnary
  org-dartlang-sdk:///lib/async/future_impl.dart 127:29            _Future._propagateToListeners.handleValueCallback
  org-dartlang-sdk:///lib/async/future_impl.dart 875:13            Object._Future._propagateToListeners

To reproduce, include this version of rohd in pubspec.yaml:

  rohd:
    git:
      url: https://github.com/mkorbel1/rohd.git
      ref: js

Then run the following test (dart test -p node translationjs.dart)

// Copyright (C) 2021-2023 Intel Corporation
// SPDX-License-Identifier: BSD-3-Clause

// ignore_for_file: avoid_multiple_declarations_per_line

import 'package:rohd/rohd.dart';
import 'package:rohd/src/utilities/simcompare.dart';
import 'package:test/test.dart';

class FlopArrayPort {
  final Logic en, ptr, data;
  FlopArrayPort(this.en, this.ptr, this.data);
}

class FlopArray extends Module {
  // for convenience, make it easier to access inputs and outputs of this module
  Logic get lclk => input('lclk');
  Logic get lrst => input('lrst');
  Logic rdData(int idx) => _rdPorts[idx].data;

  final List<FlopArrayPort> _wrPorts = [], _rdPorts = [];

  final int numWrites, numReads, awidth, dwidth, numEntries;
  FlopArray(Logic lclk, Logic lrst, List<Logic> rdEn, List<Logic> rdPtr,
      List<Logic> wrEn, List<Logic> wrPtr, List<Logic> wrData,
      {this.numEntries = 8})
      : numWrites = wrEn.length,
        numReads = rdEn.length,
        dwidth = wrData[0].width,
        awidth = rdPtr[0].width,
        super(name: 'floparray_${wrEn.length}w_${rdEn.length}r') {
    // make sure widths of everything match expectations
    if (rdPtr.length != numReads) {
      throw Exception('Read pointer length must match number of read enables.');
    }
    if (wrPtr.length != numWrites) {
      throw Exception(
          'Write pointer length must match number of write enables');
    }
    if (wrData.length != numWrites) {
      throw Exception('Write data length must match number of write enables');
    }

    // register inputs and outputs with ROHD
    addInput('lclk', lclk);
    addInput('lrst', lrst);
    for (var i = 0; i < numReads; i++) {
      _rdPorts.add(FlopArrayPort(
          addInput('rdEn$i', rdEn[i]),
          addInput('rdPtr$i', rdPtr[i], width: awidth),
          addOutput('rdData$i', width: dwidth)));
    }
    for (var i = 0; i < numWrites; i++) {
      _wrPorts.add(FlopArrayPort(
          addInput('wrEn$i', wrEn[i]),
          addInput('wrPtr$i', wrPtr[i], width: awidth),
          addInput('wrData$i', wrData[i], width: dwidth)));
    }

    _buildLogic();
  }

  void _buildLogic() {
    // create local storage bank
    final storageBank = List<Logic>.generate(
        numEntries, (i) => Logic(name: 'storageBank_$i', width: dwidth));

    // Sequential(lclk, [  // normally this should be here
    Sequential(SimpleClockGenerator(10).clk, [
      //for testing purposes, easier to just plug a clock in here
      If(lrst, then: [
        ...storageBank
            .map((e) => e < 0) // zero out entire storage bank on reset
      ], orElse: [
        ...List.generate(
            numEntries,
            (entry) => [
                  ..._wrPorts.map((wrPort) =>
                      // set storage bank if write enable and pointer matches
                      If(wrPort.en & wrPort.ptr.eq(entry),
                          then: [storageBank[entry] < wrPort.data])),
                  ..._rdPorts.map((rdPort) =>
                      // read storage bank if read enable and pointer matches
                      If(rdPort.en & rdPort.ptr.eq(entry),
                          then: [rdPort.data < storageBank[entry]])),
                ]).expand((e) => e) // flatten
      ]),
    ]);
  }
}

void main() {
  tearDown(() async {
    await Simulator.reset();
  });

  group('simcompare', () {
    test('translation', () async {
      const numRdPorts = 2;
      const numWrPorts = 2;
      final ftm = FlopArray(
        Logic(),
        Logic(),
        List<Logic>.generate(numRdPorts, (index) => Logic()),
        List<Logic>.generate(numRdPorts, (index) => Logic(width: 6)),
        List<Logic>.generate(numWrPorts, (index) => Logic()),
        List<Logic>.generate(numWrPorts, (index) => Logic(width: 6)),
        List<Logic>.generate(numWrPorts, (index) => Logic(width: 16)),
      );
      await ftm.build();

      final vectors = [
        Vector({'lrst': 0}, {}),
        Vector({'lrst': 1}, {}),
        Vector({'lrst': 1, 'wrEn0': 0, 'rdEn0': 0, 'wrEn1': 0, 'rdEn1': 0}, {}),
        Vector({'lrst': 0}, {}),
        Vector({'wrEn1': 1, 'wrPtr1': 4, 'wrData1': 0xf}, {}),
        Vector({'wrEn1': 0, 'rdEn0': 1, 'rdPtr0': 4}, {}),
        // Vector({'wrEn1': 0, 'rdEn0': 0}, {'rdData0': 0xf}),
      ];
      await SimCompare.checkFunctionalVector(ftm, vectors);
    });
  });
}

[fishythefish]

I'm having a little trouble distilling an isolated repro since it's hard to get the compiler to codegen smaller code in the same way, but I can identify the issue:

Here's the function that produces the error:

void _driveX(Set<Logic>? drivenSignals) {
  for (final receiver in receivers) {
    receiverOutput(receiver).put(LogicValue.x);
    if (drivenSignals != null &&
        (!drivenSignals.contains(receiver) || receiver.value.isValid)) {
      drivenSignals.add(receiver);
    }
  }
}

Here's what we compile it to (with default flags):

_driveX$1(drivenSignals) {
  var t1, t2, t3, t4, t5, _i, t6, t7;
  type$.nullable_Set_Logic._as(drivenSignals);
  for (t1 = this.get$receivers(), t2 = t1.length, t3 = drivenSignals != null, t4 = type$.Logic, t5 = A._instanceType(drivenSignals)._precomputed1, _i = 0; _i < t1.length; t1.length === t2 || (0, A.throwConcurrentModificationError)(t1), ++_i) {
    t6 = t4._as(t1[_i]);
    t7 = this.__Conditional__assignedReceiverToOutputMap_A;
    t7 === $ && A.throwLateFieldNI("_assignedReceiverToOutputMap");
    t7 = t7.$index(0, t6);
    t7.toString;
    t7._wire.put$3$fill$signalName(B._FilledLogicValue__LogicValueEnum_2_1, false, t7.name);
    if (t3)
      t7 = !drivenSignals._duplicate_detection_set$_set.contains$1(0, t6) || t6._wire._currentValue.get$isValid();
    else
      t7 = false;
    if (t7) {
      t5._as(t6);
      t7 = drivenSignals._duplicate_detection_set$_set;
      if (t7.contains$1(0, t6))
        drivenSignals._duplicates.add$1(0, t6);
      t7.add$1(0, t6);
    }
  }
}

The issue here is with t5 = A._instanceType(drivenSignals)._precomputed1. It's only safe to call _instanceType here after drivenSignals is known to be non-null, but we hoist it out of the loop. This means that when drivenSignals is null, we'll attempt to access $ti and fail.

Our error reporting then tries to be helpful and incorrectly reports this as Null check operator used on a null value (because a null check followed by a property access would fail in the same way).

@rakudrama: What's the right way to fix the hoisting logic here?

[rakudrama]

Smaller repro:

class DupSet<T> {
  void add(T e) {}

  @pragma('dart2js:never-inline')
  bool contains(Object? e) { return gI++ > 10; }
}

int gI = 0;

class Logic {}
class Logic1 extends Logic {}

void loop(List<Logic> receivers, DupSet<Logic>? drivenSignals) {
  for (final receiver in receivers) {
    if (drivenSignals != null && !drivenSignals.contains(receiver)) {
      drivenSignals.add(receiver);
    }
  }
}

main() {
  gI = 100;
  final ds = DupSet<Logic>();

  loop([Logic()], ds);
  loop([Logic()], null);
  loop([Logic1()], DupSet<Logic1>());
}

    loop(receivers, drivenSignals) {
      var t1, t2, _i, receiver;
      for (t1 = drivenSignals != null, t2 = A._instanceType(drivenSignals)._precomputed1, _i = 0; _i < 1; ++_i) {
        receiver = receivers[_i];
        if (t1 && !drivenSignals.contains$1(0, receiver))
          t2._as(receiver);
      }
    },

(DupSet.add is inlined, leaving just the e as T of the parametric covariance check (t2._as(receiver))).

The condition for the if is essentially

    bool cond =  drivenSignals != null && !drivenSignals.contains(receiver);
    if (cond) {
      drivenSignals.add(receiver);
    }

so there is no strengthened dominating input.

I think what is happening is that the type expression is generated based on the (inferred) static type of the argument in the condition. The hoisting is invalid, since the static type of the argument is different at the hoisted location.
This might be fixed by making the inferred static type explicit by generating pinned HTypeKnown instructions. These extra instructions should be eliminated if there is no material control flow dependency.

[sigmundch]

We are still investigating the root cause.

For what it's worth, I was able to elide the issue in @rakudrama's example by adding a line that obfuscates what's going on and makes the compiler act conservatively. For example:

...
// TODO: delete after #54329 is fixed
@pragma('dart2js:never-inline')
confuseTheCompiler(x) => x;

...
    if (drivenSignals != null && !drivenSignals.contains(receiver)) {
      confuseTheCompiler(drivenSignals).add(receiver);
    }

@mkorbel1 - you could try a similar hack to workaround this issue on your end while we work on a proper fix. Let us know if you get unblocked in doing so!

[mkorbel1]

While it did not work in the initial location I tried (on the _driveX function), it looks like this gets the test to pass when placed here:

lib/src/collections/duplicate_detection_set.dart

  @override
  @pragma('dart2js:never-inline')
  bool add(T value) {
    if (_set.contains(value)) {
      _duplicates.add(value);
    }

    return _set.add(value);
  }

However, I don't really understand the bug or whether adding this @pragma in a release of the rohd package would reduce the chances of other users hitting this issue in another way. Do you expect that it's a viable workaround or just whacking one of the many moles that might hit the bug? Is this a unique/special pattern in the rohd package that excites the bug that is unlikely to appear elsewhere?

[fishythefish]

The bug is an invalid optimization that dart2js is performing, and it could arise in other ways. We're working on isolating the root cause and fixing the bug, but we want to offer a workaround you can use in the interim in case this is blocking.

Putting that pragma on add should prevent users from hitting this issue via add being inlined, but it will also prevent benign/beneficial inlining of add. Siggi's suggestion is a bit more targeted - add a no-op function like confuseTheCompiler, annotate it with the pragma to prevent it from being inlined, then change the implementation of _driveX (as linked in #54329 (comment)) to say confuseTheCompiler(drivenSignals).add(receiver) instead of just drivenSignals.add(receiver). Based on the stacktrace, that will hopefully stop this test from failing.

---

Explaining why the bug occurs and the workaround works is a little involved, so consider this optional reading. In the implementation of _driveX, you invoke DuplicateDetectionSet<T>.add. Dart semantics require us to perform a runtime check that the argument you pass to .add does actually have type T. dart2js ends up inlining the call to .add, leaving the type check directly in the body of _driveX.

In order to perform the type check, we have to know what concrete type T actually is. Based on the static type of drivenSignals, we know that we can extract T via a particular sequence of implementation-specific property accesses. Of course, we can only access these properties if drivenSignals is not null. Ordinarily, this wouldn't be an issue since all of the relevant code is guarded by drivenSignals != null. However, due to a bug we're trying to identify, we hoist the code that extracts T outside of the null check, which is why you see a (slightly confusing) null error.

Putting the pragma directly on add does work around the issue - if add isn't inlined, then the type check happens in the body of add rather than _driveX, so no part of the type checking code can get hoisted out of the null check in _driveX. However, this does prevent add from being inlined anywhere.

Instead, Siggi suggests wrapping the receiver drivenSignals in a confuseTheCompiler function before invoking .add on it in _driveX. Recall that dart2js knew how to extract T from drivenSignals because of the static type of drivenSignals. Since confuseTheCompiler is marked never-inline and because it doesn't have static type annotations, the compiler essentially thinks confuseTheCompiler(drivenSignals) could be dynamic. Therefore, it generates different code to extract T, and this code doesn't have the hoisting issue described above.

[rakudrama]

Fixed by 7933cd7