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

[config] Label Clang 18.0-19.trunk SA & Tidy checkers #4193

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

sylvestre
Copy link
Contributor

No description provided.

@sylvestre
Copy link
Contributor Author

Failure is this one but not sure why it is my fault:

Run set +e # Do not hard exit on an erroring call!
Checkers REMOVED from upstream, with CodeChecker still assigning labels:
Skipped! Specify '--report-removed' to execute.

Checkers ADDED to upstream, without labels in CodeChecker:
 + bugprone-chained-comparison
 + bugprone-crtp-constructor-accessibility
 + bugprone-unused-local-non-trivial-variable
 + cppcheck-constParameterPointer
 + cppcheck-constParameterReference
 + cppcheck-constVariablePointer
 + cppcheck-constVariableReference
 + cppcheck-eraseIteratorOutOfBounds
 + cppcheck-eraseIteratorOutOfBoundsCond
 + cppcheck-knownPointerToBool
 + cppcheck-pointerOutOfBoundsCond
 + hicpp-ignored-remove-result
 + modernize-use-designated-initializers
 + optin.core.EnumCastOutOfRange
 + readability-use-std-min-max
Coverage check returned: 8.
Error: Process completed with exit code 8.

@whisperity whisperity added analyzer 📈 Related to the analyze commands (analysis driver) clang-tidy 🐉 clang-tidy is a clang-based C++ “linter” tool. config ⚙️ labels Mar 18, 2024
@whisperity
Copy link
Contributor

whisperity commented Mar 18, 2024

Failure is this one but not sure why it is my fault:

@sylvestre You changed a label configuration file, so the CI job that checks whether the label configuration file covers the nightly build of the respective analysers were executed.

For example, it caught that you made a typo in the config file, as even though it looks like you added a record for one of the checkers in that list, you actually did not! 🙂

@whisperity
Copy link
Contributor

@NagyDonat @Discookie Can you please describe the cplusplus.ArrayDelete checker for us? It went out of alpha following llvm/llvm-project#83985, but we did NOT have a label/guideline/documentation/severity/etc. configuration for the check's alpha version, and now should add one for the non-alpha one. What are your suggestions?

@whisperity whisperity changed the title Document new clang-tidy checkers [config] Label Clang 18.0-19.trunk SA & Tidy checkers Mar 27, 2024
@whisperity whisperity added the clang sa 🐉 The Clang Static Analyzer is a source code analysis tool that finds bugs in C-family programs. label Mar 27, 2024
@whisperity
Copy link
Contributor

@NagyDonat @gamesh411 Same comment for optin.core.EnumCastOutOfRange, following llvm/llvm-project#67157 & llvm/llvm-project#68191. For the previous alpha.cplusplus.EnumCastOutOfRange, we had

    "alpha.cplusplus.EnumCastOutOfRange": [
      "doc_url:https://clang.llvm.org/docs/analyzer/checkers.html#alpha-cplusplus-enumcastoutofrange-c",
      "profile:extreme",
      "profile:sensitive",
      "severity:MEDIUM"
    ],

@NagyDonat
Copy link

cplusplus.ArrayDelete is documented at https://clang.llvm.org/docs/analyzer/checkers.html#cplusplus-arraydelete-c and it is specific to C++. I'm not familiar with the meaning of the potential severity and profile values in codechecker, so I cannot help you with that. Maybe @gamesh411 could categorize these checkers in that respect?

@whisperity
Copy link
Contributor

I'm not familiar with the meaning of the potential severity and profile values in codechecker, so I cannot help you with that.

@NagyDonat These are the official descriptions for these values. I'm not inherently sure how strictly we adhere to them.

"profile": {
"default": "High-quality standard checks with a low false positive rate.",
"sensitive": "Default checks + more comprehensive checks with a low false positive rate.",
"security": "Checkers that warn for potentially vulnerable code. Contains all SEI Cert C/C++ rules.",
"portability": "Checks that aim to detect code issues emerging from platform differences (eg. between 32-bit and 64-bit architectures).",
"extreme": "Sensitive checks + more comprehensive checks with a manageable false positive rate."
},
"severity": {
"CRITICAL": "Currently used for indicating compilation errors.",
"HIGH": "A true positive indicates that the source code will cause a run-time error.",
"MEDIUM": "A true positive indicates that the source code that may not cause a run-time error (yet), but against intuition and hence prone to error.",
"LOW": "A true positive indicates that the source code is hard to read/understand or could be easily optimized.",
"STYLE": "A true positive indicates that the source code is against a specific coding guideline or could improve readability.",
"UNSPECIFIED": "Checker severity is not specified for a checker."
},

Basically the crucial issue about adding something to profile:default is that CodeChecker analyze will run them by default (hence the name), which is bad if the checker is throwing copious amounts of false positives or crashing left, right, and centre. The worse the checker is (in terms of user experience and correctness), the more it is pushed out on the default -> sensitive -> extreme spectrum.

Is it good to have this checker enabled by default for users? It's not part of core. but it sounds reasonable. Are the results sensible?

What happens if we default-enable a cplusplus.Foo CSA checker on a C source file?

@gamesh411
Copy link
Collaborator

EnumCastOutOfRange:
When alpha.cplusplus.EnumCastOutOfRange became optin.core.EnumCastOutOfRange, the strictness of the checker was weakened. The part from the MEDIUM severity description still holds, though: "...may not cause a run-time error (yet), but against intuition and hence prone to error...". Therefore, I think it can be MEDIUM severity. However, as it does not support another major use case for enums (using them as flags), I would not put it in the default profile. Let's keep it in the sensitive and extreme profiles.

ArrayDelete:
I did not participate much in the development of this checker, but this error causes heap corruption at the very least. I would put it in HIGH severity. The check did not produce any findings for our OS project test set, so it is not noisy either. IMO, we should put it in the default category.

@whisperity
Copy link
Contributor

@gamesh411 Thanks! I'll update the patch soon and then I think we will be done with adding the new classifications!

@whisperity whisperity merged commit 0aa460f into Ericsson:master Apr 25, 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
analyzer 📈 Related to the analyze commands (analysis driver) clang sa 🐉 The Clang Static Analyzer is a source code analysis tool that finds bugs in C-family programs. clang-tidy 🐉 clang-tidy is a clang-based C++ “linter” tool. config ⚙️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants