Cannot parse `(a.a[a.a[a]])`

[numero-744]

This one looks fun™

https://github.com/openhwgroup/cva6/blob/028ce43fce323728012c7f0d37253994701e7372/core/issue_read_operands.sv#L494

Below is an example, as minimal as I could do.
The commented line is the one which fails.
All other lines are correctly parsed.

assign a = (a);
assign a = (a.a);
assign a = (a.a[a]);
assign a = (a.a[a.a]);
//assign a = (a.a[a.a[a]]); ERROR
assign a = a.a[a.a[a]];
assign a = (a[a.a[a]]);
assign a = (a.a[a[a]]);
assign a = (a.a[a.a]);
assign a = (a.a.a.a);
assign a = (a.a[a.a.a.a]);

Current commit: d589921 (Mon Mar 10 14:56:16 2025 +0100) Update generated parser

PS: I confirm that the previous fix, for #17, worked 💚

[numero-744]

Here is a similar issue

https://github.com/openhwgroup/cva6/blob/028ce43fce323728012c7f0d37253994701e7372/core/issue_read_operands.sv#L506

The simplified example, using the same version as in the issue description.

      //assign a = (a.a[a] == a[a::a-1:0]); ERROR
      assign a = a.a[a] == a[a::a-1:0];
      assign a = (a.a[a] == a[a-1:0]);
      assign a = (a[a] == a[a::a-1:0]);
      assign a = (a.a == a[a::a-1:0]);
      //assign a = (a.a[a] != a[a::a-1:0]); ERROR
      assign a = (a[a::a-1:0]);
      assign a = (a.a[a]);
      assign a = (a.a[a] == a[a::a:0]);
      assign a = (a.a[a] == a[a::a-1]);

[gmlarumbe]

These ones seem to be caused by dynamic precedences in the grammar. I have done some debugging that fix it in the dev branch but make other tests fail so it will probably take some time...

[numero-744]

I understand, thank you very much for your work 💚
I'll continue to look for other bugs in the meantime

[numero-744]

Hello,

Good news, few errors remain to parse CVA6 codebase 😃

I have tried the dev branch (after merging it into master), it seems it did not fix the errors yet.
I noticed that you have added CVA6 repository in this branch.
Do you plan to merge it in master?
I am not sure if it is a good idea, there is a lot of files and CVA6 repository moves a lot (hence the need for mergiraf, which will likely depend on this crate).
Maybe adding the simplified examples such as those I have provided is enough?
Else, what about adding it as a submodule instead?
Dependabot could keep it updated for you with a PR regularly, and so your CI could automatically trigger detect errors.
Also it could be optionally cloned.

Note: the two remaining error types are:

• +: with nested indexing in wt_axi_adapter. It is probably the same error as this issue so I'll wait for this issue to be fixed and I'll open another one if it does not also fix the +: issue.
• macros, I'll probably open an issue for this later because there might be interesting discussions about macros. CVA6 code also could be adapted to have a more friendly usage of macros ^^'

Thank you again!

[gmlarumbe]

Hi again!

I already merged the CVA6 files for CI to the master branch. I believe this is the best option so far since this project helped finding some new bugs and keeping an unchanging version of it will ensure that these will not show up again after new updates to the grammar. In case that new changes in CVA6 result in parsing errors these can be added as snippets to the test/files/github/ directory.

On the other hand updating the reference files can be done manually quickly with the update_tests.sh shell script. I believe this will take less time for code maintenance than the Dependabot/PR option, even though it would be great to have automation like that in the future.

There are also some updates that might have fixed other parser bugs in the CVA6 repo. Regarding the remaining ones:

+: with nested indexing in wt_axi_adapter. It is probably the same error as this issue so I'll wait for this issue to be fixed and I'll open another one if it does not also fix the +: issue.

Sounds great! Hope I can find a fix soon...

macros, I'll probably open an issue for this later because there might be interesting discussions about macros. CVA6 code also could be adapted to have a more friendly usage of macros ^^'

