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

Handle ambigous profile/group parameters #4377

Merged
merged 2 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions analyzer/codechecker_analyzer/analyzers/config_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@

from abc import ABCMeta
from enum import Enum
from string import Template
import collections
import platform
import sys
import re

from codechecker_analyzer import analyzer_context
Expand Down Expand Up @@ -149,10 +151,12 @@ def initialize_checkers(self,
and "debug" checker groups. "osx" checker group is also not included
unless the target platform is Darwin.
- Command line "--enable/--disable" flags.
- Their arguments may start with "profile:" or "guideline:" prefix
which makes the choice explicit.
- Without prefix it means a profile name, a guideline name or a
checker group/name in this priority order.
- Their arguments may start with "prefix:", "profile:" or
"guideline:" label which makes the choice explicit.
- Without a label, it means either a profile name, a guideline name
or a checker prefix group/name, unless there is a clash
between these names, in which case an error is emitted.
If there is a name clash, using a label is mandatory.

checkers -- [(checker name, description), ...] Checkers to add with
their description.
Expand Down Expand Up @@ -210,23 +214,41 @@ def initialize_checkers(self,
profiles = checker_labels.get_description('profile')
guidelines = checker_labels.occurring_values('guideline')

templ = Template("The ${entity} name '${identifier}' conflicts with a "
"checker name prefix '${identifier}'. Please use -e "
"${entity}:${identifier} to enable checkers of the "
"${identifier} ${entity} or use -e "
"prefix:${identifier} to select checkers which have "
"a name starting with '${identifier}'.")

for identifier, enabled in cmdline_enable:
if ':' in identifier:
if "prefix:" in identifier:
identifier = identifier.replace("prefix:", "")
self.set_checker_enabled(identifier, enabled)

elif ':' in identifier:
for checker in checker_labels.checkers_by_labels([identifier]):
self.set_checker_enabled(checker, enabled)

elif identifier in profiles:
if identifier in reserved_names:
LOG.warning("Profile name '%s' conflicts with a "
"checker(-group) name.", identifier)
for checker in checker_labels.checkers_by_labels(
[f'profile:{identifier}']):
self.set_checker_enabled(checker, enabled)
LOG.error(templ.substitute(entity="profile",
identifier=identifier))
sys.exit(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is exit status 1 the proper exit code in case of configuration errors?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same applies 10 lines below

Copy link
Member

Choose a reason for hiding this comment

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

I think yes. These are the exit code listed by CodeChecker analyze --help
Exit status

0 - Successful analysis
1 - CodeChecker error
3 - Analysis of at least one translation unit failed
128+signum - Terminating on a fatal signal whose number is signum

else:
for checker in checker_labels.checkers_by_labels(
[f'profile:{identifier}']):
self.set_checker_enabled(checker, enabled)

elif identifier in guidelines:
if identifier in reserved_names:
LOG.warning("Guideline name '%s' conflicts with a "
"checker(-group) name.", identifier)
for checker in checker_labels.checkers_by_labels(
[f'guideline:{identifier}']):
self.set_checker_enabled(checker, enabled)
LOG.error(templ.substitute(entity="guideline",
identifier=identifier))
sys.exit(1)
else:
for checker in checker_labels.checkers_by_labels(
[f'guideline:{identifier}']):
self.set_checker_enabled(checker, enabled)

else:
self.set_checker_enabled(identifier, enabled)
3 changes: 2 additions & 1 deletion analyzer/codechecker_analyzer/checkers.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ def available(ordered_checkers, available_checkers):
if checker_name.startswith('profile:') or \
checker_name.startswith('guideline:') or \
checker_name.startswith('severity:') or \
checker_name.startswith('sei-cert:'):
checker_name.startswith('sei-cert:') or \
checker_name.startswith('prefix:'):
continue

name_match = False
Expand Down
56 changes: 41 additions & 15 deletions analyzer/codechecker_analyzer/cmd/analyze.py
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,22 @@ def add_arguments_to_parser(parser):
Note that compiler errors and warnings are captured by CodeChecker only if it
was emitted by clang-tidy.

Checker prefix groups
------------------------------------------------
Checker prefix groups allow you to enable checkers that share a common
prefix in their names. Checkers within a prefix group will have names that
start with the same identifier, making it easier to manage and reference
related checkers.

You can enable/disable checkers belonging to a checker prefix group:
'-e <label>:<value>', e.g. '-e prefix:security'.

Note: The 'prefix' label is mandatory when there is ambiguity between the
name of a checker prefix group and a checker profile or a guideline. This
prevents conflicts and ensures the correct checkers are applied.

See "CodeChecker checkers --help" to learn more.

Checker labels
------------------------------------------------
Each checker is assigned several '<label>:<value>' pairs. For instance,
Expand All @@ -708,6 +724,10 @@ def add_arguments_to_parser(parser):
You can enable/disable checkers belonging to a label: '-e <label>:<value>',
e.g. '-e profile:default'.

Note: The 'profile' label is mandatory when there is ambiguity between the
name of a checker profile and a checker prefix group or a guideline. This
prevents conflicts and ensures the correct checkers are applied.

See "CodeChecker checkers --help" to learn more.

Guidelines
Expand All @@ -723,6 +743,10 @@ def add_arguments_to_parser(parser):
Guidelines are labels themselves, and can be used as a label:
'-e guideline:<value>', e.g. '-e guideline:sei-cert'.

Note: The 'guideline' label is mandatory when there is ambiguity between the
name of a guideline and a checker prefix group or a checker profile. This
prevents conflicts and ensures the correct checkers are applied.

Batch enabling/disabling checkers
------------------------------------------------
You can fine-tune which checkers to use in the analysis by setting the enable
Expand All @@ -739,35 +763,37 @@ def add_arguments_to_parser(parser):
metavar='checker/group/profile',
default=argparse.SUPPRESS,
action=OrderedCheckersAction,
help="Set a checker (or checker group), "
"profile or guideline "
"to BE USED in the analysis. In case of "
"ambiguity the priority order is profile, "
"guideline, checker name (e.g. security "
"means the profile, not the checker "
"group). Moreover, labels can also be "
help="Set a checker (or checker prefix group), "
"profile or guideline to BE USED in the "
"analysis. Labels can also be "
"used for selecting checkers, for example "
"profile:extreme or severity:STYLE. See "
"'CodeChecker checkers --label' for "
"further details.")
"further details. In case of a name clash "
"between the checker prefix "
"group/profile/guideline name, the use of "
"one of the following labels is "
"mandatory: 'prefix:', 'profile:', "
"'guideline:'.")

checkers_opts.add_argument('-d', '--disable',
dest="disable",
metavar='checker/group/profile',
default=argparse.SUPPRESS,
action=OrderedCheckersAction,
help="Set a checker (or checker group), "
help="Set a checker (or checker prefix group), "
"profile or guideline "
"to BE PROHIBITED from use in the "
"analysis. In case of "
"ambiguity the priority order is profile, "
"guideline, checker name (e.g. security "
"means the profile, not the checker "
"group). Moreover, labels can also be "
"analysis. Labels can also be "
"used for selecting checkers, for example "
"profile:extreme or severity:STYLE. See "
"'CodeChecker checkers --label' for "
"further details.")
"further details. In case of a name clash "
"between the checker prefix "
"group/profile/guideline name, the use of "
"one of the following labels is "
"mandatory: 'prefix:', 'profile:', "
"'guideline:'.")

