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

[cfg] Update labels file for updated checkers #4383

Merged
merged 6 commits into from
Nov 27, 2024

Conversation

gamesh411
Copy link
Collaborator

Since the last release some ClangSA checkers were moved to different packages and some new checkers were introduced. This change updates the label configuration of CodeChecker to reflect this.

I just eyeballed the severities and the profiles, so it's open for suggestions.

Notes:
alpha.security.taint.TaintPropagation does not have a documentation link, so I just put the GenericTaintChecker link as doc-url. Alternatively, we could use doc entry: https://clang.llvm.org/docs/analyzer/user-docs/TaintAnalysisConfiguration.html

With this patch, all the taint-related checkers share the same severity and profiles as the now-removed alpha.security.taint.TaintPropagation.

I put BlockInCriticalSection in the default profile, as I have deemed it not too noisy, and the results were meaningful IMO.

Since the last release some ClangSA checkers were moved to different
packages and some new checkers were introduced. This change updates the
label configuration of CodeChecker to reflect this.
@gamesh411 gamesh411 force-pushed the checker-label-update branch from 41555b3 to 09fb4bd Compare November 22, 2024 11:38
@gamesh411 gamesh411 requested a review from dkrupp November 22, 2024 11:46
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

Thanks for the updates.
Some checkers might go to the default profile. Please the noisiness.

config/labels/analyzers/clang-tidy.json Show resolved Hide resolved
config/labels/analyzers/clang-tidy.json Show resolved Hide resolved
@@ -788,6 +812,13 @@
"profile:sensitive",
"severity:MEDIUM"
],
"cert-arr39-c": [
Copy link
Member

Choose a reason for hiding this comment

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

this checker is an alias of the bugprone-sizeof-expression.
To avoid duplicate findings, we just add the main checker to the profiles and the guidelines. So please move these to the bugprone-sizeof-expression.
"guideline:sei-cert",
"profile:security",
"sei-cert:arr39-c",

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moving there, this and the bugprone-sizeof-expression is also quite noisy

@@ -796,6 +827,13 @@
"doc_url:https://clang.llvm.org/extra/clang-tidy/checks/cert/con54-cpp.html",
"severity:MEDIUM"
],
"cert-ctr56-cpp": [
"doc_url:https://clang.llvm.org/extra/clang-tidy/checks/cert/ctr56-cpp.html",
Copy link
Member

Choose a reason for hiding this comment

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

this is just an alias checker please move teh guideline and sei-cert profile correspondence to
checker
bugprone-pointer-arithmetic-on-polymorphic-object

config/labels/analyzers/clang-tidy.json Show resolved Hide resolved
@@ -278,7 +273,7 @@
"profile:sensitive",
"severity:HIGH"
],
"alpha.unix.SimpleStream": [
"unix.SimpleStream": [
Copy link
Member

Choose a reason for hiding this comment

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

maybe to profile:default depending how noisy it is

@gamesh411 gamesh411 force-pushed the checker-label-update branch from 7c0ba25 to 28504b8 Compare November 25, 2024 16:07
@gamesh411 gamesh411 force-pushed the checker-label-update branch from 28504b8 to 1c93f14 Compare November 25, 2024 16:11
@gamesh411
Copy link
Collaborator Author

gamesh411 commented Nov 25, 2024

I have checked the reports for clangsa and tidy, and this is what I came up with.

Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

Please add back the severity and link documentation of

  • cert-arr39-c
  • cert-ctr56-cpp

checkers, but don't assign them to profiles.

I am hesitant about adding bugprone-return-const-ref-from-parameter and bugprone-pointer-arithmetic-on-polymorphic-object to the default profile.

config/labels/analyzers/clang-tidy.json Show resolved Hide resolved
config/labels/analyzers/clang-tidy.json Show resolved Hide resolved
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

LGTM

@dkrupp dkrupp merged commit 53655b1 into Ericsson:master Nov 27, 2024
8 of 9 checks passed
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.

2 participants