Macro expansion and code preprocessing seem hard to integrate in the parser. Some basic and common cases can be implemented but some of them can be very painful. You can check other threads from other tree-sitter parsers (e.g tree-sitter/tree-sitter-c#13). But feel free to open an issue with your ideas, that will always be welcome :)

[numero-744]

Sorry for the delayed answer.

Thanks for the detailed explanation about copying CVA6 files, I understand your point and I agree with you.

Thanks for the link about macros.
If I understand correctly, tree sitter does not fulfill the requirements to easily expand the macro (especially if it is defined in another file).
A weak solution would be to assume that a macro can be used at any point in the grammar, and can let the parser at any point in the grammar… The second point feels unrealistic to implement. (And we could have the same thing for the macro symbolic arguments!)

It seems you have allowed macros use in type expressions since my last check, thank you for that!
The only remaining error about macros seems to be about an "early fix" (as in "early optimization": trying to fix an error which never occurs) in CVA6 repository.
Below, I patch the file to remove this early fix while keeping the same preprocessed contents.
The patched file is handled by this crate while the non-patched file is not.

% diff core/cva6_rvfi{,_patched}.sv
339,340d338
<   `define COMMA ,
<
346c344
<   `CONNECT_RVFI_FULL(CVA6Cfg.FpPresent, fcsr, { csr.fcsr_q.frm `COMMA csr.fcsr_q.fflags})
---
>   `CONNECT_RVFI_FULL(CVA6Cfg.FpPresent, fcsr, { csr.fcsr_q.frm, csr.fcsr_q.fflags})
% (vpp() { verilator -E "$1" | grep -v '^`line' }; diff -Bwqs =(vpp core/cva6_rvfi.sv) =(vpp core/cva6_rvfi_patched.sv))                               test-tree-sitter
Files /tmp/zsh8JnaNq and /tmp/zshZ9XkJ5 are identical

Unless you have an idea about macros I would keep the status-quo and make this patch applied in CVA6 repository (and stop going off-topic in this issue 😉).

EDIT: added thanks for macro use in type expressions (sorry I my train was arriving at station)

[gmlarumbe]

If I understand correctly, tree sitter does not fulfill the requirements to easily expand the macro (especially if it is defined in another file).

Exactly. AFAIK tree-sitter cannot preprocess code and only parses it. That means that current macro support is a workaround limited to a few code patterns that happen often (e.g. ifdefs in module port definitions, see #11 or #13).

With respect to other macro usages like the one below:

`CONNECT_RVFI_FULL(CVA6Cfg.FpPresent, fcsr, { csr.fcsr_q.frm `COMMA csr.fcsr_q.fflags})

Adding support to the grammar to parse correctly this example would add a lot of complexity and seems a very specific case. On the other hand this grammar parses well almost everything but the COMMA macro:

(text_macro_usage ` (simple_identifier) (
 (list_of_actual_arguments
  (actual_argument
   (param_expression
	(mintypmax_expression
	 (expression
	  (primary
	   (hierarchical_identifier (simple_identifier) . (simple_identifier)))))))
  ,
  (actual_argument
   (param_expression
	(mintypmax_expression
	 (expression
	  (primary
	   (hierarchical_identifier (simple_identifier)))))))
  ,
  (actual_argument
   (param_expression
	(mintypmax_expression
	 (expression
	  (primary
	   (concatenation {
		(expression
		 (primary
		  (hierarchical_identifier (simple_identifier) . (simple_identifier) .
		   (ERROR (simple_identifier))
		   (text_macro_usage ` (simple_identifier))
		   (ERROR (simple_identifier))
		   . (simple_identifier) . (simple_identifier))))
		})))))))
 ))

It seems you have allowed macros use in type expressions since my last check, thank you for that!

No problem! Feel free to bring up more bugs/issues and discuss them :)

[numero-744]

Hello 👋

The `COMMA macro has been removed from cva6_rvfi.sv 🚀

For the dynamic precedence issue I may miss something but I don't really understand why it is dynamic.
According to 11.3.2 of 1800-2017 (or 7.9. Operator precedence and associativity of SystemVerilog 3.1a) the precedence is defined with a static order, isn't it?

[gmlarumbe]

The `COMMA macro has been removed from cva6_rvfi.sv

Great! There was a similar macro usage in file uvme_isa_covg.sv

  cp_rd_rs1_hazard: coverpoint instr.rvfi.rd1_addr {
    ignore_bins IGN_RS1_HAZARD_OFF = {[0:$]} `WITH (!reg_hazards_enabled);
    bins RD[] = {[0:31]} iff (instr.rvfi.rd1_addr == instr.rvfi.rs1_addr);
  }

Dynamic precedences are used in tree-sitter when there are two valid possibilities for the syntax tree but only one must be chosen. I believe this is done on a per-token basis so it does not have to do with operator precedence.

You can check tree-sitter grammar DSL for more info: https://tree-sitter.github.io/tree-sitter/creating-parsers/2-the-grammar-dsl.html#the-grammar-dsl

[numero-744]

Hello @gmlarumbe

Any news about the fix? Can I help in any way?

it does not have to do with operator precedence

Thanks for the disambiguation about dynamic precedence, I thought SystemVerilog could be parsed with only static precedence.

[gmlarumbe]

Hi @numero-744 ,

Sorry for the late reply, I have been a bit busy lately.

I tried to fix the bugs related to dynamic precedences but they seem more complex than I thought since fixing some cases makes others break.

In the future I hope I can spare some time to take a look at these in detail.

[gmlarumbe]

Hi @numero-744 ,

This issue has been fixed in #33.

I also added the following cva6 files to CI:

• tb_mem.sv
• ariane_regfile.sv
• uvme_cvxif_vseq.sv
• wt_axi_adapter.sv

There is one particular case for which I could not find the solution:

assign a = (a.a == a[a::a-1:0]);

This one has been moved to a separate issue: #34

Feel free to reopen if I missed something.