[FIRRTL] InferResets: deleting op used to drive reset network results in use-after-free

[youngar]

firrtl.circuit "MovableNodeShouldDominate" {
  firrtl.module @MovableNodeShouldDominate(in %clock: !firrtl.clock) {
    %child_clock = firrtl.instance child @Child(in clock: !firrtl.clock)
    firrtl.connect %child_clock, %clock : !firrtl.clock
    %ui1 = firrtl.constant 1 : !firrtl.uint<1>
    %0 = firrtl.asAsyncReset %ui1 : (!firrtl.uint<1>) -> !firrtl.asyncreset
    %localReset = firrtl.node %0 {annotations = [{class = "sifive.enterprise.firrtl.FullAsyncResetAnnotation"}]} : !firrtl.asyncreset
  }
  firrtl.module @Child(in %clock: !firrtl.clock) {
    %reg = firrtl.reg %clock : !firrtl.clock, !firrtl.uint<8>
  }
}

./bin/circt-opt --pass-pipeline='builtin.module(firrtl.circuit(firrtl-infer-resets))' reset.mlir

InferResets needs to wire the result of the NodeOp to the instance of Child, and to do so replaces it with a wire. This results in a dangling reference to the deleted node stored in the ResetDomain.

Running it under valgrind yields many errors, such as the one below, but my line numbers won't line up with main due to a lot of printf debugging:

==176563== Invalid read of size 8
==176563==    at 0x1C35A53: mlir::Value::getLoc() const (llvm/mlir/lib/IR/Value.cpp:0)
==176563==    by 0x12A6F98: implementAsyncReset (lib/Dialect/FIRRTL/Transforms/InferResets.cpp:1688)
==176563==    by 0x12A6F98: implementAsyncReset (lib/Dialect/FIRRTL/Transforms/InferResets.cpp:1647)
==176563==    by 0x12A6F98: (anonymous namespace)::InferResetsPass::runOnOperationInner() (lib/Dialect/FIRRTL/Transforms/InferResets.cpp:545)
==176563==    by 0x12A4465: (anonymous namespace)::InferResetsPass::runOnOperation() (lib/Dialect/FIRRTL/Transforms/InferResets.cpp:513)

==176563==  Address 0x613f688 is 40 bytes inside a block of size 128 free'd
==176563==    at 0x4C39A93: free (vg_replace_malloc.c:872)
==176563==    by 0x18E466D: llvm::iplist_impl<llvm::simple_ilist<mlir::Operation>, llvm::ilist_traits<mlir::Operation> >::erase(llvm::ilist_iterator<llvm::ilist_detail::node_options<mlir::Operation, true, false, void, false>, false, false>) (ilist.h:205)
==176563==    by 0x12A7755: erase (OpDefinition.h:131)
==176563==    by 0x12A7755: implementAsyncReset (lib/Dialect/FIRRTL/Transforms/InferResets.cpp:1742)
==176563==    by 0x12A7755: implementAsyncReset (lib/Dialect/FIRRTL/Transforms/InferResets.cpp:1647)
==176563==    by 0x12A7755: (anonymous namespace)::InferResetsPass::runOnOperationInner() (lib/Dialect/FIRRTL/Transforms/InferResets.cpp:545)
==176563==    by 0x12A4465: (anonymous namespace)::InferResetsPass::runOnOperation() (lib/Dialect/FIRRTL/Transforms/InferResets.cpp:513)

[darthscsi]

Might it be enough to just add the wire and tie the output of the node to the wire rather than trying to replace the node?