Skip to content

More cppcheck fixes #4686

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.

Sign up for GitHub

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

Closed
cujomalainey wants to merge 10 commits into from
Closed

More cppcheck fixes #4686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e185dff
init: fix uninitiallized variable
cujomalainey Aug 26, 2021
0a906e2
dmic_nhlt: fix uninitialized value
cujomalainey Aug 26, 2021
fb20fad
multiband_drc: fix null pointer check
cujomalainey Aug 26, 2021
609b5c1
numbers: fix signed int overflow
cujomalainey Sep 21, 2021
28a590f
dmic: change condition to satisfy cppcheck
cujomalainey Sep 21, 2021
ad960cc
codec_adapter: remove pointless null pointer check
cujomalainey Sep 21, 2021
9b04a73
crossover: fix return type
cujomalainey Sep 21, 2021
f23a95e
dmic: conditionally include variables
cujomalainey Sep 21, 2021
e93e2d4
style: make delcaration match implementations
cujomalainey Sep 21, 2021
0722e08
ipc: don't shadow functions with variables
cujomalainey Sep 21, 2021
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
2 changes: 1 addition & 1 deletion src/audio/codec_adapter/codec/generic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ codec_load_config(struct comp_dev *dev, void *cfg, size_t size,

comp_dbg(dev, "codec_load_config() start");

if (!dev || !cfg || !size) {
if (!cfg || !size) {
comp_err(dev, "codec_load_config(): wrong input params! dev %x, cfg %x size %d",
(uint32_t)dev, (uint32_t)cfg, size);
return -EINVAL;
Expand Down
8 changes: 2 additions & 6 deletions src/audio/codec_adapter/codec_adapter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ struct comp_dev *codec_adapter_new(const struct comp_driver *drv,

comp_cl_dbg(drv, "codec_adapter_new() start");

if (!drv || !config) {
if (!config) {
comp_cl_err(drv, "codec_adapter_new(), wrong input params! drv = %x config = %x",
(uint32_t)drv, (uint32_t)config);
return NULL;
Expand Down Expand Up @@ -123,11 +123,7 @@ int load_setup_config(struct comp_dev *dev, void *cfg, uint32_t size)

comp_dbg(dev, "load_setup_config() start.");

if (!dev) {
comp_err(dev, "load_setup_config(): no component device.");
ret = -EINVAL;
goto end;
} else if (!cfg || !size) {
if (!cfg || !size) {
comp_err(dev, "load_setup_config(): no config available cfg: %x, size: %d",
(uintptr_t)cfg, size);
ret = -EINVAL;
Expand Down
2 changes: 1 addition & 1 deletion src/audio/crossover/crossover.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ static inline void crossover_reset_state(struct comp_data *cd)
* \return the position at which pipe_id is found in config->assign_sink.
* -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,
Copy link
Collaborator

@paulstelian97 paulstelian97 Sep 27, 2021

Choose a reason for hiding this comment

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

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

uint32_t pipe_id)
{
int i;
Expand Down
5 changes: 3 additions & 2 deletions src/audio/multiband_drc/multiband_drc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,15 @@ static int multiband_drc_init_coef(struct multiband_drc_comp_data *cd, int16_t n
struct sof_multiband_drc_config *config = cd->config;
struct multiband_drc_state *state = &cd->state;
uint32_t sample_bytes = get_sample_bytes(cd->source_format);
int num_bands = cd->config->num_bands;
int i, ch, ret;
int i, ch, ret, num_bands;

if (!config) {
comp_cl_err(&comp_multiband_drc, "multiband_drc_init_coef(), no config is set");
return -EINVAL;
}

num_bands = config->num_bands;
Copy link
Collaborator

@marc-hb marc-hb Aug 27, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cujomalainey cujomalainey Aug 27, 2021

Choose a reason for hiding this comment

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

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


/* Sanity checks */
if (nch > PLATFORM_MAX_CHANNELS) {
comp_cl_err(&comp_multiband_drc,
Expand Down
4 changes: 3 additions & 1 deletion src/drivers/intel/dmic/dmic_computed.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ static void find_modes(struct dai *dai,
mfir = fir_list[j]->decim_factor;

/* 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)
Copy link
Collaborator

@lyakh lyakh Oct 1, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@singalsu singalsu Oct 1, 2021

Choose a reason for hiding this comment

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

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.

continue;

mcic = osr / mfir;
Expand Down Expand Up @@ -593,7 +593,9 @@ static int configure_registers(struct dai *dai,
uint32_t ref;
int32_t ci;
uint32_t cu;
#if defined(DMIC_IPM_VER1) || defined(DMIC_IPM_VER2)
int ipm;
#endif
int of0;
int of1;
int fir_decim;
Expand Down
2 changes: 1 addition & 1 deletion src/drivers/intel/dmic/dmic_nhlt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ int dmic_set_config_nhlt(struct dai *dai, void *spec_config)
uint32_t channel_ctrl_mask;
uint32_t fir_control;
uint32_t pdm_ctrl_mask;
uint32_t ref;
uint32_t ref = 0;
Copy link
Collaborator

@lyakh lyakh Oct 1, 2021

Choose a reason for hiding this comment

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

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.

uint32_t val;
const uint8_t *p = spec_config;
int num_fifos;
Expand Down
2 changes: 1 addition & 1 deletion src/include/sof/ipc/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ ipc_cmd_hdr *mailbox_validate(void);
*
* @param hdr Points to the IPC command header.
*/
void ipc_cmd(ipc_cmd_hdr *hdr);
void ipc_cmd(ipc_cmd_hdr *_hdr);
Copy link
Collaborator

@marc-hb marc-hb Sep 23, 2021

Choose a reason for hiding this comment

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

Just curious what is the warning here.

Copy link
Contributor Author

@cujomalainey cujomalainey Sep 23, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cujomalainey cujomalainey Sep 23, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@marc-hb marc-hb Sep 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@cujomalainey cujomalainey Sep 23, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@marc-hb marc-hb Sep 28, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@plbossart plbossart Sep 28, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@marc-hb marc-hb Sep 28, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@plbossart plbossart Sep 28, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@lyakh lyakh Oct 1, 2021

Choose a reason for hiding this comment

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

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.


/**
* \brief IPC message to be processed on other core.
Expand Down
2 changes: 1 addition & 1 deletion src/include/sof/math/numbers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ static inline int ceil_divide(int a, int b)
* If the signs are the same, we check if there was any remainder in
* the division by multiplying the number back.
*/
if (!((a ^ b) & (1 << ((sizeof(int) * 8) - 1))) && c * b != a)
if (!((a ^ b) & (1U << ((sizeof(int) * 8) - 1))) && c * b != a)
c++;

return c;
Expand Down
2 changes: 1 addition & 1 deletion src/init/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ static int primary_core_init(int argc, char *argv[], struct sof *sof)
#ifndef __ZEPHYR__
int main(int argc, char *argv[])
{
int err;
int err = 0;

trace_point(TRACE_BOOT_START);

Expand Down
10 changes: 5 additions & 5 deletions src/ipc/ipc3/handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,20 +1303,20 @@ static int ipc_glb_tplg_free(uint32_t header,
int (*free_func)(struct ipc *ipc, uint32_t id))
{
struct ipc *ipc = ipc_get();
struct sof_ipc_free ipc_free;
struct sof_ipc_free ipc_free_msg;
int ret;

/* copy message with ABI safe method */
IPC_COPY_CMD(ipc_free, ipc->comp_data);
IPC_COPY_CMD(ipc_free_msg, ipc->comp_data);

tr_info(&ipc_tr, "ipc: comp %d -> free", ipc_free.id);
tr_info(&ipc_tr, "ipc: comp %d -> free", ipc_free_msg.id);

/* free the object */
ret = free_func(ipc, ipc_free.id);
ret = free_func(ipc, ipc_free_msg.id);

if (ret < 0) {
tr_err(&ipc_tr, "ipc: comp %d free failed %d",
ipc_free.id, ret);
ipc_free_msg.id, ret);
}

return ret;
Expand Down