circt icon indicating copy to clipboard operation
circt copied to clipboard

[FIRParser] unexpectedly accepting of invalid FIRRTL

Open dtzSiFive opened this issue 2 years ago • 3 comments

This started with a module definition at the wrong indentation level-- we currently we accept "FIRRTL" like this:

circuit Oops:
  module Oops:
  input in: UInt
  input out: UInt
  wire w: UInt
  w <- in
  out <- w

Producing an empty module "Oops" and discarding the statements.

Poking at things, it seems we even accept things like:

circuit Oops:
  module Oops:
    input in: UInt<1>
    output out: UInt<1>
    out <- in

  asdf:
    dsfa

asdf
input in: UInt
  input out: UInt
  wire w: UInt
  w <- in
  out <- w
  input in: UInt

dtzSiFive avatar Jun 03 '22 20:06 dtzSiFive

Yeah, this is wrong. This is one area where the FIRRTL spec actually has a lot of clarifying information on how this is supposed to work (which is admittedly based on how the SFC works).

seldridge avatar Jun 04 '22 00:06 seldridge

Looks like we call success when parsing statements once the indent level drops too low (below level started at), and since we mostly-blindly chunk the input by "module"/EOF (or so, for parallel processing), this combines to result in anything at indent level <= 1 before the next 'module' or EOF being allowed (as seen in example above, once indent level drops anything goes).

I haven't looked into where it might make sense to check for these sorts of unprocessed bits (or otherwise how best to address this). I'll take a look at that part of FIRRTL spec, thanks!

This also explains behavior encountered in #2978 (second circuit's module parsed into first circuit without a warning).

dtzSiFive avatar Jun 08 '22 01:06 dtzSiFive

Previously: #2787.

dtzSiFive avatar Sep 21 '22 22:09 dtzSiFive