Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make autoware_cmake Clang-friendly #11

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mojomex
Copy link

@mojomex mojomex commented Dec 2, 2024

Description

Currently, many packages in Autoware cannot be built with Clang (for the purposes of this PR, Clang = Clang-15, the newest version packaged on Ubuntu 22.04).

This PR demotes a number of additional compiler warnings only output by Clang from error to warning, in order to make a large number of Autoware packages buildable while staying minimally invasive. The main aim of this PR is to facilitate a timely introduction of Clang-Tidy across all Autoware packages.

Why Clang?

Clang provides the foundation for most modern state-of-the-art static analysis tools, such as Clang-Tidy and FB-Infer. Static analysis is of utmost importance to catch bugs before they hit production, especially in autonomous driving. Not having Clang support prevents us from fully introducing this common industry practice. Also see the Clang-Tidy discussion.

Further, Clang detects many more problems in the source code, thus giving developers better input on how to improve their code. Clang-specific features might also be a reason for some developers, see this discussion.

The current situation

Currently, out of 347 packages, 17 (5%) fail on Clang due to compiler errors, preventing 172 (50%) dependent packages from being built. Note that even with fixes in those 17 packages, other packages do have compiler errors as well.

Root cause

As Clang detects more code problems than GCC, and since -Werror is set for both compilers, the additional warnings flagged by Clang are promoted to error and cause many packages' builds to fail.

These errors (taken from the fixed log retroactively) are preventing Autoware from being built*:

208 -Wdeprecated-copy
66 -Wdelete-non-abstract-non-virtual-dtor
27 -Wunused-private-field
26 -Wc11-extensions
20 -Wsign-conversion
19 -Wformat-security
17 -Wunused-parameter
17 -Winconsistent-missing-override
10 -Wunused-lambda-capture
10 -Wunused-const-variable
8 -Wimplicit-const-int-float-conversion
6 -Wunused-but-set-variable
6 -Wdelete-abstract-non-virtual-dtor
5 -Wdefaulted-function-deleted
3 -Woverloaded-virtual
1 -Wunused-function
1 -Wpessimizing-move
1 -Winfinite-recursion
1 -Wdeprecated-builtins

Detailed warnings here:
warnings.txt

Full log (before):
build-old.log

Full log (after):
build-new.log

Possible fixes

  • Fix all errors in all modules (quite many people involved, hard to manage)
  • Temporarily demote affected Clang-specific warnings to warn

The first option is best handled by architects or teams such as the Autoware engineering team, as this is a coordinated effort involving many contributors.

This PR implements the second option, and since all these warnings are in addition to warnings similar to GCC's ones, I think it is fine to not treat them as error temporarily. Build quality will still not be lower than that produced by GCC.

Effects of this PR

Out of 347 packages, 3 (<1%) fail on Clang, preventing 6 (2%) dependent packages from being built.
This is an 👉 👉 improvement of 114% 👈 👈 in the number of successfully buildable packages!

To get a list of all warnings that were promoted to error and caused a build failure, run:

colcon build 2>&1 | tee build.log
cat build.log | grep -oP '\[-Werror,\K[^]]*' | sort | uniq -c

*: Build command:

colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -DCMAKE_C_COMPILER=clang-15 -DCMAKE_CXX_COMPILER=clang++-15

Tests performed

Not applicable.

Effects on system behavior

  • No effects when building with GCC (default).
  • More packages buildable when building with Clang

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: Max SCHMELLER <[email protected]>
@mojomex
Copy link
Author

mojomex commented Dec 3, 2024

To Do

Limit the list of disabled warnings only to the most impactful ones. Impact, as in, the number of code fixes saved by disabling the warning here.

For some warnings, it will be better to fix them in the respective packages, as they might either be real bugs or they might only affect a single package.

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.

1 participant