More cppcheck fixes #4686

cujomalainey · 2021-08-26T23:59:18Z

No description provided.

cujomalainey · 2021-08-27T00:05:52Z

Looks like I have some yaml errors, will fixup to get cppcheck then will mark ready

singalsu

Approve for dmic_nhlt.c, thanks!

lgirdwood

@cujomalainey thanks, good to have this check.

.github/workflows/codestyle.yml

marc-hb · 2021-08-27T16:53:55Z

src/audio/multiband_drc/multiband_drc.c

Can we use C99 declarations+initializations now? There was no significant objection in
https://github.com/orgs/thesofproject/teams/steering-committee/discussions/25

I think we need to make a final call on that in a TSC, hasn't been one in a while.

marc-hb · 2021-08-27T17:00:51Z

One possibility is to run it only on modified files. This makes it much, much easier to ignore false positives.

This is how it's done (manually...) in sof-test: https://github.com/thesofproject/sof-test/blob/main/tools/CI/check-gitrange.bash

Zephyr uses a Github Action instead:
uses: trilom/file-changes-action@v1.2.3
https://github.com/trilom/file-changes-action/tree/master/src
https://github.com/zephyrproject-rtos/test_results/blob/master/.github/workflows/check_results.yml

cujomalainey · 2021-08-27T19:33:44Z

One possibility is to run it only on modified files. This makes it much, much easier to ignore false positives.

I can take a look, but it might not work given that this needs visibility of all headers and such to be able to infer properly. We might have to go with the suppression method.

marc-hb · 2021-08-29T23:12:01Z

I can take a look, but it might not work given that this needs visibility of all headers and such to be able to infer properly.

Neither the sof-test script nor the github action I gave "hide" anything. They only extract a list of modified files that can be passed as arguments to cppcheck. That does not stop cppcheck from looking at any other header or other file it needs.

PS: any idea why I get only the init.c warning on the files fixed by this PR and not the other warnings? I'm using the same options with cppcheck 1.90

We might have to go with the suppression method.

From past experience that can become very painful very quickly... not familiar with cppcheck but static checkers always have a large and highly variable number of false positives - especially C checkers because C is so baroque and the pre-processor even more.

cujomalainey · 2021-09-01T23:23:56Z

PS: any idea why I get only the init.c warning on the files fixed by this PR and not the other warnings? I'm using the same options with cppcheck 1.90

I am using 2.3/4 i think, also the flags you pass can vary the results greatly, not all of these were identified with the same flags I am building into the CI. Some are warnings, not errors.

We might have to go with the suppression method.

From past experience that can become very painful very quickly... not familiar with cppcheck but static checkers always have a large and highly variable number of false positives - especially C checkers because C is so baroque and the pre-processor even more.

Given the number of bugs I have filed against upstream in the short time I have worked on this I would agree, they are broken and yield many false positives, which is why I have this only turned on for the most serious of errors currently, to reduce the noise. That being said I have found "notices" that were extremely concerning memory leaks.

marc-hb · 2021-09-03T06:38:03Z

which is why I have this only turned on for the most serious of errors currently, to reduce the noise.

In fact checking only the files modified in the PR allows turning many more checks on.

Running fewer checks on ALL files is best done as a nightly build, not as a PR check.

cujomalainey · 2021-09-03T21:10:27Z

which is why I have this only turned on for the most serious of errors currently, to reduce the noise.

In fact checking only the files modified in the PR allows turning on many more checks.

Running fewer checks on ALL files is best done as a nightly build, not as a PR check.

So errors on push for all files, all for only files touched by the PR on PR?

cujomalainey · 2021-09-03T21:33:18Z

which is why I have this only turned on for the most serious of errors currently, to reduce the noise.

In fact checking only the files modified in the PR allows turning on many more checks.
Running fewer checks on ALL files is best done as a nightly build, not as a PR check.

So errors on push for all files, all for only files touched by the PR on PR?

For reference I will start with error on the second, there are simply way too many warning/info/style issues to open up that fire hose right now

we should never need it, but this is just to silence cppcheck Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

cppcheck error Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

cppcheck pointed out that either our nullpointer check was not needed or that we are failing to do it before using the pointer. The check looks valid so lets fix the lookup. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

cujomalainey · 2021-09-21T23:47:49Z

Also to all the die hard C fans here, I apologize for my sass in my commits against C, I have been learning rust in my spare time and am quickly falling in love.

conditionally used variables should be conditionally compiled in Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

minor style violation, these should match exactly Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

ipc_free is a function, we should avoid shadowing in the event we tried to use the function here. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

marc-hb · 2021-09-22T02:50:10Z

I have punted the CI commit to my cppcheck-ci-enable branch as ubuntu 20.04 seems to be far behind running pre cppcheck 2.0.

I found no Github plan to provide a more recent Ubuntu version any time soon https://github.com/actions/virtual-environments

The recommendation is to use Docker and that's what we already do for other jobs. Did you consider that?

I apologize for my sass in my commits against C,

I was looking forward to some cheap shots but didn't find any :-(

cujomalainey · 2021-09-22T18:30:48Z

I have punted the CI commit to my cppcheck-ci-enable branch as ubuntu 20.04 seems to be far behind running pre cppcheck 2.0.

I found no Github plan to provide a more recent Ubuntu version any time soon https://github.com/actions/virtual-environments

The recommendation is to use Docker and that's what we already do for other jobs. Did you consider that?

Nope, I will look into that

I apologize for my sass in my commits against C,

I was looking forward to some cheap shots but didn't find any :-(

See the crossover commit

.github changes removed

marc-hb · 2021-09-22T20:39:56Z

See the crossover commit

Oops, missed it.

Integer promotion rules give me a headache every time I have to look them up.

For even more fun try this:

--- a/src/arch/xtensa/CMakeLists.txt
+++ b/src/arch/xtensa/CMakeLists.txt
@@ -124,7 +124,7 @@ target_compile_options(sof_options INTERFACE ${stdlib_flag} -fno-inline-function
 target_compile_options(sof_options INTERFACE
 	$<$<COMPILE_LANGUAGE:C>:
 		-${optimization_flag} -g
-		-Wall -Werror
+		-Wall -Wsign-conversion -Wsign-compare
 		-Wl,-EL
 		-Wmissing-prototypes
 		-Wpointer-arith

lgirdwood · 2021-09-23T10:28:26Z

Just one doxygen item to fix

[0/1] cd /home/runner/work/sof/sof/docbuild && doxygen sof.doxygen
/home/runner/work/sof/sof/src/include/sof/ipc/common.h:183: error: argument 'hdr' of command @param is not found in the argument list of ipc_cmd(ipc_cmd_hdr *_hdr) (warning treated as error, aborting now)
FAILED: CMakeFiles/doc /home/runner/work/sof/sof/docbuild/CMakeFiles/doc 
cd /home/runner/work/sof/sof/docbuild && doxygen sof.doxygen

marc-hb · 2021-09-23T17:16:31Z

src/include/sof/ipc/common.h

 * @param hdr Points to the IPC command header.
 */
-void ipc_cmd(ipc_cmd_hdr *hdr);
+void ipc_cmd(ipc_cmd_hdr *_hdr);


Just curious what is the warning here.

That the signature in the header didnt match the implementation signature in the c files

See https://github.com/thesofproject/sof/blob/main/src/ipc/ipc3/handler.c#L1513

I disagree 100% with this check. I don't care about this particular line but I think this check should be turned off.

The function signature is the same otherwise the code would not compile. That's how the compiler reports an actual signature mismatch:

sof/src/ipc/ipc3/handler.c:1513:6: error: conflicting types for 'ipc_cmd' 1513 | void ipc_cmd(ipc_cmd_hdr *_hdr) | ^~~~~~~ src/include/sof/ipc/common.h:191:6: note: previous declaration of 'ipc_cmd' was here 191 | void ipc_cmd(ipc_cmd_hdr **hdr); | ^~~~~~~

Parameter names in function declarations are totally optional and purely informative.

Having slightly different parameter names between the declaration and the definition is a very useful C feature. For instance:

Slightly longer and more descriptive parameter names in the function declaration because there is no context.

Slightly shorter parameter names in the function definition because there is all the context and repeating long symbol names over and over in the function body makes the code more verbose and less readable.

In this particular case neither _hdr nor hdr adds any information. The difference between the two serves a different purpose here: the underscore _ has a very specific and fairly clear meaning in the function code that makes no sense in the function declaration.

Again I don't care about this particular line but it's a good example of how counter-productive this check is.

This was a style level violation, the downside is some cppcheck checks that are low level informative/style are actually reporting serious issues such as memory leaks. We can disable this specific check, but style as a general thing should be on kept I think.

Why on earth would a developer make a conscious attempt to name things differently in the header and the implementation? It's permitted by the standard but it's really stupid.

I gave two examples above.

look at the cleanups I did for the kernel with 100+ of such changes.

I will try to find a few. How long ago?

flag additional issues with redundant inits and NULL pointer dereferences

For now I really can't imagine how something ignored by all compilers can cause problems that bad.

static analysis is always right.

I wish.

Did you read my point: it's a sign that the code developers did not maintain consistency between different parts of the code, and a likely sign of more errors. So when we see such problems, we also look at the rest of the function. it's the same with initialized variables or duplicate initialization, usually that means error handling is not done right.

I don't see what's objectionable about this. It's not like the SOF code is perfect or even good.

Another example: it's perfectly legal to have shadowed variables because the compiler knows what to do. And yet this is a real problem leading to bugs.

Did you read my point: it's a sign that the code developers did not maintain consistency between different parts of the code, and a likely sign of more errors. So when we see such problems, we also look at the rest of the function.

I did miss that indirection sorry. So this is like looking for typos and then performing a second code review pass where the typo was found. I'm not surprised at all that a second pass of code review finds bugs considering how rushed the first pass can be but is this really the most efficient to find problems and does this find the most important issues? How about - for instance - spending crucially limited time and expertise raising the bar on the first code review pass instead?

There is a gazillion of analyzers and checks that go beyond compilers to try and correct C limitations and design issues. Turning them all at once usually drowns important issues in an ocean of minor or irrelevant issues or false positives. I don't even care so much about that particular check actually but I care a lot about finding the right balance of cppcheck settings so that @cujomalainey can finally enable it in CI, something he hasn't been able to do because of the aforementioned "ocean" as he stated above, see Aug. 26th comments. For evidence that I care a 5 seconds look at https://github.com/thesofproject/sof/commits/main/.github is enough.

So will enabling this particular parameter name check in CI help detect some real problems? I can't imagine any for the moment but I'd love to be proven wrong.

because the compiler knows what to do.

I think there is a big difference between code and comments ignored by the compiler. Both code and comments should be good (see the many doxygen and comment fixes I made in #4509, #4441, #4106, #3912, #2626,...) but there's a clear line between code and comments. Parameter names in declarations are not code, they're (important!) comments.

cppcheck is still to verbose to be used for CI, specifically it does not work well with unions and our friends at AMD used unions. Coverity is the same, too verbose.

It's more of a maintainer tool that is run on a e.g. weekly basis - and before releases as we didn't do for 1.9... Oh well.

I've seen the same warning - mismatched parameter name in declaration and definition without cppcheck. I think the compiler checks it itself (maybe enabled by some flags, or added in some versions), or maybe it was one of the tools, used during the kernel / SOF / Zephyr build. I wasn't using anything special for sure.

paulstelian97 · 2021-09-27T08:19:09Z

src/audio/crossover/crossover.c

 *	   -EINVAL if not found.
 */
-static uint8_t crossover_get_stream_index(struct sof_crossover_config *config,
+static int crossover_get_stream_index(struct sof_crossover_config *config,


Someone needs to check that all calls to this are error-checked now, hopefully there's no hidden error but just in case.

paulstelian97

Makes sense, good enough for me.

plbossart

static analysis is always right.

lgirdwood · 2021-09-29T15:55:16Z

@cujomalainey are you able to align the doxygen comment? We are good to go once this is done. Thanks !

lyakh · 2021-10-01T07:15:32Z

src/drivers/intel/dmic/dmic_nhlt.c

 	uint32_t fir_control;
 	uint32_t pdm_ctrl_mask;
-	uint32_t ref;
+	uint32_t ref = 0;


ref can be uninitialised below if neither DMIC_IPM_VER1 nor DMIC_IPM_VER2 is defined. Probably this cannot happen, so it isn't really important what happens then, but in case there is such a possibility, with this initialisation we'd probably hit an error:

if (ref != val) { dai_err(dai, "dmic_set_config_nhlt(): illegal OUTCONTROL%d = 0x%08x", n, val); return -EINVAL; }

Again - probably unimportant, but just to be aware of.

lyakh · 2021-10-01T07:27:13Z

src/drivers/intel/dmic/dmic_computed.c


 			/* Skip if previous decimation factor was the same */
-			if (j > 1 && fir_list[j - 1]->decim_factor == mfir)
+			if (j != 0 && fir_list[j - 1]->decim_factor == mfir)


this is an actual flow change, is this intended? @singalsu is this correct? If this is fixing an error, we should indicate that. Also not sure why you're saying that this check is pointless?

Yep, there's a mistake, also a duplicate decimation factor in fir_list[1] should be checked for. The check should have been if (j > 0 && but check for non-zero is fine too. The commit title and description could be changed.

lyakh · 2021-10-01T07:38:18Z

src/include/sof/ipc/common.h

 * @param hdr Points to the IPC command header.
 */
-void ipc_cmd(ipc_cmd_hdr *hdr);
+void ipc_cmd(ipc_cmd_hdr *_hdr);


I've seen the same warning - mismatched parameter name in declaration and definition without cppcheck. I think the compiler checks it itself (maybe enabled by some flags, or added in some versions), or maybe it was one of the tools, used during the kernel / SOF / Zephyr build. I wasn't using anything special for sure.

lgirdwood · 2021-10-12T13:03:26Z

@cujomalainey I guess you are busy so @lyakh will fix it up for you.

lyakh · 2021-10-12T13:18:19Z

@cujomalainey I guess you are busy so @lyakh will fix it up for you.

#4864

cujomalainey · 2021-10-12T22:17:37Z

@lgirdwood apologies yes I got caught up with internal work past couple weeks, I will pick this up when I get the chance.

lgirdwood · 2021-10-15T13:16:43Z

Can close as now merged with fix for doxygen.

cujomalainey · 2021-10-15T19:46:58Z

Sounds good, will reopen when i have time to address the comments

marc-hb · 2021-10-15T22:54:52Z

@cujomalainey #4864 is merged and has all the commits here. I don't think you will ever re-open this PR :-)

No one can add any commit to your branch at https://github.com/cujomalainey/sof/commits/cppcheck-ci so @lyakh had to copy your commits to a new PR.

cujomalainey · 2021-10-16T02:58:48Z

Thanks marc, I forgot my chain was apart of that

And you can do prs against each other's repos to land stuff on remote branches

marc-hb · 2021-10-16T05:31:32Z

And you can do prs against each other's repos to land stuff on remote branches

Yes but you cannot change someone else's fork so a new PR was required in this case.

cujomalainey requested review from johnylin76 and singalsu August 26, 2021 23:59

cujomalainey requested review from chiang831, dbaluta, lbetlej, lgirdwood, marc-hb, mmaka1, plbossart and zrombel as code owners August 26, 2021 23:59

cujomalainey force-pushed the cppcheck-ci branch 2 times, most recently from e7b5d84 to 9cff547 Compare August 27, 2021 00:03

cujomalainey marked this pull request as draft August 27, 2021 00:05

singalsu approved these changes Aug 27, 2021

View reviewed changes

lgirdwood approved these changes Aug 27, 2021

View reviewed changes

marc-hb previously requested changes Aug 27, 2021

View reviewed changes

lgirdwood added this to the v1.9 milestone Sep 7, 2021

cujomalainey added 3 commits September 21, 2021 15:18

init: fix uninitiallized variable

e185dff

we should never need it, but this is just to silence cppcheck Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

dmic_nhlt: fix uninitialized value

0a906e2

cppcheck error Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

multiband_drc: fix null pointer check

fb20fad

cppcheck pointed out that either our nullpointer check was not needed or that we are failing to do it before using the pointer. The check looks valid so lets fix the lookup. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

cujomalainey force-pushed the cppcheck-ci branch 2 times, most recently from 027512e to d2acb82 Compare September 21, 2021 22:39

cujomalainey added 3 commits September 21, 2021 16:52

dmic: conditionally include variables

f23a95e

conditionally used variables should be conditionally compiled in Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

style: make delcaration match implementations

e93e2d4

minor style violation, these should match exactly Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

ipc: don't shadow functions with variables

0722e08

ipc_free is a function, we should avoid shadowing in the event we tried to use the function here. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

cujomalainey requested a review from marc-hb September 21, 2021 23:58

lgirdwood approved these changes Sep 23, 2021

View reviewed changes

marc-hb reviewed Sep 23, 2021

View reviewed changes

marc-hb requested review from lyakh, paulstelian97 and singalsu September 23, 2021 20:38

paulstelian97 reviewed Sep 27, 2021

View reviewed changes

paulstelian97 approved these changes Sep 27, 2021

View reviewed changes

plbossart approved these changes Sep 28, 2021

View reviewed changes

lyakh reviewed Oct 1, 2021

View reviewed changes

lyakh mentioned this pull request Oct 12, 2021

replacement of #4686 with a single doxygen fix #4864

Merged

lgirdwood closed this Oct 15, 2021

More cppcheck fixes #4686

More cppcheck fixes #4686

Uh oh!

Conversation

cujomalainey commented Aug 26, 2021

Uh oh!

cujomalainey commented Aug 27, 2021

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Aug 27, 2021

Uh oh!

cujomalainey commented Aug 27, 2021

Uh oh!

marc-hb commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cujomalainey commented Sep 1, 2021

Uh oh!

marc-hb commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cujomalainey commented Sep 3, 2021

Uh oh!

cujomalainey commented Sep 3, 2021

Uh oh!

cujomalainey commented Sep 21, 2021

Uh oh!

marc-hb commented Sep 22, 2021

Uh oh!

cujomalainey commented Sep 22, 2021

Uh oh!

marc-hb commented Sep 22, 2021

Uh oh!

lgirdwood commented Sep 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulstelian97 left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented Sep 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

marc-hb commented Aug 29, 2021 •

edited

Loading

marc-hb commented Sep 3, 2021 •

edited

Loading

marc-hb Sep 23, 2021 •

edited

Loading

cujomalainey commented Oct 16, 2021 •

edited

Loading