[bazel] Change clang-tidy rules into test rules #18763

dmcardle · 2023-05-30T14:35:08Z

This enables targets instantiated by the clang-tidy rules to depend on test targets.

This enables targets instantiated by the clang-tidy rules to depend on test targets. Issue lowRISC#18726 Signed-off-by: Dan McArdle <[email protected]>

dmcardle · 2023-05-30T15:00:33Z

I'll also add bazel build --nobuild //... to CI, per @a-will's comment #18726 (comment).

pamaury · 2023-05-30T15:05:27Z

That looks great.
I just have a quick question: running clang-tidy on a file is really like a test in some sense. With the current code (and your fix), the test itself is a dummy and the output of clang-tidy is an artifact. Is there a reason this method was chosen compared to the test actually running clang-tidy and therefore the output being recorded like a normal test output? Is this so that clang-tidy runs on build instead of test?

Signed-off-by: Dan McArdle <[email protected]>

dmcardle · 2023-05-30T17:34:54Z

That looks great. I just have a quick question: running clang-tidy on a file is really like a test in some sense. With the current code (and your fix), the test itself is a dummy and the output of clang-tidy is an artifact. Is there a reason this method was chosen compared to the test actually running clang-tidy and therefore the output being recorded like a normal test output? Is this so that clang-tidy runs on build instead of test?

Thanks!

Running clang-tidy as a test instead of a build step would add a small amount of complexity, which I just wanted to avoid in this PR. I think we'd need to accumulate each build command into a shell script rather than immediately dispatching to ctx.actions.run.

I've been thinking of the Bazel aspect as a shadow of the build graph, so I just wanted to mirror the C compilation actions with clang-tidy actions. (Now that I'm making these rules into test rules, maybe I should reevaluate this position that clang-tidy is more like a build step than a test.)

I chose not to repurpose the *.clang-tidy.out artifacts because I'm using them in #18719. In that PR, I basically create a bunch of clang-tidy report objects at build time, then I merge all those sub-reports to create the complete report.

pamaury · 2023-05-30T21:14:17Z

I see, this makes a lot of sense now, it's probably simpler this way.

dbeitel-opentitan · 2023-05-31T14:47:25Z

I'm testing it now. Appears to be fixed.

dbeitel-opentitan · 2023-05-31T16:19:07Z