checkers_opts.add_argument('--enable-all',
dest="enable_all",
Expand Down
24 changes: 24 additions & 0 deletions analyzer/codechecker_analyzer/cmd/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,22 @@ def add_arguments_to_parser(parser):
Note that compiler errors and warnings are captured by CodeChecker only if it
was emitted by clang-tidy.

Checker prefix groups
------------------------------------------------
Checker prefix groups allow you to enable checkers that share a common
prefix in their names. Checkers within a prefix group will have names that
start with the same identifier, making it easier to manage and reference
related checkers.

You can enable/disable checkers belonging to a checker prefix group:
'-e <label>:<value>', e.g. '-e prefix:security'.

Note: The 'prefix' label is mandatory when there is ambiguity between the
name of a checker prefix group and a checker profile or a guideline. This
prevents conflicts and ensures the correct checkers are applied.

See "CodeChecker checkers --help" to learn more.

Checker labels
------------------------------------------------
Each checker is assigned several '<label>:<value>' pairs. For instance,
Expand All @@ -652,6 +668,10 @@ def add_arguments_to_parser(parser):
You can enable/disable checkers belonging to a label: '-e <label>:<value>',
e.g. '-e profile:default'.

Note: The 'profile' label is mandatory when there is ambiguity between the
name of a checker profile and a checker prefix group or a guideline. This
prevents conflicts and ensures the correct checkers are applied.

See "CodeChecker checkers --help" to learn more.

Guidelines
Expand All @@ -667,6 +687,10 @@ def add_arguments_to_parser(parser):
Guidelines are labels themselves, and can be used as a label:
'-e guideline:<value>', e.g. '-e guideline:sei-cert'.

Note: The 'guideline' label is mandatory when there is ambiguity between the
name of a guideline and a checker prefix group or a checker profile. This
prevents conflicts and ensures the correct checkers are applied.

Batch enabling/disabling checkers
------------------------------------------------
You can fine-tune which checkers to use in the analysis by setting the enable
Expand Down
81 changes: 76 additions & 5 deletions analyzer/tests/unit/test_checker_handling.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,17 @@ def checkers_by_labels(self, labels):
if labels[0] == 'profile:default':
return ['core', 'deadcode', 'security.FloatLoopCounter']

if labels[0] == 'prefix:security':
return ['security.insecureAPI.bzero',
'security.insecureAPI.getpw']

if labels[0] == 'profile:security':
return ['alpha.security']

if labels[0] == 'profile:sensitive':
return ['alpha.core.BoolAssignment',
'alpha.core.TestAfterDivZero']

if labels[0] == 'guideline:sei-cert':
return ['alpha.core.CastSize', 'alpha.core.CastToStruct']

Expand Down Expand Up @@ -154,6 +162,17 @@ def f(checks, checkers):
'alpha.security.ArrayBound',
'alpha.security.MallocOverflow']

