dynamic_cast failure on [create-object-name-match] rule

[danghai]

Hi, I see verible supports the project: https://github.com/google/riscv-dv However, I got error when I run the script: verilog_style/run.sh.

The script:

find src/ -type f \( -name "*.sv" -o -name "*.svh" \) \
  | grep -vFf ./verilog_style/exclude_filelist.f \
  | xargs /tools/verible/verilog_lint --rules=-macro-name-style

The error message:

verilog_lint: ./common/util/casts.h:30: To verible::down_cast(From*) [with To = const verible::SyntaxTreeLeaf*; From = verible::Symbol]: Assertion `f == nullptr || dynamic_cast<To>(f) != nullptr' failed.
xargs: /tools/verible/verilog_lint: terminated by signal 6

Could you help me take the look? Thank you

[fangism]

Can you narrow down and share the exact test case?

[fangism]

This file is sync'd to the verible code base about one month old -- might be time to consider updating. https://github.com/google/riscv-dv/blob/master/verilog_style/build-verible.sh

I scanned verilog_lint on the risc-dv codebase using a build within the last few days, and don't see that error.

[danghai]

Hi @fangism , thanks response. I try to pick the newest commit from verible, but I still got the same error:

From https://github.com/google/verible
 * branch            master     -> FETCH_HEAD
Already up to date.
Note: checking out 'a6b4ab6d12a414c7fc71f389e463c3109848956a'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

The output log error:

Executed 0 out of 410 tests: 410 tests pass.
INFO: Build completed successfully, 1 total action
done
danghai@ubuntu:~/hhoangda/riscv-dv$ ./verilog_style/run.sh 
verilog_lint: ./common/util/casts.h:30: To verible::down_cast(From*) [with To = const verible::SyntaxTreeLeaf*; From = verible::Symbol]: Assertion `f == nullptr || dynamic_cast<To>(f) != nullptr' failed.
xargs: /tools/verible/verilog_lint: terminated by signal 6

What is your output? Thank you

[fangism]

I modified the xargs command with -t to see what file failed.

...
/tools/verible/verilog_lint '--rules=-macro-name-style' src/riscv_asm_program_gen.sv
verilog_lint: ./common/util/casts.h:30: To verible::down_cast(From*) [with To = const verible::SyntaxTreeLeaf*; From = verible::Symbol]: Assertion `f == nullptr || dynamic_cast<To>(f) != nullptr' failed.
Aborted

Let me work on reducing it.

[fangism]

That was doing a fresh build with risc-dv's script. When I run my own build:

$ verilog_lint --rules=-macro-name-style src/riscv_asm_program_gen.sv
src/riscv_asm_program_gen.sv:95:101: Line length exceeds max: 100; is: 101 go/verilog-style#line-length [line-length]

Something must be sufficiently different with our builds.

[fangism]

Reduced test case, using: /tools/verible/verilog_lint --ruleset=none --rules=create-object-name-match issue-222.sv

class c;
  virtual function void g();
    umode_program = s::type_id::create(l("umode_program"));
  endfunction
endclass

Again this only occurs with the riscv-dv built verilog_lint, and not my local binary.

[fangism]

b/150615846

[danghai]

What do you mean b/150615846?

[fangism]

b/150615846 is just an internal reference number for tracking (only developers might need to use it)

[tgorochowik]

I opened https://github.com/google/riscv-dv/pull/522 which seems to fix the issue for me. To be honest I am not fully sure what is the difference between those two directories.

I am able to reproduce the issue, even locally, but only when using old gcc and the same build command as you use in your repository (build --cxxopt='-std=c++17' //...).

When I'm using a modern gcc (9), both binaries work well.

[tgorochowik]

With further investigation I've noticed, that the rule does not even trigger, even if the name is changed. The l() call here is causing the issue.

That's because of the tree differences:

Node (tag: kParenGroup) {
  (#'(' @67-68: "(")
  Node (tag: kArgumentList) {
    Node (tag: kExpression) {
      (#TK_StringLiteral @68-84: ""umode_program"")
    }
  }
  (#')' @84-85: ")")
}

Without the l(..) vs

Node (tag: kParenGroup) {
  (#'(' @67-68: "(")
  Node (tag: kArgumentList) {
    Node (tag: kExpression) {
      Node (tag: kReferenceCallBase) {
        Node (tag: kFunctionCall) {
          Node (tag: kLocalRoot) {
            Node (tag: kUnqualifiedId) {
              (#SymbolIdentifier @68-69: "l")
            }
          }
          Node (tag: kParenGroup) {
            (#'(' @69-70: "(")
            Node (tag: kArgumentList) {
              Node (tag: kExpression) {
                (#TK_StringLiteral @70-86: ""umode_program"")
              }
            }
            (#')' @86-87: ")")
          }
        }
      }
    }
  }
  (#')' @87-88: ")")
}

With the l(..)

So in addition to the binaries being different in those two output directories, the check itself is not handling all cases properly, I'll prepare a patch (but it won't really fix the segfault).

[tgorochowik]

@fangism should this case even be verified by this linter rule?

Assuming we have

umode_program = s::type_id::create(l("umode_program"));

The l(...) is kind of arbitrary, and could potentially have more arguments, we don't really know what it returns or is mapped to if it's a macro. I see two options here:

• ignore such cases
• completely disallow calling create without a direct string literal inside (could be a separate rule?)

And I was wrong above - both approaches should indeed fix the assert and thus the crash.

[fangism]

@fangism should this case even be verified by this linter rule?

Assuming we have

umode_program = s::type_id::create(l("umode_program"));

The l(...) is kind of arbitrary, and could potentially have more arguments, we don't really know what it returns or is mapped to if it's a macro. I see two options here:

• ignore such cases
• completely disallow calling create without a direct string literal inside (could be a separate rule?)

And I was wrong above - both approaches should indeed fix the assert and thus the crash.

Analysis needs to be conservative, so if you can't easily deduce a violation, it's ok to let it pass.

[tgorochowik]

Ok, I finally managed to find the specific thing that caused the assert to fail/or pass.

It is the -c opt flag. It is kind of obvious when you know what it means :man_facepalming:

--compilation_mode (fastbuild|opt|dbg) (-c) This option takes an argument of fastbuild, dbg or opt, and affects various C/C++ code-generation options, such as the level of optimization and the completeness of debug tables. Bazel uses a different output directory for each different compilation mode, so you can switch between modes without needing to do a full rebuild every time.

• fastbuild means build as fast as possible: generate minimal debugging information (-gmlt -Wl,-S), and don't optimize. This is the default. Note: -DNDEBUG will not be set.
• dbg means build with debugging enabled (-g), so that you can use gdb (or another debugger).
• opt means build with optimization enabled and with assert() calls disabled (-O2 -DNDEBUG). Debugging information will not be generated in opt mode unless you also pass --copt -g.

So it has nothing to do with the compiler version. It is all because the recommended compilation command in the README disables the asserts.