[fix] Treat clang-diagnostic-* checkers as compiler flags #3874

vodorok · 2023-04-12T12:26:04Z

Previously the handling of clang-diagnostics was flawed because tidy cannot enable compiler warnings when specified as a checker, only suppression of the resulting reports are available:

From the official clang-tidy docs:

Clang diagnostics are treated in a similar way as check diagnostics. Clang diagnostics are displayed by clang-tidy and can be filtered out using the -checks= option. However, the -checks= option does not affect compilation arguments, so it cannot turn on Clang warnings which are not already turned on in the build configuration. The -warnings-as-errors= option upgrades any warnings emitted under the -checks= flag to errors (but it does not enable any checks itself).

After this patch all of these checkers will be treated as compiler warning flags, this way the compiler warnings can be enabled as clang-diagnostics.
There is an added benefit of this method, as the checker prefix based activation is also working now.

Before the patch:

Trying to use -e clang-diagnostic-... notation did not produce any result because tidy wouldn't insert the corresponding compiler warning to the input compilation command. We thought adding it to the checks list enabled the warning, but it didn't.

After the patch:

Using -e W... notation results in a warning from CodeChecker to use the clang-diagnostic-unused-variable counterpart.
Using the -e clang-diagnostic-... now properly works. and enables the related compiler warnings.
When using --enable-all, instead of adding every compiler warning individually to the build command, -Weverything is added.

There are some considerations regarding the old -W... notation.

Using the -W... notation is be deprecated, but for the time being still usable.\
With --enable-all the usage of -d -W... notation is ignored, and results in a warning. The -d compiler-diagnostics-... is the recommended way to disable a compiler warning.

Previously the handling of clang-diagnostics was flawed, because tidy cannot enable those as a checker, only suppression of the resulting reports are available: https://github.com/kimgr/clang-tools-extra/blob/master/docs/clang-tidy.rst After this patch all of these checkers will be treated as compiler warning flags, this way the compiler warnings can be enabled as clang-diagnostics. There is an added benefit of this method, as the checker prefix based activation is also working now. The old -W notation will be deprecated, but for the time being still usable.

Szelethus

The story here, as I understand it is the following. Both the CLI and the GUI could incorrectly let you believe that we handle compiler warnings as checkers, but that is only party true. We can't reliably enable/disable them. Not only that, the checker name they are displayed with start with "compiler-diagnostic-", but they must be named as "W" on the CLI.

Previously the handling of clang-diagnostics was flawed, because tidy cannot enable those as a checker, only suppression of the resulting reports are available:

Yes, because compiler warnings are flags to the compiler, not the analyzer component. This makes some sense for a compiler developer, but as a user of a static analyzer tool, compiler warnings should be checkers. If clang-tidy won't do it on its own for whetever reason, we need to make it so that CodeChecker acts as if they were.

https://github.com/kimgr/clang-tools-extra/blob/master/docs/clang-tidy.rst

This is painfully out of date. Back in the day, LLVM didn't have a single monorepo housing all of its subprojects, but all subprojects had their own repository (imagine how painful an LLVM-wide refactoring used to be!). Not only is this repo from that era, it is ~6 years before the merging, and its not even the official mirror either.

This is probably what you are after:
https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics

After this patch all of these checkers will be treated as compiler warning flags,

Not the other way around?

this way the compiler warnings can be enabled as clang-diagnostics.

Meaning that the name of the checker on the GUI and the name to be used on the CLI will be identical.

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

Szelethus · 2023-04-12T13:40:51Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

-                checker_name += '*'
+
+            # If clang-diagnostic is enabled, else add it as a disabled
+            # check.


Did you mean to say that all clang-diagnostics not explicitly enabled should be disabled?

I'll rephrase this comment. I wanted to write something like this:

If a clang-diagnostic is enabled add it as a -W flag, else add it as a disabled checker to the checkers list.

This english is still a little broken.

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

Szelethus · 2023-04-12T13:49:12Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

+
+            # If clang-diagnostic is enabled, else add it as a disabled
+            # check.
+            if checker_name.startswith('clang-diagnostic-') and \


