| Message ID | 20190603174323.48251-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | PCI: rpaphp: Avoid a sometimes-uninitialized warning | expand |
On Mon, Jun 3, 2019 at 10:44 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > Looking at the loop in a vacuum as clang would, fndit could be > uninitialized if entries was ever zero or the if statement was > always true within the loop. Regardless of whether or not this > warning is a problem in practice, "found" variables should always > be initialized to false so that there is no possibility of > undefined behavior. Thanks for the patch Nathan. fndit isn't really being used for anything other than a print statement outside of the loop. How about: ``` diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c index bcd5d357ca23..c3899ee1db99 100644 --- a/drivers/pci/hotplug/rpaphp_core.c +++ b/drivers/pci/hotplug/rpaphp_core.c @@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name, struct of_drc_info drc; const __be32 *value; char cell_drc_name[MAX_DRC_NAME_LEN]; - int j, fndit; + int j; info = of_find_property(dn->parent, "ibm,drc-info", NULL); if (info == NULL) @@ -245,17 +245,13 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name, /* Should now know end of current entry */ - if (my_index > drc.last_drc_index) - continue; - - fndit = 1; - break; + /* Found it */ + if (my_index <= drc.last_drc_index) { + sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, + my_index); + 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))) && ``` (not sure my tabs were pasted properly in the above...)
Hi Nick, On Mon, Jun 03, 2019 at 02:07:50PM -0700, Nick Desaulniers wrote: > On Mon, Jun 3, 2019 at 10:44 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > Looking at the loop in a vacuum as clang would, fndit could be > > uninitialized if entries was ever zero or the if statement was > > always true within the loop. Regardless of whether or not this > > warning is a problem in practice, "found" variables should always > > be initialized to false so that there is no possibility of > > undefined behavior. > > Thanks for the patch Nathan. fndit isn't really being used for > anything other than a print statement outside of the loop. How about: Thank you for the review, this seems reasonable. I will send a v2 shortly. > > ``` > diff --git a/drivers/pci/hotplug/rpaphp_core.c > b/drivers/pci/hotplug/rpaphp_core.c > index bcd5d357ca23..c3899ee1db99 100644 > --- a/drivers/pci/hotplug/rpaphp_core.c > +++ b/drivers/pci/hotplug/rpaphp_core.c > @@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct > device_node *dn, char *drc_name, > struct of_drc_info drc; > const __be32 *value; > char cell_drc_name[MAX_DRC_NAME_LEN]; > - int j, fndit; > + int j; > > info = of_find_property(dn->parent, "ibm,drc-info", NULL); > if (info == NULL) > @@ -245,17 +245,13 @@ static int rpaphp_check_drc_props_v2(struct > device_node *dn, char *drc_name, > > /* Should now know end of current entry */ > > - if (my_index > drc.last_drc_index) > - continue; > - > - fndit = 1; > - break; > + /* Found it */ > + if (my_index <= drc.last_drc_index) { > + sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, > + my_index); > + 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))) && > ``` > (not sure my tabs were pasted properly in the above...) Doesn't look like it but no worries. Thanks, Nathan
Quoting Nathan Chancellor <natechancellor@gmail.com>: > When building with -Wsometimes-uninitialized, clang warns: > > 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 > > Looking at the loop in a vacuum as clang would, fndit could be > uninitialized if entries was ever zero or the if statement was > always true within the loop. Regardless of whether or not this > warning is a problem in practice, "found" variables should always > be initialized to false so that there is no possibility of > undefined behavior. > > Link: https://github.com/ClangBuiltLinux/linux/issues/504 > Fixes: 2fcf3ae508c2 ("hotplug/drc-info: Add code to search > ibm,drc-info property") > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/pci/hotplug/rpaphp_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/hotplug/rpaphp_core.c > b/drivers/pci/hotplug/rpaphp_core.c > index bcd5d357ca23..07b56bf2886f 100644 > --- a/drivers/pci/hotplug/rpaphp_core.c > +++ b/drivers/pci/hotplug/rpaphp_core.c > @@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct > device_node *dn, char *drc_name, > struct of_drc_info drc; > const __be32 *value; > char cell_drc_name[MAX_DRC_NAME_LEN]; > - int j, fndit; > + int j, fndit = 0; Not sure fndit is needed at all. Looking into the full function code, fndit only serves as doing a single action. That action could be done in the loop directly, see below And while you are at it, I guess you should also handle the initialisation of cell_drc_name. In the current code, it looks like content of cell_drc_name is undefined when doing the strcmp when fndit is not 1. > > info = of_find_property(dn->parent, "ibm,drc-info", NULL); > if (info == NULL) > -- > 2.22.0.rc2 diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c index bcd5d357ca23..87113419a44f 100644 --- a/drivers/pci/hotplug/rpaphp_core.c +++ b/drivers/pci/hotplug/rpaphp_core.c @@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name, struct of_drc_info drc; const __be32 *value; char cell_drc_name[MAX_DRC_NAME_LEN]; - int j, fndit; + int j; info = of_find_property(dn->parent, "ibm,drc-info", NULL); if (info == NULL) @@ -248,14 +248,10 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name, if (my_index > drc.last_drc_index) continue; - fndit = 1; + /* Found it */ + sprintf(cell_drc_name, "%s%d", drc.drc_name_prefix, my_index); 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))) && Christophe
diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c index bcd5d357ca23..07b56bf2886f 100644 --- a/drivers/pci/hotplug/rpaphp_core.c +++ b/drivers/pci/hotplug/rpaphp_core.c @@ -230,7 +230,7 @@ static int rpaphp_check_drc_props_v2(struct device_node *dn, char *drc_name, struct of_drc_info drc; const __be32 *value; char cell_drc_name[MAX_DRC_NAME_LEN]; - int j, fndit; + int j, fndit = 0; info = of_find_property(dn->parent, "ibm,drc-info", NULL); if (info == NULL)
When building with -Wsometimes-uninitialized, clang warns: 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 Looking at the loop in a vacuum as clang would, fndit could be uninitialized if entries was ever zero or the if statement was always true within the loop. Regardless of whether or not this warning is a problem in practice, "found" variables should always be initialized to false so that there is no possibility of undefined behavior. Link: https://github.com/ClangBuiltLinux/linux/issues/504 Fixes: 2fcf3ae508c2 ("hotplug/drc-info: Add code to search ibm,drc-info property") Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/pci/hotplug/rpaphp_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)