# "security" checker prefix group
security_prefix = [
'security.insecureAPI.bzero',
'security.insecureAPI.getpw'
]

# "sensitive" profile, but alpha -> not in default.
sensitive_profile_alpha = [
'alpha.core.BoolAssignment',
'alpha.core.TestAfterDivZero']

# "default" profile.
default_profile = [
'security.FloatLoopCounter',
Expand All @@ -175,6 +194,8 @@ def f(checks, checkers):

checkers = []
checkers.extend(map(add_description, security_profile_alpha))
checkers.extend(map(add_description, security_prefix))
checkers.extend(map(add_description, sensitive_profile_alpha))
checkers.extend(map(add_description, default_profile))
checkers.extend(map(add_description, cert_guideline))
checkers.extend(map(add_description, statisticsbased))
Expand All @@ -197,15 +218,41 @@ def f(checks, checkers):
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), default_profile))

# Enable alpha checkers explicitly with prefix label.
# Using the "prefix" label is optional in this case, because the
# checker group name "alpha" does not conflict with any checker
# profiles or guidelines.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('prefix:alpha', True)])
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), security_profile_alpha))
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), default_profile))

# Enable alpha checkers explicitly.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers, [('alpha', True)])
cfg_handler.initialize_checkers(checkers,
[('alpha', True)])
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), security_profile_alpha))
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), default_profile))

# Enable checkers of the "security" checker prefix group.
# Using the "prefix" label is mandatory in this case, because the
# checker group name "security" conflicts with a profile name.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('prefix:security', True)])
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), security_prefix))
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), default_profile))

# Enable "security" profile checkers.
# Using the "profile" label is mandatory in this case, because the
# profile name "security" conflicts with a checker group name.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('profile:security', True)])
Expand All @@ -215,22 +262,46 @@ def f(checks, checkers):
(cfg_handler.checks(), default_profile))

# Enable "security" profile checkers without "profile:" prefix.
# This should throw an error, because the profile name "security"
# conflicts with a checker group name.
cfg_handler = ClangSA.construct_config_handler(args)
with self.assertRaises(SystemExit) as e:
cfg_handler.initialize_checkers(checkers,
[('security', True)])
self.assertEqual(e.exception.code, 1)

# Enable "sensitive" profile checkers with the "profile:" label.
# Using the "profile" label is optional in this case, because the
# profile name "sensitive" does not conflict with any checker
# prefix group names.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('security', True)])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you replacing the security prefix? enabling with sensitive?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After this change is introduced, "-e security" should throw an error as "security" is a profile name that conflicts with a checker group name. However, "-e sensitive" should continue to function as it has previously, because the "sensitive" profile name does not conflict with any checker group names. This is what I intend to test here.

[('profile:sensitive', True)])
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), security_profile_alpha))
(cfg_handler.checks(), sensitive_profile_alpha))
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), default_profile))

# Enable "sensitive" profile checkers without the "profile:" label.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('sensitive', True)])
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), sensitive_profile_alpha))
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), default_profile))

# Enable "sei-cert" guideline checkers.
# Enable "sei-cert" guideline checkers with the "guideline:" label.
# Using the "guideline" label is optional in this case, because the
# guideline name "sei-cert" does not conflict with any checker
# prefix group names.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('guideline:sei-cert', True)])
self.assertTrue(all_with_status(CheckerState.ENABLED)
(cfg_handler.checks(), cert_guideline))

# Enable "sei-cert" guideline checkers.
# Enable "sei-cert" guideline checkers without the "guideline:" label.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('sei-cert', True)])
Expand Down
Loading
Loading