We already have the variable warning_name, that holds the information on whether we recognized the checker as compiler warning, no? Why are we reinveting the wheel instead of reusing the result of get_compiler_warning_name?

At the first iteration of this pr, I modified get_compiler_warning_name in a manner that it worked on clang-diagnostic- checker names. I did not like this approach, because in that implementation all the disabled clang-diagnostic- checkers were included as -Wno compiler flags in the build command passed to tidy.

I still think this is bad design. The code would look more readable if we moved that logic to get_compiler_warning_name (where it belongs), and we really need to, check whether this is a disabled clang-diagnostic after the fact.

vodorok · 2023-04-12T14:37:19Z

@Szelethus

The story here, as I understand it is the following. Both the CLI and the GUI could incorrectly let you believe that we handle compiler warnings as checkers, but that is only party true. We can't reliably enable/disable them. Not only that, the checker name they are displayed with start with "compiler-diagnostic-", but they must be named as "W" on the CLI.

I think you are mistaken here. The intent is to forgo the W designation, and from the perspective of the CodeChecker CLI use only the clang-diagnostic naming.

The following matrix shows the current state of compiler warning activation

	-W...	-Wno...	clang-diagnostics...
--enable	adds -W... flag to the build command	adds -Wno... flag to the build command	adds clang-diagnostic-... to the checks list / not working
--disable	adds -Wno-no... flag to the build command	broken Wno-no flag is added	adds -clang-diagnostic-.. to the checks list / suppresses the compiler warnings based on checker name prefixes

Previously the handling of clang-diagnostics was flawed, because tidy cannot enable those as a checker, only suppression of the resulting reports are available:

Yes, because compiler warnings are flags to the compiler, not the analyzer component. This makes some sense for a compiler developer, but as a user of a static analyzer tool, compiler warnings should be checkers. If clang-tidy won't do it on its own for whetever reason, we need to make it so that CodeChecker acts as if they were.

This is the intention of the patch.

https://github.com/kimgr/clang-tools-extra/blob/master/docs/clang-tidy.rst

This is painfully out of date. Back in the day, LLVM didn't have a single monorepo housing all of its subprojects, but all subprojects had their own repository (imagine how painful an LLVM-wide refactoring used to be!). Not only is this repo from that era, it is ~6 years before the merging, and its not even the official mirror either.

Thanks for pointing out that I included an outdated (and forked) documentation. The official docs still state the same.
https://clang.llvm.org/extra/clang-tidy/
Look below the checker group table

From the official clang-tidy docs:

Clang diagnostics are treated in a similar way as check diagnostics. Clang diagnostics are displayed by clang-tidy and can be filtered out using the -checks= option. However, the -checks= option does not affect compilation arguments, so it cannot turn on Clang warnings which are not already turned on in the build configuration. The -warnings-as-errors= option upgrades any warnings emitted under the -checks= flag to errors (but it does not enable any checks itself).

quote end

This is probably what you are after: https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics

This was it thanks!

After this patch all of these checkers will be treated as compiler warning flags,

Not the other way around?

No, because of the previously mentioned facts.

this way the compiler warnings can be enabled as clang-diagnostics.

Meaning that the name of the checker on the GUI and the name to be used on the CLI will be identical.
This is true.

Before the patch:

You tried to use -e clang-diagnostic-vla you did not get any result because tidy wouldn't insert the corresponding compiler warning to the input compilation command, and we thought adding it to the checks list enables it.

After the patch:

You use -e Wunused-variable you get a warning from CodeChecker to use the clang-diagnostic-unused-variable counterpart.
You can use the clang-diagnostic-vla naming to enable the vla-related compiler warnings.

Thanks for the review.

Szelethus

Tiny things left to attend to.

edit: Please do better in the summary.

Szelethus · 2023-04-13T10:45:24Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

-                checker_name += '*'
+
+            # If clang-diagnostic is enabled, else add it as a disabled
+            # check.


This english is still a little broken.

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

web/tests/functional/report_viewer_api/test_get_run_results.py

Szelethus

Ugh, sorry :)

The summary is nice.

