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

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

Open
imphil opened this issue Mar 11, 2020 · 6 comments
Open

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

imphil opened this issue Mar 11, 2020 · 6 comments
Labels
preprocessor anything related to preprocessing (conditionals, macros, etc.)

Comments

@imphil
Copy link
Contributor

imphil commented Mar 11, 2020

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

fangism commented Mar 11, 2020

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 fangism added the preprocessor anything related to preprocessing (conditionals, macros, etc.) label Mar 11, 2020
@fangism
Copy link
Collaborator

fangism commented Mar 11, 2020

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

imphil commented Mar 12, 2020

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

imphil commented Mar 12, 2020

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

imphil added a commit to imphil/ibex that referenced this issue Mar 12, 2020
Verible doesn't do real pre-processing currently, and fails to parse
code if define sections span across headers of blocks, as we did.

Use another syntax instead for the one case where we did that to work
around this limitation. The code isn't less readable as result, making
this an acceptable trade-off.

Works around chipsalliance/verible#228
@fangism
Copy link
Collaborator

fangism commented Mar 12, 2020

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

fangism commented Mar 12, 2020

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.

imphil added a commit to lowRISC/ibex that referenced this issue Mar 13, 2020
Verible doesn't do real pre-processing currently, and fails to parse
code if define sections span across headers of blocks, as we did.

Use another syntax instead for the one case where we did that to work
around this limitation. The code isn't less readable as result, making
this an acceptable trade-off.

Works around chipsalliance/verible#228
nikhiljha pushed a commit to nikhiljha/verible that referenced this issue Sep 27, 2022
kbieganski added a commit to antmicro/verible that referenced this issue Apr 28, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress. Hoping to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue Apr 28, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue May 23, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue May 23, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue May 23, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
kbieganski added a commit to antmicro/verible that referenced this issue May 24, 2023
Currently, the formatter doesn't handle many scenarios involving preprocessor
`ifdef`s/`endif`s interleaved with `begin`s, module headers, etc. (chipsalliance#228, chipsalliance#241, chipsalliance#267)
This patch attempts to solve this problem by performing multiple passes of the
formatting on preprocessed variants of the source. Each of these variants has a
different set of preprocessor branches enabled. Together, they should cover the
entire source (though that doesn't work in all cases yet). After several
formatting passes for different variants of the AST, a correct and properly
formatted file is produced.

This is still work in progress, so not everything works, and the code isn't very
clean. I'd love to get some early feedback on this.

Signed-off-by: Krzysztof Bieganski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preprocessor anything related to preprocessing (conditionals, macros, etc.)
Projects
None yet
Development

No branches or pull requests

2 participants