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.31k stars 199 forks source link

Failure to parse if/else macros in always block headers #228

Open imphil opened 4 years ago

imphil commented 4 years ago

STR:

$ wget https://raw.githubusercontent.com/lowRISC/ibex/master/rtl/ibex_counters.sv
$ verilog_format ibex_counters.sv 
rtl/ibex_counters.sv:65:1: syntax error, rejected "`else" (https://github.com/google/verible).
rtl/ibex_counters.sv:70:13: syntax error, rejected "else" (https://github.com/google/verible).
rtl/ibex_counters.sv:72:9: syntax error, rejected "end" (https://github.com/google/verible).
rtl/ibex_counters.sv:78:7: syntax error, rejected "end" (https://github.com/google/verible).
rtl/ibex_counters.sv:80:7: syntax error, rejected "end" (https://github.com/google/verible).
rtl/ibex_counters.sv:83:5: syntax error, rejected "end" (https://github.com/google/verible).

Failing code:

`ifdef FPGA_XILINX
      // DSP output register requires synchronous reset.
      always @(posedge clk_i) begin
`else
      always @(posedge clk_i or negedge rst_ni) begin
`endif

https://github.com/lowRISC/ibex/blob/master/rtl/ibex_counters.sv

fangism commented 4 years ago

This will unlikely be supported in Verible any time soon, at least not without a proper preprocessor. What you have is preprocessor conditionals that do not cleanly straddle boundaries of a whole language construct -- that is the current limitation/restriction of parsing unpreprocessed code. In your example, the break around the start of an always construct.

fangism commented 4 years ago

Possible workaround (not great):

`ifdef FPGA_XILINX
`define CONDITION posedge clk_i
`else
`define CONDITION posedge clk_i or negedge rst_ni
`endif
      always @(`CONDITION) begin
...
      end
imphil commented 4 years ago

Thanks @fangism. I agree that we can find solutions for this particular case, but there will be cases where these workarounds don't become viable. Especially for verilog_format, where using another pre-processor in front of Verible isn't an option, we're out of luck.

So I'd encourage you to look into ways to make the pre-processing step work. (It would be interesting to see how e.g. clang-format handles that, e.g. in the light of invalid syntax inside define-gated regions.)

imphil commented 4 years ago

Another thing: If I go with the proposed solution, I get the following info message:

I verilog/preprocessor/verilog_preprocess.cc:203] Re-defining macro CONDITION
I verilog/preprocessor/verilog_preprocess.cc:203] Re-defining macro CONDITION
fangism commented 4 years ago

Another thing: If I go with the proposed solution, I get the following info message:

I verilog/preprocessor/verilog_preprocess.cc:203] Re-defining macro CONDITION
I verilog/preprocessor/verilog_preprocess.cc:203] Re-defining macro CONDITION

But it doesn't cause a non-zero exit status, right? This probably should've been turned into a quieter log message.

fangism commented 4 years ago

Thanks @fangism. I agree that we can find solutions for this particular case, but there will be cases where these workarounds don't become viable. Especially for verilog_format, where using another pre-processor in front of Verible isn't an option, we're out of luck.

So I'd encourage you to look into ways to make the pre-processing step work. (It would be interesting to see how e.g. clang-format handles that, e.g. in the light of invalid syntax inside define-gated regions.)

clang-format implements a completely separate hand-written pseudo-C++-parser that is separate from the proper parser used by the compiler toolchain. In essence, it is formatting only based on lexing (with limited sloppy parsing, and ability to work with C-like languages in general), and not true syntactic structure (which is why it handles a lot of loosely incorrect code). Yet, C++ is a much syntactically simpler language than SystemVerilog. I simply do not have the resources to develop/maintain a hand-written parser or pseudo-parser, over maintaining a generator-based lexer/parser -- not today. The current syntax-tree based strategy is what allowed the formatter to get this far with as little developer resources as it has received. Should someone contribute a lexer-based front-end that is convertible to an internal representation that the formatter uses, most of Verible's (language-agnostic) formatting engine could be reused.

I also have some ideas around "composite" formatting: splitting or versioning different views of text, formatting separately, and then stitching them back together, but that is very low priority right now, compared to other formatter issues.