-Wsometimes-uninitialized in drivers/pci/hotplug/rpaphp_core.c

[nathanchance]

When building powerpc64 big endian (pseries_defconfig):

drivers/pci/hotplug/rpaphp_core.c:243:14: warning: variable 'fndit' is used uninitialized whenever 'for' loop exits because its condition is false [-Wsometimes-uninitialized]
        for (j = 0; j < entries; j++) {
                    ^~~~~~~~~~~
drivers/pci/hotplug/rpaphp_core.c:256:6: note: uninitialized use occurs here
        if (fndit)
            ^~~~~
drivers/pci/hotplug/rpaphp_core.c:243:14: note: remove the condition if it is always true
        for (j = 0; j < entries; j++) {
                    ^~~~~~~~~~~
drivers/pci/hotplug/rpaphp_core.c:233:14: note: initialize the variable 'fndit' to silence this warning
        int j, fndit;
                    ^
                     = 0
1 warning generated.

Now I think this warning is legitimate looking at the function (link):

static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name,
				char *drc_type, unsigned int my_index)
{
	struct property *info;
	unsigned int entries;
	struct of_drc_info drc;
	const __be32 *value;
	char cell_drc_name[MAX_DRC_NAME_LEN];
	int j, fndit;

	info = of_find_property(dn->parent, "ibm,drc-info", NULL);
	if (info == NULL)
		return -EINVAL;

	value = of_prop_next_u32(info, NULL, &entries);
	if (!value)
		return -EINVAL;

	for (j = 0; j < entries; j++) {
		of_read_drc_info_cell(&info, &value, &drc);

		/* Should now know end of current entry */

		if (my_index > drc.last_drc_index)
			continue;

		fndit = 1;
		break;
	}
	/* Found it */

	if (fndit)
		sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, 
			my_index);

	if (((drc_name == NULL) ||
	     (drc_name && !strcmp(drc_name, cell_drc_name))) &&
	    ((drc_type == NULL) ||
	     (drc_type && !strcmp(drc_type, drc.drc_type))))
		return 0;

	return -EINVAL;
}

In theory, if entries is zero or the if (my_index > drc.last_drc_index) is true every loop iteration (no idea if these are possible in practice) , fndit will be uninitialized so I don't think this is a false positive. Regardless, "found" variables really should be set to zero by default... I'll send a patch for this tomorrow or Monday.

However, I think I may have uncovered a bug in clang. Removing the break statement after fndit's initialization removes the warning.

A minimal test case: https://godbolt.org/z/A_ChRo

clang only warns on a() when it should warn for both a() and b(). Running it locally, it never prints out that we initialize k.

I'm not missing something, right?

[nathanchance]

Patch sent: https://lore.kernel.org/lkml/20190603174323.48251-1-natechancellor@gmail.com/

[nathanchance]

v2: https://lore.kernel.org/lkml/20190603221157.58502-1-natechancellor@gmail.com/

[theyoyojo]

Looks like a good simplification of somewhat convoluted control flow.

[theyoyojo]

Oops I just realized that my reply to your lkml post is missing the "like" between "Looks" and "a"... Oh well

[nickdesaulniers]

probably should bump the thread again @nathanchance

[nathanchance]

probably should bump the thread again @nathanchance

Done.

[nickdesaulniers]

accepted: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?h=next-test&id=fde46ba593df92c4d787d2cc35a11f46583c89a6

[nathanchance]

New hash: https://git.kernel.org/powerpc/c/0df3e42167caaf9f8c7b64de3da40a459979afe8

[nathanchance]

Merged into mainline: https://git.kernel.org/torvalds/c/0df3e42167caaf9f8c7b64de3da40a459979afe8