Introduce clang-tidy #920

pjungkamp · 2025-06-04T22:09:52Z

What is `clang-tidy`

clang-tidy is a linter developed as part of the clang-tools. This linter makes use of the clang C/C++ compiler front end to analyze and lint based on the actual C/C++ AST instead of the typical regex-based lints used by e.g. cppcheck.

This allows clang-tidy to automatically fix some of the errors it detects automatically. It is also nicely integrated with other clang tooling like the clangd language server.

Progress

Compile villas-node using clang and fix compiler warnings/errors
Run clang-tidy with a subset of checks enabled as a baseline check
- modernize-use-override: Enfore CppCoreGuideline C.128. Refactor and cleanups #907 (comment)
- misc-const-correctness: Add const to all variables that can be.
- clang-analyzer-*: Use clang static analyzer to find memory leaks, null dereferences, use-after-free and other bugs hidden in control flow or non-exception-safe code.
Detect and run clang-tidy automatically through cmake's CMAKE_CXX_CLANG_TIDY integration.
Enforce a subset of warnings as errors in pre-commit (and therefore the CI)

Signed-off-by: Philipp Jungkamp <[email protected]>

pjungkamp · 2025-06-04T22:41:09Z

I'd like to also enable readability-isolate-declaration. This is more of a style choice, less about correctness.

pjungkamp · 2025-06-04T22:47:15Z

common/lib/memory.cpp

The LinearAllocator triggered the clang-analyzer "dead-write" warning. I think this is definitely broken!

Okay, im fine with leaving it as is for now.. I am not aware of anybody using this code.

Maybe @n-eiling

I think the FPGA code is only using the HostRamAllocator. What is a dead-write warning?

A dead write is a write to non-volatile memory that is never read for the remainder of that memory's lifetime. This constructor modifies the function parameters after the member variables have been assigned by the member-initialization-list. This means that these modifications will just be discarded at the end of a function.

pjungkamp · 2025-06-04T22:50:54Z

@stv0g I added the NIX_LD and NIX_LD_LIBRARY_PATH environment variables to the devShells to allow the prebuilt pre-commit binaries to run on NixOS, which is missing a system-wide dynamic linker.

See https://github.com/nix-community/nix-ld?tab=readme-ov-file#usage

stv0g · 2025-06-05T05:13:58Z

Great work :-)

This PR is getting a bit too large for confidently merging and reviewing it.

Especially the clang-tidy commit (cab6e02) is quite big..

I would favour to merge this as is now, or splitting up the PR into smaller ones which can be merged immediatly.

n-eiling

In general this looks like it improves code quality quite a bit, espc. regarding override/virtual. However, I'm not convinced of adding const everywhere. Surely the compiler will do this optimization automatically. I doubt it adds much value during programming. I get the value for function parameters, especially if part of an interface, but for local variables I don't see the point.
I want us to focus on making contributions to VILLAS easier, and forcing people to use const everywhere may not be a move in this direction, or what do you think?

Also again: Please do not do style and logic changes in one PR, especially a PR of this size.

n-eiling · 2025-06-05T06:43:17Z

CMakeLists.txt

+    -Wall
+    -Wno-unknown-pragmas
+    -Wno-vla-cxx-extension
+    -Wno-unused-command-line-argument


can you motivate why you introduced these new options?

GCC and Clang have a different set of warnings enabled with -Wall. This silences two warnings that would, together with -Werror break a Clang Build.

wouldn't fixing them be more desireable?

I introduced two new flags here.

-Wno-vla-cxx-extension: Clang would issue a warning for each variable length array on the stack. This affects most node types.

-Wno-unused-command-line-argument: CMake seems to introduce -Wl,--compress-debug-sections command line arguments which Clang doesn't understand and then warns on.

n-eiling · 2025-06-05T06:46:37Z

common/include/villas/kernel/vfio_device.hpp


  // libpci handle of the device
-  const kernel::devices::PciDevice *pci_device;
+  [[maybe_unused]] const kernel::devices::PciDevice *pci_device;


Is this really something we want to introduce? It adds clutter to the code. In this instance, we should check whether we actually need this (now or in the future) and rather remove it.
Anything touching the FPGA code should be tested by someone with access to the FPGA before we should approve changes.

This private member variable was set in the constructor, yet never read or used anywhere. I didn't know whether this was some kind of guard variable keeping a PCI connection alive or something like that.

I suspect it's a leftover from when vfio devices were always PCI devices. If it's really unused now, we can remove it.

n-eiling · 2025-06-05T06:57:08Z

common/lib/kernel/devices/pci_device.cpp

+    auto line = std::unique_ptr<char, decltype(line_destructor)>{
+        line_ptr, line_destructor};
+    if (ret == -1)
+      break;


I don't see the point of this. It's way less readable than the C code. If you absolutely must use C++, then why not use proper C++ file I/O? Also any change here needs to be tested by someone with access to an FPGA.

n-eiling · 2025-06-05T06:58:05Z

common/lib/kernel/kernel.cpp

-  FILE *f = fopen(PROCFS_PATH "/cmdline", "r");
+  auto file_destructor = [](FILE *file) { fclose(file); };
+  auto f = std::unique_ptr<FILE, decltype(file_destructor)>{
+      fopen(PROCFS_PATH "/cmdline", "r"), file_destructor};


Same here. I don't see the point of this change. Also needs FPGA testing.

clang-analyzer found a control flow path that lead to fclose not being called here. More specifically, it's missing an fclose before the return 0; // FOUND line. It also guards against leaks if an exception is thrown (I don't currently see any exception throwing code here, but I'd argue that using smart pointers for resource owning pointers is a better and less error-prone programming style.