I spoke too soon. I see this failure now:
source /tools/Xilinx/Vivado/2022.2/scripts/updatemem/main.tcl -notrace
Command: update_mem -meminfo external/bitstreams/cache/f8b3c19a27f0559c091c4a41ce8efd50731f8bef/otp.mmi -data bazel-out/k8-fastbuild/bin/hw/bitstream/gcp_spliced_rom_otp_test_locked1.update.mem -proc dummy -bit bazel-out/k8-fastbuild/bin/hw/bitstream/gcp_spliced_rom_otp_test_locked1.tmpsrc.bit -out bazel-out/k8-fastbuild/bin/hw/bitstream/gcp_spliced_rom_otp_test_locked1.spliced.bit -force
Loading bitfile bazel-out/k8-fastbuild/bin/hw/bitstream/gcp_spliced_rom_otp_test_locked1.tmpsrc.bit
Loading data files...
Updating memory content...
Creating bitstream...
Writing bitstream bazel-out/k8-fastbuild/bin/hw/bitstream/gcp_spliced_rom_otp_test_locked1.spliced.bit...
0 Infos, 0 Warnings, 0 Critical Warnings and 0 Errors encountered.
update_mem completed successfully
update_mem: Time (s): cpu = 00:00:24 ; elapsed = 00:00:26 . Memory (MB): peak = 997.262 ; gain = 514.625 ; free physical = 174 ; free virtual = 9496
INFO: [Common 17-206] Exiting updatemem at Wed May 31 08:56:16 2023...
INFO: From UpdateUsrAccessValue hw/bitstream/gcp_spliced_rom_otp_test_locked1.bit:
[2023-05-31T15:56:16Z INFO opentitanlib::util::usr_access] Bitstream file old USR_ACCESS value: 0x5785f455
[2023-05-31T15:56:16Z INFO opentitanlib::util::usr_access] Bitstream file new USR_ACCESS value: 0x0
[2023-05-31T15:56:16Z INFO opentitanlib::util::usr_access] Replaced WRITE_CRC_REG command at 0xf23f68 with NOOP
[2023-05-31T15:56:16Z INFO opentitanlib::util::usr_access] Replaced WRITE_CRC_REG command at 0xf24140 with NOOP
[2023-05-31T15:56:16Z INFO opentitanlib::util::usr_access] Bitstream file old USR_ACCESS value: 0x0
[2023-05-31T15:56:16Z INFO opentitanlib::util::usr_access] Bitstream file new USR_ACCESS value: 0x7577ba7a
[2023-05-31T15:56:16Z INFO opentitantool] Command result: success.
INFO: From Running clang tidy on sw/device/silicon_creator/rom/boot_policy.c:
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sw/device/silicon_creator/rom/boot_policy.c"
No compilation database found in /home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20506/execroot/lowrisc_opentitan/sw/device/silicon_creator/rom or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
INFO: From Running clang tidy on sw/device/silicon_creator/rom/rom_epmp.c:
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sw/device/silicon_creator/rom/rom_epmp.c"
No compilation database found in /home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20505/execroot/lowrisc_opentitan/sw/device/silicon_creator/rom or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
5 warnings generated.
./sw/device/lib/base/memory.h:88:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
__builtin_memcpy(&val, ptr, sizeof(uint32_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:88:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
__builtin_memcpy(&val, ptr, sizeof(uint32_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:120:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
__builtin_memcpy(&val, ptr, sizeof(uint64_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:120:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
__builtin_memcpy(&val, ptr, sizeof(uint64_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:146:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
__builtin_memcpy(ptr, &value, sizeof(uint32_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:146:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
__builtin_memcpy(ptr, &value, sizeof(uint32_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:172:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
__builtin_memcpy(ptr, &value, sizeof(uint64_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:172:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
__builtin_memcpy(ptr, &value, sizeof(uint64_t));
^~~~~~~~~~~~~~~~
/home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20505/execroot/lowrisc_opentitan/sw/device/silicon_creator/rom/rom_epmp.c:89:3: warning: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
memset(&epmp_state, 0, sizeof(epmp_state));
^~~~~~
/home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20505/execroot/lowrisc_opentitan/sw/device/silicon_creator/rom/rom_epmp.c:89:3: note: Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11
memset(&epmp_state, 0, sizeof(epmp_state));
^~~~~~

INFO: From Running clang tidy on sw/device/silicon_creator/rom/keys/fake/sigverify_rsa_keys_fake.c:
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sw/device/silicon_creator/rom/keys/fake/sigverify_rsa_keys_fake.c"
No compilation database found in /home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20509/execroot/lowrisc_opentitan/sw/device/silicon_creator/rom/keys/fake or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
INFO: From Running clang tidy on sw/device/lib/base/memory.c:
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sw/device/lib/base/memory.c"
No compilation database found in /home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20503/execroot/lowrisc_opentitan/sw/device/lib/base or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
5 warnings generated.
./sw/device/lib/base/memory.h:88:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
__builtin_memcpy(&val, ptr, sizeof(uint32_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:88:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
__builtin_memcpy(&val, ptr, sizeof(uint32_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:88:3: warning: Null pointer passed as 2nd argument to memory copy function [clang-analyzer-unix.cstring.NullArg]
__builtin_memcpy(&val, ptr, sizeof(uint32_t));
^
/home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20503/execroot/lowrisc_opentitan/sw/device/lib/base/memory.c:149:10: note: Assuming 'end' is <= 'tail_offset'
for (; end > tail_offset; --end) {
^~~~~~~~~~~~~~~~~
/home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20503/execroot/lowrisc_opentitan/sw/device/lib/base/memory.c:149:3: note: Loop condition is false. Execution continues on line 157
for (; end > tail_offset; --end) {
^
/home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20503/execroot/lowrisc_opentitan/sw/device/lib/base/memory.c:157:10: note: Assuming 'end' is > 'body_offset'
for (; end > body_offset; end -= sizeof(uint32_t)) {
^~~~~~~~~~~~~~~~~
/home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20503/execroot/lowrisc_opentitan/sw/device/lib/base/memory.c:157:3: note: Loop condition is true. Entering loop body
for (; end > body_offset; end -= sizeof(uint32_t)) {
^
/home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20503/execroot/lowrisc_opentitan/sw/device/lib/base/memory.c:160:27: note: Calling 'read_32'
uint32_t word_right = read_32(&rhs8[i]);
^~~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:86:3: note: Null pointer value stored to 'ptr'
ptr = __builtin_assume_aligned(ptr, alignof(uint32_t));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:88:3: note: Null pointer passed as 2nd argument to memory copy function
__builtin_memcpy(&val, ptr, sizeof(uint32_t));
^ ~~~
./sw/device/lib/base/memory.h:120:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
__builtin_memcpy(&val, ptr, sizeof(uint64_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:120:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
__builtin_memcpy(&val, ptr, sizeof(uint64_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:146:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
__builtin_memcpy(ptr, &value, sizeof(uint32_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:146:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
__builtin_memcpy(ptr, &value, sizeof(uint32_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:172:3: warning: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
__builtin_memcpy(ptr, &value, sizeof(uint64_t));
^~~~~~~~~~~~~~~~
./sw/device/lib/base/memory.h:172:3: note: Call to function 'memcpy' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memcpy_s' in case of C11
__builtin_memcpy(ptr, &value, sizeof(uint64_t));
^~~~~~~~~~~~~~~~

INFO: From Running clang tidy on sw/device/silicon_creator/rom/keys/fake/spx/sigverify_spx_keys_fake.c:
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sw/device/silicon_creator/rom/keys/fake/spx/sigverify_spx_keys_fake.c"
No compilation database found in /home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20511/execroot/lowrisc_opentitan/sw/device/silicon_creator/rom/keys/fake/spx or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
INFO: From Running clang tidy on sw/device/silicon_creator/rom/sigverify_keys_spx.h:
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sw/device/silicon_creator/rom/sigverify_keys_spx.h"
No compilation database found in /home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20512/execroot/lowrisc_opentitan/sw/device/silicon_creator/rom or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
ERROR: /home/dbeitel/rivos/org.lowrisc.opentitan/sw/device/silicon_creator/rom/BUILD:107:11: Running clang tidy on sw/device/silicon_creator/rom/rom.c failed: (Exit 1): clang_tidy.py failed: error executing command rules/scripts/clang_tidy.py .clang-tidy.lock bazel-out/k8-fastbuild-ST-2cc462681f62/bin/sw/device/silicon_creator/rom/rom.c.rom_common.clang-tidy.out external/lowrisc_rv32imcb_files/bin/clang-tidy ... (remaining 62 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox
Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sw/device/silicon_creator/rom/rom.c"
No compilation database found in /home/dbeitel/.opentitan/69b6c25f90ef784e2d66915c4f35fe18/sandbox/linux-sandbox/20507/execroot/lowrisc_opentitan/sw/device/silicon_creator/rom or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory

dmcardle · 2023-05-31T17:56:29Z

@dbeitel-opentitan Thanks for pointing that out. The //quality:clang_tidy_check target is very new. Rather than incrementally chiseling away at the failures, I'll just remove clang-tidy's warnings-as-errors behavior for now.

[bazel] Change clang-tidy rules into test rules

dccaba7

This enables targets instantiated by the clang-tidy rules to depend on test targets. Issue lowRISC#18726 Signed-off-by: Dan McArdle <[email protected]>

dmcardle added the SW:Build System label May 30, 2023

dmcardle requested review from pamaury and a-will May 30, 2023 14:35

dmcardle requested a review from cfrantz as a code owner May 30, 2023 14:35

pamaury approved these changes May 30, 2023

View reviewed changes

[ci] Exercise Bazel analysis and loading phases for //...

45f78a0

Signed-off-by: Dan McArdle <[email protected]>

dmcardle requested review from milesdai and rswarbrick as code owners May 30, 2023 15:59

milesdai approved these changes May 31, 2023

View reviewed changes

dmcardle merged commit 511b94e into lowRISC:master May 31, 2023

dmcardle mentioned this pull request May 31, 2023

[bazel] master build failing #18726

Closed

dmcardle deleted the dmcardle/clang-tidy-make-test branch July 23, 2023 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bazel] Change clang-tidy rules into test rules #18763

[bazel] Change clang-tidy rules into test rules #18763

dmcardle commented May 30, 2023

dmcardle commented May 30, 2023

pamaury commented May 30, 2023

dmcardle commented May 30, 2023

pamaury commented May 30, 2023

dbeitel-opentitan commented May 31, 2023

dbeitel-opentitan commented May 31, 2023

dmcardle commented May 31, 2023

[bazel] Change clang-tidy rules into test rules #18763

[bazel] Change clang-tidy rules into test rules #18763

Conversation

dmcardle commented May 30, 2023

dmcardle commented May 30, 2023

pamaury commented May 30, 2023

dmcardle commented May 30, 2023

pamaury commented May 30, 2023

dbeitel-opentitan commented May 31, 2023

dbeitel-opentitan commented May 31, 2023

dmcardle commented May 31, 2023