verilator/5.036.bcr.1 #6042

will-keen · 2025-09-29T18:07:54Z

add verilated_random.h and verilated_saif.h to srcs to pick up released functionality
patch such that hierarchical verilation invokes the verilator binary built by bazel

@bazel-io skip_check unstable_url

Diff vs 5.036:

diff --git a/5.036/MODULE.bazel b/5.036.bcr.1/MODULE.bazel
index d017b25c..254c4c69 100644
--- a/5.036/MODULE.bazel
+++ b/5.036.bcr.1/MODULE.bazel
@@ -2,7 +2,7 @@
 
 module(
     name = "verilator",
-    version = "5.036",
+    version = "5.036.bcr.1",
     bazel_compatibility = [">=7.2.1"],
 )
 
diff --git a/5.036/overlay/BUILD.bazel b/5.036.bcr.1/overlay/BUILD.bazel
index b7687177..b2e5c735 100644
--- a/5.036/overlay/BUILD.bazel
+++ b/5.036.bcr.1/overlay/BUILD.bazel
@@ -261,6 +261,8 @@ cc_library(
         "include/verilated.cpp",
         "include/verilated_fst_c.cpp",
         "include/verilated_imp.h",
+        "include/verilated_random.cpp",
+        "include/verilated_saif_c.cpp",
         "include/verilated_syms.h",
         "include/verilated_threads.cpp",
         "include/verilated_vcd_c.cpp",
@@ -272,8 +274,10 @@ cc_library(
         "include/verilated_fst_c.h",
         "include/verilated_funcs.h",
         "include/verilated_intrinsics.h",
+        "include/verilated_random.h",
         "include/verilated_sc.h",
         "include/verilated_sym_props.h",
+        "include/verilated_saif_c.h",
         "include/verilated_threads.h",
         "include/verilated_timing.h",
         # Needed for verilated_vcd_c.cpp and verilated_fst_c.cpp
diff --git a/5.036.bcr.1/patches/hierarchical_binary_name.patch b/5.036.bcr.1/patches/hierarchical_binary_name.patch
new file mode 100644
index 00000000..028807c9
--- /dev/null
+++ b/5.036.bcr.1/patches/hierarchical_binary_name.patch
@@ -0,0 +1,11 @@
+--- a/src/V3EmitMk.cpp
++++ b/src/V3EmitMk.cpp
+@@ -814,7 +814,7 @@ class EmitMkHierVerilation final {
+         of.puts("VM_HIER_RUN_DIR := " + cwd + "\n");
+         of.puts("# Common options for hierarchical blocks\n");
+         const string fullpath_bin = V3Os::filenameRealPath(v3Global.opt.buildDepBin());
+-        const string verilator_wrapper = V3Os::filenameDir(fullpath_bin) + "/verilator";
++        const string verilator_wrapper = V3Os::filenameDir(fullpath_bin) + "/verilator_bin";
+         of.puts("VM_HIER_VERILATOR := " + verilator_wrapper + "\n");
+         of.puts("VM_HIER_INPUT_FILES := \\\n");
+         const V3StringList& vFiles = v3Global.opt.vFiles();
diff --git a/5.036/source.json b/5.036.bcr.1/source.json
index 6f14fe0e..245ce475 100644
--- a/5.036/source.json
+++ b/5.036.bcr.1/source.json
@@ -2,10 +2,11 @@
     "url": "https://github.com/verilator/verilator/archive/refs/tags/v5.036.tar.gz",
     "integrity": "sha256-QZmWSILVbPahnOgMail+vjsMNeqBEGzU9yI0JZQzfEc=",
     "strip_prefix": "verilator-5.036",
+    "patch_strip": 1,
     "overlay": {
         ".bazelignore": "sha256-4Yz6+7DIt5CedRfM7s3dxN7HstNIP9KBMBXro1MaVu0=",
-        "BUILD.bazel": "sha256-V8wEJDlOhjHfr7nU8jsYy1Qy9TDS37t2yNJ9cfFdrL8=",
-        "MODULE.bazel": "sha256-ofcRFsBsEQUDwfBNVr4LcNSUanqflHvny054WoONV6s=",
+        "BUILD.bazel": "sha256-X5ndeNeEM67en2cZ/BSnLmrPyS+aQ2C/1gRhoP6qRRw=",
+        "MODULE.bazel": "sha256-wJNW524laImuehijpMgRFbD6EgWSuzlPDGMLnz2PCqk=",
         "private/BUILD.bazel": "sha256-um1vB3qYV9jNLMhTue172PCrJbZf/4Nc9gFiwpXqWYY=",
         "private/verilator_astgen.py": "sha256-aBHwsUzSKEEEseMUDdXS2XTT8X0jHfAJw7Yu8qvKqPA=",
         "private/verilator_bisonpre.py": "sha256-G92zsCtKeJSK2WtjbM47gwwKY19i5QolU6eKp6yUnr0=",
@@ -13,5 +14,8 @@
         "private/verilator_flexfix.py": "sha256-eMDxDSMiytb//P2Ej8KfCTwsblf+/WEsGa1gk2GkfK8=",
         "private/verilator_utils.bzl": "sha256-0nDLkhOTQfvlyjg+B57KXhru4e2unIBOr87YcCbCzq8=",
         "private/verilator_version.py": "sha256-GOQW7JAmkikFKdk7KzNQOiAt8Wk2WZvpC6V2UdLV36U="
+    },
+    "patches": {
+        "hierarchical_binary_name.patch": "sha256-kDgIFSCn1Gr6ssXr9vbcFR8ZUF10nq/oaq/JOihGynk="
     }
 }

@bazel-io

- add verilated_random.h and verilated_saif.h to srcs to pick up released functionality - patch such that hierarchical verilation invokes the verilator binary built by bazel @bazel-io skip_check unstable_url

google-cla · 2025-09-29T18:08:01Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

bazel-io · 2025-09-29T18:08:13Z

Hello @UebelAndre, modules you maintain (verilator) have been updated in this PR.
Please review the changes. You can view a diff against the previous version in the "Generate module diff" check.

gemini-code-assist

Code Review

This pull request introduces version 5.036.bcr.1 for the verilator module. This is a patch release that addresses an issue with hierarchical Verilation by correctly referencing the Bazel-built binary. Additionally, it incorporates verilated_random.h and verilated_saif.h to expose more of Verilator's functionality. The changes, including the new patch, updated overlay files, and presubmit configuration, are well-structured and adhere to the Bazel Central Registry guidelines. The implementation appears solid, and I did not find any issues of high or critical severity.

will-keen · 2025-09-29T18:14:42Z

@bazel-io skip_check unstable_url

(Verilator has no releases available on github, only tags)

will-keen · 2025-09-29T18:32:05Z

@bazel-io skip_check unstable_url

will-keen · 2025-09-30T16:04:46Z

Hi @UebelAndre, all the status checks are green now, please would you mind reviewing when you are able? Thanks!

UebelAndre

Thanks! This seems reasonable but do you think this should be updated to use runfiles instead? I think trying to match the binary name (which is only different because of an arbitrary choice to not conflict with the library name) seems kinda brittle. Is there some kind of test that can be added here?

will-keen · 2025-10-03T10:29:28Z

Thanks! This seems reasonable but do you think this should be updated to use runfiles instead? I think trying to match the binary name (which is only different because of an arbitrary choice to not conflict with the library name) seems kinda brittle. Is there some kind of test that can be added here?

Thanks, that's good feedback.

I've added a test, which passes with the current solution, but will try to find a cleaner solution for the path to the binary.

will-keen · 2025-10-08T13:16:53Z

@UebelAndre I believe we've addressed the feedback, please could you take another look?

UebelAndre

I'm confused why the patch is necessary. There's https://github.com/periareon/rules_verilog which has rules which is able to successfully run verilator without this change. What specifically is hierarchical verilation?

will-keen · 2025-10-08T17:13:19Z

I'm confused why the patch is necessary. There's https://github.com/periareon/rules_verilog which has rules which is able to successfully run verilator without this change. What specifically is hierarchical verilation?

Happy to explain - it's a mode where Verilator invokes itself recursively on smaller RTL modules. Full details are available here: https://verilator.org/guide/latest/verilating.html#hierarchical-verilation

We use bazel_rules_hdl rather than rules_verilog - https://github.com/hdl/bazel_rules_hdl.

As it stands, if you pass Verilator the --hierarchical flag with bazel_rules_hdl, it fails, because it looks for its own binary with the name verilator under $VERILATOR_HOME. The patch in this PR is to allow Verilator to find its own executable in the bazel runfiles. Verilator is creating makefiles dynamically with the path to the binary in the patched file.

The original patch I created worked just by looking for verilator_bin rather than verilator as the executable name. The new patch uses runfiles as you suggested, which is a bit more invasive but more robust.

The test I added demonstrates Verilator working for a toy design with --hierarchical passed in. Without the patch it demonstrates the original failure.

I hope that clarifies, please let me know if you need further info.

UebelAndre · 2025-10-08T17:13:26Z

I'm also so sorry, I opened #6136 just to see if it passed CI but I forgot there's no need to click approve and it merged. I was not planning to merge that at all until this change was resolved... In return I will be more responsive here.

UebelAndre · 2025-10-08T17:21:37Z

I'm confused why the patch is necessary. There's https://github.com/periareon/rules_verilog which has rules which is able to successfully run verilator without this change. What specifically is hierarchical verilation?

Happy to explain - it's a mode where Verilator invokes itself recursively on smaller RTL modules. Full details are available here: https://verilator.org/guide/latest/verilating.html#hierarchical-verilation

We use bazel_rules_hdl rather than rules_verilog - https://github.com/hdl/bazel_rules_hdl.

As it stands, if you pass Verilator the --hierarchical flag with bazel_rules_hdl, it fails, because it looks for its own binary with the name verilator under $VERILATOR_HOME. The patch in this PR is to allow Verilator to find its own executable in the bazel runfiles. Verilator is creating makefiles dynamically with the path to the binary in the patched file.

The original patch I created worked just by looking for verilator_bin rather than verilator as the executable name. The new patch uses runfiles as you suggested, which is a bit more invasive but more robust.

The test I added demonstrates Verilator working for a toy design with --hierarchical passed in. Without the patch it demonstrates the original failure.

I hope that clarifies, please let me know if you need further info.

Ah, I see now, thank you! In that case I think there should be a way to completely override the path to verilator and then then verilator_bin should probably be renamed to bin/verilator and an alias should be made (verilator_bin) so that all the naming aligns.

UebelAndre · 2025-10-08T17:25:46Z

@will-keen also, that code you're patching, is that getting vendored into a makefile? That's then subsequently run in the same action? If it persists outside the function then using runfiles here wouldn't be what you want since that's gonna have absolute paths to sandbox locations that will likely be deleted, right? I think what you'd want is just execpath always if so.

UebelAndre · 2025-10-09T16:16:33Z

modules/verilator/5.036.bcr.1/overlay/BUILD.bazel


 cc_library(
-    name = "verilator",
+    name = "verilator_lib",


This should stay verilator so the library name is libverilator, Otherwise this becomes libverilator_lib or liblibverilator.

UebelAndre · 2025-10-09T16:17:03Z

modules/verilator/5.036.bcr.1/overlay/BUILD.bazel


 cc_binary(
-    name = "verilator_bin",
+    name = "verilator",


You should be able to change this to bin/verilator and it'll generate a binary named verilator just in a bin directory.

You can then make a verilator_bin alias for backward compatibility.

I made this change in #6162

OK thanks! We'll open a new PR to add the verilated_saif.h, verilated_random.h and the test for hierarchical verilation.

will-keen · 2025-10-10T11:25:53Z

Thanks for your help @UebelAndre - the bin/verilator change seems to work for us.

I'll close this PR and @kbraval will create a new one for 5.036.bcr.3 on top of your .bcr.2 - we'd still like to add the extra header files and the test for hierarchical verilation, if that's OK?

I also had a 5.040 cooking which I'll come back to soonish

UebelAndre · 2025-10-10T16:30:35Z

Thanks for your help @UebelAndre - the bin/verilator change seems to work for us.

I'll close this PR and @kbraval will create a new one for 5.036.bcr.3 on top of your .bcr.2 - we'd still like to add the extra header files and the test for hierarchical verilation, if that's OK?

I also had a 5.040 cooking which I'll come back to soonish

Yeah, fixing headers and adding testing sounds great! Sorry again for conflicting with this change and making things unnecessarily difficult 😅

verilator/5.036.bcr.1

b555ef9

- add verilated_random.h and verilated_saif.h to srcs to pick up released functionality - patch such that hierarchical verilation invokes the verilator binary built by bazel @bazel-io skip_check unstable_url

gemini-code-assist bot reviewed Sep 29, 2025

View reviewed changes

bazel-io added the skip-url-stability-check Skip the URL stability check for the PR label Sep 29, 2025

UebelAndre suggested changes Sep 30, 2025

View reviewed changes

Add test for hierarchical verilation

78c0ed8

remove erroneously added test file

b2091bc

Wyverald added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label Oct 3, 2025

Use runfiles to locate verilator binary

205c254

Wyverald requested a review from UebelAndre October 7, 2025 22:18

UebelAndre suggested changes Oct 8, 2025

View reviewed changes

Swap verilator_bin and verilator labels

1753990

UebelAndre suggested changes Oct 9, 2025

View reviewed changes

will-keen closed this Oct 10, 2025

verilator/5.036.bcr.1 #6042

verilator/5.036.bcr.1 #6042

Uh oh!

Conversation

will-keen commented Sep 29, 2025

Uh oh!

google-cla bot commented Sep 29, 2025

Uh oh!

bazel-io commented Sep 29, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

will-keen commented Sep 29, 2025

Uh oh!

will-keen commented Sep 29, 2025

Uh oh!

will-keen commented Sep 30, 2025

Uh oh!

UebelAndre left a comment

Choose a reason for hiding this comment

Uh oh!

will-keen commented Oct 3, 2025

Uh oh!

will-keen commented Oct 8, 2025

Uh oh!

UebelAndre left a comment

Choose a reason for hiding this comment

Uh oh!

will-keen commented Oct 8, 2025

Uh oh!

UebelAndre commented Oct 8, 2025

Uh oh!

UebelAndre commented Oct 8, 2025

Uh oh!

UebelAndre commented Oct 8, 2025

Uh oh!

UebelAndre Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

UebelAndre Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

UebelAndre Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

will-keen Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

will-keen commented Oct 10, 2025

Uh oh!

UebelAndre commented Oct 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants