lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.49k stars 742 forks source link

[lint] otp_ctrl is (sort of) failing verible lint #5027

Open rswarbrick opened 3 years ago

rswarbrick commented 3 years ago

If you look at the generated lint.log, you'll find that Verible is choking on a syntax error. This is currently being ignored by the parse-lint-report.py (another bug, addressed by PR #5026).

The syntax errors look like this:

INFO: Running
I verilog/preprocessor/verilog_preprocess.cc:203] Re-defining macro REGWEN_PATH
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:73:9: syntax error, rejected "first_match" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:75:3: syntax error, rejected "endproperty" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:80:35: syntax error, rejected ";" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:84:3: syntax error, rejected "endproperty" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:89:35: syntax error, rejected ";" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:93:3: syntax error, rejected "endproperty" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:97:35: syntax error, rejected ";" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:101:3: syntax error, rejected "endproperty" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:108:35: syntax error, rejected ";" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:112:3: syntax error, rejected "endproperty" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:116:35: syntax error, rejected ";" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:120:3: syntax error, rejected "endproperty" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:123:35: syntax error, rejected ";" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:127:3: syntax error, rejected "endproperty" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:131:35: syntax error, rejected ";" (https://github.com/google/verible).
../src/lowrisc_fpv_otp_ctrl_csr_assert_0/otp_ctrl_csr_assert_fpv.sv:135:3: syntax error, rejected "endproperty" (https://github.com/google/verible).

I guess that Verible just doesn't support this sort of SVA yet. Is the problem just that we should be excluding the auto-generated CSR assertion files from the file list?

Note that this bug blocks merging #5026, because fixing parse-lint-report.py will (correctly) cause us to fail CI.

@msfschaffner: Since you know about both OTP and linting, I'm hoping you'll have a clever idea! :-)

rswarbrick commented 3 years ago

Oh! Looking at #5026 again, it's not just otp_ctrl. But I think the errors are essentially the same across the board.

msfschaffner commented 3 years ago

Yeah I just stumbled over this 2 days ago as well when we removed the unnecessary waivers in https://github.com/lowRISC/opentitan/commit/9ce4bd9eba8ee99a301ce52f136ca70aa06ccf93 It looks like that regex pattern masked some of the errors that occurred during Verible runs. For this extra syntax error, I had provisioned https://github.com/lowRISC/opentitan/commit/268692c5b6afcf51d5146be6c024aa4760ede4a6 but I backed both these commits out again since I thought they may have led to the flaky CI issues we where seeing.

I've opened https://github.com/google/verible/issues/652 since this needs to be fixed upstream.

Unfortunately it's not possible to waive syntax errors easily: https://github.com/google/verible/tree/master/verilog/tools/lint#syntax-errors.

In the mean time, we could (in order of my preference ;)):

  1. fix the DV code to use a different syntax (@cindychip can you check whether that would be possible?)
  2. tweak the DV core files to not include the offending file for Verible runs
  3. detect and waive this particular condition in the parser script
  4. disable DV linting in PR CI (it kills all lint targets anyway)
  5. let the status quo be until this issue has been resolved upstream (may take a while)

WDYT?

cindychip commented 3 years ago

Thanks Rupert for investigating the issue. And thanks Michael for the alternatives. For 1). It is possible to not use keyword property, but the sequence will be quite messy. I would prefer 2) if it is not too hard to implement. Otherwise I am happy to try to implement 1) or disattach the CSR FPV check for DVs until it is fixed (Since we have the CSR DV tests, i do not think FPV check is very important)

msfschaffner commented 3 years ago

Thanks for checking @cindychip. I have added a workaround to #5026 that implements solution 2. above. PTAL and let me know what you think.

msfschaffner commented 2 years ago

Since Verible has unergone many improvements over the last year, we may want to try again whether it now recognizes these keywords properly.