Szelethus · 2023-04-13T12:17:40Z

analyzer/codechecker_analyzer/analyzers/config_handler.py

-    Removes 'W' or 'Wno' from the compiler warning name, if this is a
-    compiler warning. Returns None otherwise.
+    Removes 'W' or 'Wno' from the compiler warning name, or
+    'clang-diagnostic-' from the checker name.


This is just not true right now.

Szelethus · 2023-04-13T12:19:25Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

+
+            # If clang-diagnostic is enabled, else add it as a disabled
+            # check.
+            if checker_name.startswith('clang-diagnostic-') and \


I still think this is bad design. The code would look more readable if we moved that logic to get_compiler_warning_name (where it belongs), and we really need to, check whether this is a disabled clang-diagnostic after the fact.

Szelethus

LGTM! If you wanna make things a bit simpler, go ahead, I'll leave that to you.

vodorok added this to the release 6.22.0 milestone Apr 12, 2023

vodorok requested a review from Szelethus April 12, 2023 12:26

vodorok self-assigned this Apr 12, 2023

vodorok requested a review from bruntib as a code owner April 12, 2023 12:26

vodorok force-pushed the clang_diags_as_compiler_warns branch 3 times, most recently from 08a3c10 to 12bcf39 Compare April 12, 2023 12:39

vodorok force-pushed the clang_diags_as_compiler_warns branch from 12bcf39 to 533b3c3 Compare April 12, 2023 12:43

Try a different approach

cf6fbf1

Szelethus requested changes Apr 12, 2023

View reviewed changes

Fix --enable-all mode, and convert tests

7dfd1ee

vodorok force-pushed the clang_diags_as_compiler_warns branch 10 times, most recently from 6c399c4 to b8c89f6 Compare April 12, 2023 23:21

bruntib approved these changes Apr 13, 2023

View reviewed changes

Szelethus requested changes Apr 13, 2023

View reviewed changes

Szelethus added bugfix 🔨 analyzer 📈 Related to the analyze commands (analysis driver) clang-tidy 🐉 clang-tidy is a clang-based C++ “linter” tool. deprecate ⏳ deprecate a feature labels Apr 13, 2023

Fix web tests, and comments

c5f4858

vodorok force-pushed the clang_diags_as_compiler_warns branch from b8c89f6 to c5f4858 Compare April 13, 2023 11:27

vodorok requested a review from Szelethus April 13, 2023 12:20

Szelethus requested changes Apr 13, 2023

View reviewed changes

Refactor warning determination logic

c1f1bbb

Szelethus approved these changes Apr 13, 2023

View reviewed changes

bruntib merged commit 2ceedf0 into Ericsson:master Apr 14, 2023

vodorok deleted the clang_diags_as_compiler_warns branch May 22, 2023 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Treat clang-diagnostic-* checkers as compiler flags #3874

[fix] Treat clang-diagnostic-* checkers as compiler flags #3874

vodorok commented Apr 12, 2023 •

edited

Loading

Szelethus left a comment

Szelethus Apr 12, 2023

vodorok Apr 12, 2023

Szelethus Apr 13, 2023

Szelethus Apr 12, 2023

vodorok Apr 12, 2023

Szelethus Apr 13, 2023

vodorok commented Apr 12, 2023 •

edited

Loading

Szelethus left a comment •

edited

Loading

Szelethus Apr 13, 2023

Szelethus left a comment •

edited

Loading

Szelethus Apr 13, 2023

Szelethus Apr 13, 2023

Szelethus left a comment

[fix] Treat clang-diagnostic-* checkers as compiler flags #3874

[fix] Treat clang-diagnostic-* checkers as compiler flags #3874

Conversation

vodorok commented Apr 12, 2023 • edited Loading

Szelethus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vodorok commented Apr 12, 2023 • edited Loading

Szelethus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szelethus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szelethus left a comment

Choose a reason for hiding this comment

vodorok commented Apr 12, 2023 •

edited

Loading

vodorok commented Apr 12, 2023 •

edited

Loading

Szelethus left a comment •

edited

Loading

Szelethus left a comment •

edited

Loading