Skip to content

Conversation

@hzeller
Copy link
Contributor

@hzeller hzeller commented Dec 22, 2025

This uses http://bant.build/ to create a compilation DB.

The change provides two useful scripts:

  • etc/bazel-make-compilation-db.sh creates a compilation db. To avoid a huge compile_commands.json, this generates the compile_flags.txt which is a much shorter file applied to all files. This keeps memory usage low for tools such as clang-tidy
  • etc/run-clang-tidy.sh a convenience script that creates the compilation DB and then calls run-clang-tidy-cached.cc. It uses clang-tidy from the llvm-toolchain configured in the MODULE.bazel

Issues #8586

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces useful scripts for generating a compilation database and running clang-tidy, which is a great improvement for developer workflow and code quality. The scripts are well-structured. My review includes several suggestions to enhance their robustness and maintainability by following shell scripting best practices. These include adding error handling (set -e), quoting variables and command substitutions to prevent word splitting issues, and using more robust methods like bazel cquery to locate build artifacts instead of relying on fragile path globbing. I've also pointed out a potential bug in a loop that could occur if a glob pattern doesn't match any files.

@hzeller hzeller force-pushed the feature-20251222-add-compdb branch from ead2044 to 930e60b Compare December 22, 2025 13:32
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@hzeller hzeller force-pushed the feature-20251222-add-compdb branch from 930e60b to 5eb9f4a Compare December 23, 2025 09:17
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@hzeller hzeller force-pushed the feature-20251222-add-compdb branch from 5eb9f4a to 3a228ad Compare December 23, 2025 09:21
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

This uses http://bant.build/ to create a compilation DB.

The change provides two useful scripts:

  * `etc/bazel-make-compilation-db.sh` creates a compilation db.
    To avoid a huge compile_commands.json, this generates the
    compile_flags.txt which is a much shorter file applied to all
    files. This keeps memory usage low for tools such as clang-tidy
  * `etc/run-clang-tidy.sh` a convenience script that creates the
    compilation DB and then calls `run-clang-tidy-cached.cc`.
    It uses `clang-tidy` from the llvm-toolchain configured in the MODULE.bazel

Issues The-OpenROAD-Project#8586

Signed-off-by: Henner Zeller <[email protected]>
@hzeller hzeller force-pushed the feature-20251222-add-compdb branch from 3a228ad to 1a2d93e Compare December 23, 2025 12:58
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@hzeller
Copy link
Contributor Author

hzeller commented Dec 23, 2025

(forgot to make it a separate commit, so it is --amend-ed)

@maliberty
Copy link
Member

compile_flags.txt seems to try to be a universal set of includes rather than the specific includes used by a given translation unit. I don't see how that will work in general. For example:

find src/ -name graphics.h
src/mpl/src/graphics.h
src/fin/src/graphics.h
src/exa/src/graphics.h

build/compile_commands.json is 3.4M so larger but not terrible. cmake is much faster at this job as it doesn't require a build. I think you only need to generate the swig wrappers and tcl encoding files.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@hzeller
Copy link
Contributor Author

hzeller commented Dec 24, 2025

compile_flags.txt seems to try to be a universal set of includes rather than the specific includes used by a given translation unit. I don't see how that will work in general. For example:

find src/ -name graphics.h
src/mpl/src/graphics.h
src/fin/src/graphics.h
src/exa/src/graphics.h

Yeah, that is a general problem of the project layout which I have said many times before. Once we can abandon CMake, we should fix the paths to always use paths relative to project root.

build/compile_commands.json is 3.4M so larger but not terrible.

For bazel, it would be several hundred megabytes, as it needs to encode all the paths. With that, running clang-tidy on 128 cores will require several hundred gigabytes of RAM. So unfortunately, a compile_commands.json is not an option right now.

cmake is much faster at this job as it doesn't require a build.

We do that now also but build the relevant generated files. Ideally, bazel would provide a compilation-db in the analysis phase, alas, it is not a feature they have. Bant is the best we have.

I think you only need to generate the swig wrappers and tcl encoding files.

Yep, changed that now, so only the fetch of the dependencies is done plus the handful of generated things (much faster)

@hzeller hzeller force-pushed the feature-20251222-add-compdb branch from 080d3a9 to 2ad0129 Compare December 24, 2025 13:45
@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@mlyoung101
Copy link

Thank you for your work Henner, just confirming this works exactly as expected from my side, including from clangd.

The one observation I do have is that clangd does not seem to generate its symbol database without a compile_commands.json, meaning that it has to re-index the project on the fly as you edit; which is a bit slow in some large files, particularly OpenDB-related things.

I expect that clangd resource usage will be high, but more tame than clang-tidy. If Bant supports generating compile_commands without too much extra trouble, it would be interesting to try and see how this behaves.

@mlyoung101
Copy link

The above ended up being simple enough to do, so I've got some numbers on it. The compile_commands.json database is ~900MB in the default generated format with no minification. clangd uses peak 12GB of RAM to generate the index, which takes about half an hour. I haven't yet tested clang-tidy, the reason being that the database seems invalid; it produces symbol redefinition errors that the compile_commands.txt doesn't seem to generate for some reason.

Machine specs are 32 cores, 64GB DDR4 RAM.

@hzeller
Copy link
Contributor Author

hzeller commented Jan 20, 2026

There are a lot of paths that are needed for bazel, as each external dependency has its own lengthy path. The paths bant emits are also for possible generated sources, so there might be a superset that is not needed in practice.
Usually, just relative paths (the ../../../externa./) should be needed, alas clangd has a fault which does not follow symbolic links directly, hence the same paths show up twice. In any case, that is the reason why you observe such a gigantic file if you use compile_commands.js, which is not very practical with clangd and in particular not clang-tidy if you run it on many cores in parallel.
Hence the recommendation (as implemented in this PR) to use compile_commands.txt.

Anyway, ready to merge ?

@maliberty maliberty merged commit 0f99689 into The-OpenROAD-Project:master Jan 21, 2026
13 checks passed
@maliberty
Copy link
Member

Thanks we can always refine more later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants