Small cppcheck fixes and refactor GitLab cppcheck job #3261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rectify cases of mismatched brackets and address possible cases of division by zero by checking if the denominator is zero before dividing. No functional changes were made. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
Line numbers had to be removed from the ignorelists in order to be diffed against since locations of the same findings can differ across runs. Therefore preprocessing has to be done on the CI findings so that it can be compared to the ignorelist and new findings can be outputted. However, since line numbers have to be removed, a situation occurs where it is difficult to reference the location of findings in code given the output of the CI job. To lessen this pain, change the cppcheck template to include code snippets which make it easier to reference where in the code the finding is referring to, even in the absence of line numbers. Ignorelisting works as before since locations of the finding may change but not the code it is referring to. Furthermore, due to the innate difficulty in maintaining ignorelists across branches and triaging new findings, allow failure as to not have constantly failing pipelines as a result of a new findings that have not been addressed yet. Lastly, update SAST ignorelists to match the newly refactored cppcheck job and the current state of the codebase. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
|
|
LinuxJedi
requested changes
May 17, 2024
There was a problem hiding this comment.
Some good catches here. I'm not 100% sure the my_rdtsc.c fixes are correct, I'll leave that for the final reviewer.
Unfortunately it doesn't compile in some of the buildbot testers:
/home/buildbot/amd64-debian-11-msan/build/mysys/my_rdtsc.c:352:13: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
ulonglong denominator = ((time3 - time2) == 0)? 1 : time3 - time2;
^
/home/buildbot/amd64-debian-11-msan/build/mysys/my_rdtsc.c:616:15: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
ulonglong denominator = ((time4 - time1) == 0)? 1 : (time4 - time1);
^
/home/buildbot/amd64-debian-11-msan/build/mysys/my_rdtsc.c:646:15: error: mixing declarations and code is incompatible with standards before C99 [-Werror,-Wdeclaration-after-statement]
ulonglong denominator = ((time4 - time1) == 0)? 1 : (time4 - time1);
^
| @@ -37,7 +37,7 @@ my_crc32_t crc32c_aarch64_available(void) | |||
| static unsigned long getauxval(unsigned int key) | |||
| { | |||
| unsigned long val; | |||
| if (elf_aux_info(key, (void *)&val, (int)sizeof(val) != 0) | |||
| if (elf_aux_info(key, (void *)&val, (int)sizeof(val) != 0)) | |||
There was a problem hiding this comment.
Good catch, it gives concern that we never compile this code.
| @@ -349,7 +349,8 @@ static ulonglong my_timer_init_frequency(MY_TIMER_INFO *mti) | |||
| } | |||
| time4= my_timer_cycles() - mti->cycles.overhead; | |||
| time4-= mti->microseconds.overhead; | |||
| return (mti->microseconds.frequency * (time4 - time1)) / (time3 - time2); | |||
| ulonglong denominator = ((time3 - time2) == 0)? 1 : time3 - time2; | |||
There was a problem hiding this comment.
If this happens then the computer's RTC is broken / missing, or the for loop got optimized out somehow. Either way, there are much bigger problems. Not sure what the correct behaviour here should be to honest.
| @@ -612,8 +613,9 @@ void my_timer_init(MY_TIMER_INFO *mti) | |||
| if (time3 - time2 > 10) break; | |||
| } | |||
| time4= my_timer_cycles(); | |||
| ulonglong denominator = ((time4 - time1) == 0)? 1 : (time4 - time1); | |||
There was a problem hiding this comment.
As my previous comment, not sure what the correct behaviour should be here.
| } | ||
| #endif |
| @@ -549,6 +549,7 @@ int ha_s3::create(const char *name, TABLE *table_arg, | |||
| s3_deinit(s3_client); | |||
| if (error) | |||
| maria_delete_table_files(name, 1, 0); | |||
| } | |||
There was a problem hiding this comment.
Good catch, this code is never compiled. I think it was for testing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR makes changes to the existing cppcheck GitLab CI job in order to make it easier to address new/existing findings. Existing ignorelists are also updated to reflect this and the current state of the codebase. Also, some trivial issues found by cppcheck are addressed.
Refactor GitLab cppcheck job
Small cppcheck fixes
Release Notes
N/A
How can this PR be tested?
Code changes are non-functional and
./mtr --suite=mainpasses successfully.Basing the PR against the correct MariaDB version
PR quality check
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.