Ok makes sense. Still you are mixing C and C++ code here, which I think is a bit ugly. Also the syntax is quite a bit more complex. I'm fine with your change, but I would prefer switching to a C++ API, or fixing the existing code.

n-eiling · 2025-06-05T06:58:13Z

common/lib/kernel/kernel.cpp

-  f = fopen(fn, "w+");
+  auto file_destructor = [](FILE *file) { fclose(file); };
+  auto f = std::unique_ptr<FILE, decltype(file_destructor)>{fopen(fn, "w+"),
+                                                            file_destructor};


n-eiling · 2025-06-05T07:31:15Z

src/villas-conf2json.cpp

-    FILE *f = fopen(argv[1], "r");
+    auto file_destructor = [](FILE *file) { fclose(file); };
+    auto f = std::unique_ptr<FILE, decltype(file_destructor)>{
+        fopen(argv[1], "r"), file_destructor};


I think this is less readable than the C code, and the better way would be to use C++ file I/O

n-eiling · 2025-06-05T07:33:05Z

tests/unit/format.cpp

-                     "Sample data mismatch at index %d: %lld != %lld", j,
-                     a->data[j].i, b->data[j].i);
+                     "Sample data mismatch at index %d: %" PRId64
+                     " != %" PRId64,


What is this? Maybe better use C++ format functions if the C ones aren't sufficient?

n-eiling · 2025-06-05T07:33:22Z

tests/unit/format.cpp

 #endif

  rewind(stream);
+  (void)errno; // TODO: should we handle rewind errors?


Maybe make an issue

n-eiling · 2025-06-05T07:34:19Z

tests/unit/queue.cpp

                   p->many ? producer_consumer_many : producer_consumer, p);

-  sleep(0.2);
+  // sleep(0.2); TODO: Is this supposed to to something? Yield?


Dead code. Can we understand why this was here before just removing it?

n-eiling · 2025-06-05T07:34:56Z

tools/format-all.sh

-                -not \( -path "${TOP_DIR}/fpga/thirdparty/*" -o -path "${TOP_DIR}/build*/*" \) | \
-    xargs clang-format --verbose -i
+git ls-files -c -z -- "*.c" ".h" "*.hpp" "*.cpp" ":!:fpga/thirdparty" |\
+    xargs -0 clang-format --verbose -i


what are the implications of this change?

It does everything as before, except it only formats files known to git and works with paths that contain spaces and even newlines.

n-eiling · 2025-06-05T08:03:32Z

And any changes on what we expect from contributions should also be added here: https://villas.fein-aachen.org/docs/node/development/contributing Let's not let this get out of date again.

pjungkamp · 2025-06-05T08:32:48Z

I want to emphasize that this PR is not yet ready. I just wanted to track the progress through the GitHub PR checklist and discuss what lints to enable initially.

I want to split the large enable clang tidy commit by the lint that it fixed. I can also split out a base commit that just fixes the clang build.

I'll also add a related PR to the style guide documentation before this is ready to review.

stv0g · 2025-06-12T05:11:45Z

I want to emphasize that this PR is not yet ready.

Could we split out-some commits into small PRs please? There are multiple commits which deserve their own PR and we can review and merge those quicker.

Thanks :)

pjungkamp force-pushed the clang-tidy branch 2 times, most recently from e130e75 to d114491 Compare June 4, 2025 22:12

pjungkamp added 2 commits June 5, 2025 00:14

fpga: Remove dead code

4fc4b5a

Signed-off-by: Philipp Jungkamp <[email protected]>

fpga: Remove unused broken unit test

bf6ea2f

Signed-off-by: Philipp Jungkamp <[email protected]>

pjungkamp force-pushed the clang-tidy branch from d114491 to 1003d2a Compare June 4, 2025 22:15

pjungkamp added 4 commits June 5, 2025 00:34

Introduce clang-tidy and fix diagnostics

cab6e02

Signed-off-by: Philipp Jungkamp <[email protected]>

nix: Add clang-built package and devShell

f67eb09

Signed-off-by: Philipp Jungkamp <[email protected]>

tools: Use git ls-files instead of find

10f83c3

Signed-off-by: Philipp Jungkamp <[email protected]>

nix: Install pre-commit hooks using direnv

3f9dfc6

Signed-off-by: Philipp Jungkamp <[email protected]>

pjungkamp force-pushed the clang-tidy branch from 1003d2a to 3f9dfc6 Compare June 4, 2025 22:34

pjungkamp commented Jun 4, 2025

View reviewed changes

n-eiling requested changes Jun 5, 2025

View reviewed changes

Introduce clang-tidy #920

Are you sure you want to change the base?

Introduce clang-tidy #920

Uh oh!

Conversation

pjungkamp commented Jun 4, 2025

What is clang-tidy

Progress

Uh oh!

pjungkamp commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjungkamp commented Jun 4, 2025

Uh oh!

stv0g commented Jun 5, 2025

Uh oh!

n-eiling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-eiling Jun 5, 2025 • edited by pjungkamp Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-eiling commented Jun 5, 2025

Uh oh!

pjungkamp commented Jun 5, 2025

Uh oh!

stv0g commented Jun 12, 2025

Uh oh!

Uh oh!

What is `clang-tidy`

pjungkamp commented Jun 4, 2025 •

edited

Loading

n-eiling Jun 5, 2025 •

edited by pjungkamp

Loading