Eliminate "unknown" checker state #3949

bruntib · 2023-07-10T10:57:11Z

Currently there are 3 kind of checkers regarding whether they are
running during analysis: enabled, disabled, unknown. Checkers can be
enabled if they are explicitly given to --enable flag, or they are the
member of "default" profile. Checkers can also be explicitly disabled.
The 3rd status is tricky: CodeChecker doesn't explicitly enable or
disable them, but gives the choice to the analyzer tool.

This behavior is bad, because a user cannot tell exactly which checkers
were executed during analysis. It is impossible to determine in case of
no error if a checker was disabled or just simply didn't report any
issue.

The goal in this commit is that every checker is either enabled or
disabled. It is the analyzer module's responsibility to assemble an
analysis command which executes at least the enabled checkers and turns
off or hides the disabled ones.

[feat] ClangTidy enables only selected checkers

Due to the previous commit every checker is either enabled or disabled.
This resulted very long analyzer commands in ClangTidy because every
checker was explicitly enabled or disabled in the clang-tidy invocation.
ClangTidy has a lot of checkers and also all compiler warnings listed by
diagtool are also considered as checkers.

The goal of this commit is to simplify the clang-tidy invocation command.
First we disable all checkers and enable only those that are necessary to
run:

clang-tidy -checks=-*,enabled1,enabled2...

This patch takes into account that clang-diagnostic-... checkers don't
turn on the corresponding warnings. They have to be enabled by -W...
compiler flag.

[feat] Hide disabled ClangSA checkers in post-processing

ClangSA checkers may rely on each other. For example unix.Malloc checker
may prevent a report from unix.MismatchedDeallocator. It is possible that
after disabling unix.Malloc the underlying analysis continues giving a
chance for unix.MismatchedDeallocator to emit a report.

In order to avoid this noisy behavior, ClangSA checkers are never
disabled during the analysis invocation. If CodeChecker wants to disable
a checker then its result should be hidden in a post-processing step.

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

Szelethus · 2023-07-11T08:18:45Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

@@ -165,13 +165,51 @@ def get_warnings(env=None):
        raise


