chipsalliance / verible

Verible is a suite of SystemVerilog developer tools, including a parser, style-linter, formatter and language server
https://chipsalliance.github.io/verible/
Other
1.25k stars 195 forks source link

verible-verilog-syntax crashing #2181

Closed joaovam closed 3 weeks ago

joaovam commented 1 month ago

The verible-verilog-syntax crashes when trying to analyze the following input:

     module m();
         id_0var_0 (var_3), var_1, var_2; 
     endmodule

the crash output is the following:

E0514 11:38:50.098383 2290788 tree_utils.h:92] Node: Programming error: expected kInstantiationType but got kDataType
F0514 11:38:50.098421 2290788 tree_utils.h:128] Check failed: 'MatchNodeEnumOrNull(SymbolCastToNode(symbol), node_enum)' Must be non-null
*** Check failure stack trace: ***
    @     0x60601bc9a2c9  absl::lts_20240116::log_internal::LogMessage::PrepareToDie()
    @     0x60601bc9a341  absl::lts_20240116::log_internal::LogMessage::SendToLog()
    @     0x60601bc99d93  absl::lts_20240116::log_internal::LogMessage::Flush()
    @     0x60601bc9a640  absl::lts_20240116::log_internal::LogMessageFatal::~LogMessageFatal()
    @     0x60601bc989a2  absl::lts_20240116::log_internal::DieBecauseNull()
    @     0x60601bbcda51  absl::lts_20240116::log_internal::DieIfNull<>()
    @     0x60601bbcd7d5  verible::CheckSymbolAsNode<>()
    @     0x60601bc613c5  verilog::MakeInstantiationBase<>()
    @     0x60601bc3ed8c  verilog::verilog_parse()
    @     0x60601bc2f107  verilog::verilog_parse_wrapper()
    @     0x60601bb6f711  verible::BisonParserAdapter<>::Parse()
    @     0x60601bb737d7  verible::FileAnalyzer::Parse()
    @     0x60601bb6379a  verilog::VerilogAnalyzer::Analyze()
    @     0x60601bb61ce5  verilog::VerilogAnalyzer::AnalyzeAutomaticMode()
    @     0x60601bb1c930  ParseWithLanguageMode()
    @     0x60601bb1ce7f  AnalyzeOneFile()
    @     0x60601bb1de65  main
    @     0x7b154b629d90  (unknown)
*** SIGABRT received at time=1715697530 on cpu 11 ***
PC: @     0x7b154b6969fc  (unknown)  pthread_kill
    @     0x60601bb3f17b         64  absl::lts_20240116::WriteFailureInfo()
    @     0x60601bb3f390         96  absl::lts_20240116::AbslFailureSignalHandler()
    @     0x7b154b642520  (unknown)  (unknown)
Aborted

The expected behavior would be to exit the program, identifying a syntax error near id_0 without crashing.

hzeller commented 1 month ago

Thanks for the report. I try to get to it, but am currently pretty busy with other things so can't promise anything timely. I am happy to review a PR that fixes it though.

[one-liner to reproduce]

bazel run -c dbg verilog/tools/syntax:verible-verilog-syntax -- - <<EOF
module m();
         id_0var_0 (var_3), var_1, var_2; 
endmodule
EOF
rafasumi commented 1 month ago

The problem seems to be happening in the rule for instantiation_base:

instantiation_base
  : instantiation_type non_anonymous_gate_instance_or_register_variable_list
    { $$ = MakeInstantiationBase($1, $2); }
  | reference call_base ',' gate_instance_or_register_variable_list
    {$$ = MakeInstantiationBase(ReinterpretReferenceAsDataTypePackedDimensions($1), ExtendNode($4,$3,$2)); }
  | reference_or_call_base
    {$$ = MakeTaggedNode(N::kFunctionCall,$1); }
  ;

The second case is matched due to the comma appearing after the call (id_0var_0 (var_3)). ReinterpretReferenceAsDataTypePackedDimensions will return a node with the kDataType tag, but MakeInstantiationBase expects a kInstantiationType tag.

Why is this second case of the rule needed at all? I can't think of any situation where it would be needed for valid syntax. When I tried commenting it out, all of Verible's tests have passed and verible-verilog-syntax correctly identified the syntax error in the input given by @joaovam:

-:2:27: syntax error at token ","
fangism commented 1 month ago

Just briefly looking at this, I recall that we don't have great support for anonymous instances, and this second grammar case may have been a haklf-baked attempt to support a single declaration that mixes anonymous with named instances (I'm not even sure if this is valid).

hzeller commented 1 month ago

So sounds like this faulty production might be good to comment out with a /* TODO: support mixed anonymous declarations */ with a reference to this bug why it was removed.

@rafasumi can you prepare a Pull Request ?

Might be worthwhile checking that running .github/bin/smoke-test.sh has the same non-zero exit code counts for all verible-verilog-syntax invocations.

rafasumi commented 1 month ago

@rafasumi can you prepare a Pull Request ?

Sure, I'll work on it!