Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

verible-verilog-syntax crashing #2181

Closed
joaovam opened this issue May 14, 2024 · 5 comments
Closed

verible-verilog-syntax crashing #2181

joaovam opened this issue May 14, 2024 · 5 comments
Labels
bug Something isn't working rejects-valid syntax If the parser wrongly rejects syntactically valid code (according to SV-2017).

Comments

@joaovam
Copy link

joaovam commented May 14, 2024

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.

@joaovam joaovam added the rejects-valid syntax If the parser wrongly rejects syntactically valid code (according to SV-2017). label May 14, 2024
@hzeller
Copy link
Collaborator

hzeller commented May 14, 2024

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

@fangism fangism added the bug Something isn't working label May 14, 2024
@rafasumi
Copy link
Contributor

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
Copy link
Collaborator

fangism commented May 31, 2024

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
Copy link
Collaborator

hzeller commented May 31, 2024

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
Copy link
Contributor

@rafasumi can you prepare a Pull Request ?

Sure, I'll work on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rejects-valid syntax If the parser wrongly rejects syntactically valid code (according to SV-2017).
Projects
None yet
Development

No branches or pull requests

4 participants