+def _add_star_for_group(
+    checkers: Iterable[str],


checkers and all_checkers? Since this is a single purpose function, how about we call this enabled_checkers, or subset_checkers?

Szelethus · 2023-07-11T08:30:17Z

analyzer/tests/unit/test_checker_handling.py

@@ -278,14 +330,18 @@ def test_disable_clangsa_checkers(self):

        for arg in analyzer.construct_analyzer_cmd(result_handler):
            if arg.startswith('-checks='):
-                self.assertIn('-clang-analyzer-*', arg)
+                self.assertLess(*self._enable_disable_pos(


This is still a chore to digest. Can't you reduce it down to something easier to read? Like, is_disabled?

Szelethus · 2023-07-11T09:15:22Z

Also, my usual complaint. You explain in the summary what this patch does, but not why this is any good.

bruntib · 2023-07-17T13:03:11Z

I fixed the things according to the comments, thank you!
As for the last one, I believe the commit messages answer the question of "what". If it doesn't then let's elaborate on it, I would appreciate if you could help to make comments as clear as possible. However, I think some comments have to describe "why" something happens, because many times it is hard to figure it out if something is done consciously or inherited from some outdated legacy.

Szelethus · 2023-07-18T10:59:37Z

Here is what I mean under the why missing from the summary.

Ideally, I would like to learn the following:

How the program works now
Why is this behaviour undesirable or incorrect, and what it should be instead
What was the intent behind this behaviour back in the day (or at least an educated guess)
If you considered multiple high level approaches to fix this, what were they, why did you pick the one found in the patch
Briefly describe what the actual patch is about, how it fixed the problem.

Szelethus

If I never had an issue with wanting to disable a checker but couldn't, would I notice any change in the new behaviour? Moving on, would I need to manually enable/disable clang-diagnostics to restore the pre-patch result set?

analyzer/codechecker_analyzer/analyzers/clangsa/analyzer.py

Szelethus · 2023-07-27T10:28:52Z

analyzer/codechecker_analyzer/analyzers/clangsa/analyzer.py

+        except plistlib.InvalidFileException:
+            # It may happen that a compilation database contains a build action
+            # multiple times. For further details see analyzer_action_str() in
+            # ResultHandler. If this happens then the analysis of the same


That comment says nothing about multiple build command occurances.

analyzer/codechecker_analyzer/analyzers/clangsa/analyzer.py

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

Szelethus · 2023-07-27T10:40:05Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

+            # This warning must be given a parameter separated by either '=' or
+            # space. This warning is not supported as a checker name so its
+            # special usage is avoided.
+            if warning_name and warning_name.startswith('frame-larger-than'):


Do we need to this in this patch? Seems to be a new addition. If we must, we could at least emit a curtesy warning that this won't be enabled.

This is a function which returns the available checkers. By this if statement this "checker" will not be displayed for the users, do they don't even know about the existence of it. They cant provide it in any analyzer command.

This doesn't explain my question. Can this not be a separate patch? If not, why is it justified here?

I might have misunderstood, but in your original comment you suggested to provide a "courtesy warning" for the users which says that this checker/warning will not be enabled. That's why I answered that there is no need for such a warning, because CodeChecker users will not even know about the existence of this "checker" if we exclude it in this get_checker_list() function.

And we have to exclude it, because otherwise an invalid clang-tidy invocation will be assembled: clang-tidy ... main.c -- -Wframe-larger-than=. This is invalid, because this warning requires an integer parameter after =. So this is why I decided to hide this checker/warning at all. If I don't exclude this warning then one of our tests fail which enable all "clang-diagnostic-" checkers:
CodeChecker analyze -e clang-diagnostic ...

That's why I answered that there is no need for such a warning, because CodeChecker users will not even know about the existence of this "checker" if we exclude it in this get_checker_list() function.

This would be valid if CodeChecker was the entity that released this warning, but that is not the case, its perfectly reasonable for a Clang user to expect this warning to just work,

Aha, I understand now that this function asks clang-tidy for the list of recognized checks, and removed this particular one, so it will act as if the requested didn't exist, resulting in a warning.

But not the right warning. It will say that the check doesn't exist, which is not true -- it does exist, we just don't don't allow running it under CodeChecker. Lets save the user some headache, his Clang binary is not broken -- please emit a warning about this.

warning: CodeChecker is incompatible with compiler warning 'frame-larger-than'. Please don't enable it.

analyzer/tests/functional/analyze_and_parse/test_files/compiler_warning_no_warn.output

analyzer/tests/unit/test_checker_handling.py

Szelethus · 2023-07-27T10:50:12Z

analyzer/tests/unit/test_checker_handling.py

-                self.assertIn('-clang-analyzer-*', arg)
+                self.assertTrue(self._is_disabled(
+                    'clang-analyzer',
+                    arg[len('-checks='):].split(',')))


Yeah, this isn't any better. It should look like this:

self.assertTrue(self._is_disabled('clang-analyzer', arg))

and other nasties should be inside _is_disabled. Do you see my point?

If I do it this way then test_default_checkers_are_not_disabled() and test_clangsa_analyzer_checks_are_disabled() will be ugly. Which one should we save? :)

Is this really a decision to make? :D Can you not make a function for this case and one for the another so that both of them are legible?

Done, restructured.

Szelethus

This is getting close!

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

Szelethus · 2023-08-14T15:25:18Z

analyzer/codechecker_analyzer/analyzers/clangtidy/analyzer.py

+            # This warning must be given a parameter separated by either '=' or
+            # space. This warning is not supported as a checker name so its
+            # special usage is avoided.
+            if warning_name and warning_name.startswith('frame-larger-than'):


That's why I answered that there is no need for such a warning, because CodeChecker users will not even know about the existence of this "checker" if we exclude it in this get_checker_list() function.

This would be valid if CodeChecker was the entity that released this warning, but that is not the case, its perfectly reasonable for a Clang user to expect this warning to just work,

Szelethus

Ugh, sorry, after taking another look at the tests, I'm still a little confused.

Szelethus · 2023-08-15T09:14:35Z

web/tests/functional/diff_local/test_diff_local.py

@@ -77,15 +77,16 @@ def setup_class(self):
            'skip_list_file': skip_list_file,
            'check_env': test_env,
            'workspace': TEST_WORKSPACE,
-            'checkers': [],
+            'checkers': ['-d', 'clang-diagnostic'],


Sorry, it seems like you have to beat this into me. Don't we disable all compiler warnings out of the box? Why is this needed? Why did you need to change almost all invocations of CodeChecker in these tests? Its a mess.

Some clang-diagnostic-... checkers have been added to the "default" profile (see the changes of clang-tidy.json), that's why some of them appeared in tests files. So either I rewrite all test files to eliminate the source of the report, or I disable clang-diagnostic-... checkers in order to remain conform with the previous behavior.

Aha, I see. In that case, can we not move the profile changed to its own PR?

Szelethus · 2023-08-15T09:17:03Z

web/tests/functional/report_viewer_api/__init__.py

-                                   '-d', 'unix.Malloc'
+                                   '-d', 'unix.Malloc',
+                                   '-d', 'clang-diagnostic',
+                                   '-e', 'clang-diagnostic-division-by-zero'


Why is this needed? I am more interested in the design reason, not the technical one. Suppose that you are new to this test file, and stumble upon this invocation -- what does it convey to you? We manually disable all warnings for some reason, but enable a specific one after that?

Was this an attempt to reduce the amount of changes needed in the tests, otherwise everything broke?

Same answer as above. As for the design reason: in this test we have multiple analyses with different configurations. Here we want to test which checkers' reports have appeared and disappeared. We control this by explicitly enabling and disabling them in different analysis actions.

Can you explain in the comments why we have this particular checker configuration?

Szelethus · 2023-08-15T09:17:48Z

web/tests/functional/report_viewer_api/__init__.py

@@ -114,7 +123,11 @@ def setup_class_common(workspace_name):
    print("Third analysis of the test project was successful.")

    # Let's run the second analysis and updat the same run.
-    codechecker_cfg['checkers'] = ['-d', 'core.StackAddressEscape']
+    codechecker_cfg['checkers'] = ['-d', 'core.StackAddressEscape',
+                                   '-d', 'unix.Malloc',


Whats the reason behind this? Why did you disable MallocChecker?

Earlier the meaning of --disable flag was to actually disable the checker in the clang invocation. Now the meaning of --disable flag is to hide the results of the given checker. So after this patch core.StackAddressEscape is actually running, even if disabled, but its reports are hidden by post-processing. This caused unix.Malloc checker to report its own diagnostics.

This sounds very fishy. I'd be shocked if not properly disabling core.StackAddressEscape would result in the sudden enabling of MallocChecker. This should only happen with modeling checkers that by design don't emit diagnostics.

edit: I see, this breaks test_fix_date_filters, where we explicitly check whether a check was disabled by CodeChecker's understanding. I again suggest that you document it somewhere why we have this nonsensical looking, but perfectly fine config.

Szelethus · 2023-08-15T09:18:28Z

web/tests/functional/report_viewer_api/test_report_filter.py

@@ -382,7 +382,7 @@ def test_detection_date_filters(self):
    def test_fix_date_filters(self):
        """ Filter by fix dates. """
        report_filter = ReportFilter(
-            detectionStatus=[DetectionStatus.RESOLVED])
+            detectionStatus=[DetectionStatus.OFF])


Can you provide more context for this change?

Similar issue than the previous one. It comes from the "disable vs. hide" semantic change. The "OFF" status means that the unix.Malloc checker has been disabled and that's the reason for the report being resolved. Earlier there was a unix.MismatchedDeallocator checker report which simply became resolved without the checker being disabled. I agree if it seems a little fuzzy. If you need more precise elaboration about the exact reasons, then I could take some more effort in its inspection once... maybe sometime... in the future... :)

Szelethus

I would like to see the 'frame-larger-than' warning having its own patch, as well as the profile changes.

I recall a long while, Artem gave me a stern talk about too large patches. They are sometimes unavoidable (like, you need to simultaneously patch clang-tidy AND clang-sa here), but those should be the exception, rather than the rule. A good rule of thumb might be the following -- each change that deserves its own discussion should deserver its own patch.

Szelethus

I still strongly disagree with not having a curtesy warning for larger-than-frame. Its a piece of information we immediately possess and are choosing to withhold for no reason, or at least it sure seems to have no good reason in my view.

I will accept that this is not the time or place to divide this PR into smaller chunks, but am rather unhappy about it. Its possible that we would have landed parts of this pach much sooner if it was more managable in scope (this last part alone consumed ~2 weeks). Having separate commits is nice (and helped me greatly!), but some parts of this PR would have definitely deserved its own PR as well.

I come from a different reviewer environment, so please let me know if I'm the one that needs to change to workflow, or at least lets try better to find a common ground. Also, I understand that 90-95% of this patch did indeed need to land in a single piece.

Granted that @dkrupp makes the call on the larger-than-frame discussion and my last few nits are addressed, this PR LGTM.

Szelethus · 2023-09-07T13:13:49Z

web/tests/functional/report_viewer_api/__init__.py

-                                   '-d', 'unix.Malloc'
+                                   '-d', 'unix.Malloc',
+                                   '-d', 'clang-diagnostic',
+                                   '-e', 'clang-diagnostic-division-by-zero'


Can you explain in the comments why we have this particular checker configuration?

Szelethus · 2023-09-07T13:26:46Z

web/tests/functional/report_viewer_api/__init__.py

@@ -114,7 +123,11 @@ def setup_class_common(workspace_name):
    print("Third analysis of the test project was successful.")

    # Let's run the second analysis and updat the same run.
-    codechecker_cfg['checkers'] = ['-d', 'core.StackAddressEscape']
+    codechecker_cfg['checkers'] = ['-d', 'core.StackAddressEscape',
+                                   '-d', 'unix.Malloc',


This sounds very fishy. I'd be shocked if not properly disabling core.StackAddressEscape would result in the sudden enabling of MallocChecker. This should only happen with modeling checkers that by design don't emit diagnostics.

edit: I see, this breaks test_fix_date_filters, where we explicitly check whether a check was disabled by CodeChecker's understanding. I again suggest that you document it somewhere why we have this nonsensical looking, but perfectly fine config.

Currently there are 3 kind of checkers regarding whether they are running during analysis: enabled, disabled, unknown. Checkers can be enabled if they are explicitly given to --enable flag, or they are the member of "default" profile. Checkers can also be explicitly disabled. The 3rd status is tricky: CodeChecker doesn't explicitly enable or disable them, but gives the choice to the analyzer tool. This behavior is bad, because a user cannot tell exactly which checkers were executed during analysis. It is impossible to determine in case of no error if a checker was disabled or just simply didn't report any issue. The goal in this commit is that every checker is either enabled or disabled. It is the analyzer module's responsibility to assemble an analysis command which executes at least the enabled checkers and turns off or hides the disabled ones.

Due to the previous commit every checker is either enabled or disabled. This resulted very long analyzer commands in ClangTidy because every checker was explicitly enabled or disabled in the clang-tidy invocation. ClangTidy has a lot of checkers and also all compiler warnings listed by diagtool are also considered as checkers. The goal of this commit is to simplify the clang-tidy invocation command. First we disable all checkers and enable only those that are necessary to run: clang-tidy -checks=-*,enabled1,enabled2... This patch takes into account that clang-diagnostic-... checkers don't turn on the corresponding warnings. They have to be enabled by -W... compiler flag.

ClangSA checkers may rely on each other. For example unix.Malloc checker may prevent a report from unix.MismatchedDeallocator. It is possible that after disabling unix.Malloc the underlying analysis continues giving a chance for unix.MismatchedDeallocator to emit a report. In order to avoid this noisy behavior, ClangSA checkers are never disabled during the analysis invocation. If CodeChecker wants to disable a checker then its result should be hidden in a post-processing step.

Szelethus

LGTM! While I've made my fair share of criticisms on some of the circumstances around this patch, overall, I'm satisfied with how it turned out. Great work!

bruntib · 2023-09-08T12:43:34Z

Thank you so much for the valuable review!

bruntib added this to the release 6.23.0 milestone Jul 10, 2023

bruntib requested a review from Szelethus July 10, 2023 10:57

bruntib requested a review from vodorok as a code owner July 10, 2023 10:57

bruntib force-pushed the no_unknown_checker_state branch from a132699 to 240dfe4 Compare July 10, 2023 13:09

Szelethus requested changes Jul 11, 2023

View reviewed changes

bruntib force-pushed the no_unknown_checker_state branch from 240dfe4 to fff8443 Compare July 17, 2023 13:00

bruntib requested a review from Szelethus July 17, 2023 13:03

bruntib force-pushed the no_unknown_checker_state branch from fff8443 to bc2aa30 Compare July 20, 2023 12:46

bruntib mentioned this pull request Jul 27, 2023

[bugfix] Don't trigger analyzer without enabled checkers #3970

Merged

Szelethus requested changes Jul 27, 2023

View reviewed changes

bruntib force-pushed the no_unknown_checker_state branch from bc2aa30 to 1e35f31 Compare July 27, 2023 12:28

bruntib requested a review from Szelethus July 27, 2023 12:29

bruntib force-pushed the no_unknown_checker_state branch from 1e35f31 to ea09ad0 Compare August 9, 2023 16:09

Szelethus mentioned this pull request Aug 14, 2023

[GUI] Jump directly to documentation url without error modal #3974

Merged

Szelethus reviewed Aug 14, 2023

View reviewed changes

Szelethus requested changes Aug 15, 2023

View reviewed changes

bruntib requested a review from Szelethus August 15, 2023 12:49

bruntib force-pushed the no_unknown_checker_state branch from ea09ad0 to 935d8a9 Compare August 21, 2023 07:55

Szelethus requested changes Aug 31, 2023

View reviewed changes

Szelethus reviewed Sep 7, 2023

View reviewed changes

bruntib added 2 commits September 8, 2023 13:38

bruntib force-pushed the no_unknown_checker_state branch from 935d8a9 to 2c5a326 Compare September 8, 2023 11:39

Szelethus approved these changes Sep 8, 2023

View reviewed changes

bruntib merged commit 27fac93 into Ericsson:master Sep 8, 2023
8 of 9 checks passed

bruntib deleted the no_unknown_checker_state branch September 8, 2023 12:50

Eliminate "unknown" checker state #3949

Eliminate "unknown" checker state #3949

Conversation

bruntib commented Jul 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szelethus commented Jul 11, 2023

bruntib commented Jul 17, 2023

Szelethus commented Jul 18, 2023

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

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

Szelethus left a 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

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

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

Szelethus left a comment

Choose a reason for hiding this comment

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

Szelethus left a comment

Choose a reason for hiding this comment

bruntib commented Sep 8, 2023

bruntib commented Jul 10, 2023 •

edited