| Message ID | 20190618092650.2943749-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vmalloc: avoid bogus -Wmaybe-uninitialized warning | expand |
On Tue, Jun 18, 2019 at 5:27 AM Arnd Bergmann <arnd@arndb.de> wrote: > > gcc gets confused in pcpu_get_vm_areas() because there are too many > branches that affect whether 'lva' was initialized before it gets > used: > > mm/vmalloc.c: In function 'pcpu_get_vm_areas': > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized] > insert_vmap_area_augment(lva, &va->rb_node, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > &free_vmap_area_root, &free_vmap_area_list); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > mm/vmalloc.c:916:20: note: 'lva' was declared here > struct vmap_area *lva; > ^~~ > > Add an intialization to NULL, and check whether this has changed > before the first use. > > Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap allocation") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > mm/vmalloc.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index a9213fc3802d..42a6f795c3ee 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -913,7 +913,12 @@ adjust_va_to_fit_type(struct vmap_area *va, > unsigned long nva_start_addr, unsigned long size, > enum fit_type type) > { > - struct vmap_area *lva; > + /* > + * GCC cannot always keep track of whether this variable > + * was initialized across many branches, therefore set > + * it NULL here to avoid a warning. > + */ > + struct vmap_area *lva = NULL; Fair enough, but is this 5-line comment really needed here? - Joel
On Tue, Jun 18, 2019 at 09:40:28AM -0400, Joel Fernandes wrote: > On Tue, Jun 18, 2019 at 5:27 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > gcc gets confused in pcpu_get_vm_areas() because there are too many > > branches that affect whether 'lva' was initialized before it gets > > used: > > > > mm/vmalloc.c: In function 'pcpu_get_vm_areas': > > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > insert_vmap_area_augment(lva, &va->rb_node, > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > &free_vmap_area_root, &free_vmap_area_list); > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > mm/vmalloc.c:916:20: note: 'lva' was declared here > > struct vmap_area *lva; > > ^~~ > > > > Add an intialization to NULL, and check whether this has changed > > before the first use. > > > > Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap allocation") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > mm/vmalloc.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > index a9213fc3802d..42a6f795c3ee 100644 > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -913,7 +913,12 @@ adjust_va_to_fit_type(struct vmap_area *va, > > unsigned long nva_start_addr, unsigned long size, > > enum fit_type type) > > { > > - struct vmap_area *lva; > > + /* > > + * GCC cannot always keep track of whether this variable > > + * was initialized across many branches, therefore set > > + * it NULL here to avoid a warning. > > + */ > > + struct vmap_area *lva = NULL; > > Fair enough, but is this 5-line comment really needed here? > How it is rewritten now, probably not. I would just set it NULL and leave the comment, but that is IMHO. Anyway Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com> Thanks! -- Vlad Rezki
On Tue, 18 Jun 2019 16:06:22 +0200 Uladzislau Rezki <urezki@gmail.com> wrote: > On Tue, Jun 18, 2019 at 09:40:28AM -0400, Joel Fernandes wrote: > > On Tue, Jun 18, 2019 at 5:27 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > gcc gets confused in pcpu_get_vm_areas() because there are too many > > > branches that affect whether 'lva' was initialized before it gets > > > used: > > > > > > mm/vmalloc.c: In function 'pcpu_get_vm_areas': > > > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > insert_vmap_area_augment(lva, &va->rb_node, > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > &free_vmap_area_root, &free_vmap_area_list); > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > mm/vmalloc.c:916:20: note: 'lva' was declared here > > > struct vmap_area *lva; > > > ^~~ > > > > > > Add an intialization to NULL, and check whether this has changed > > > before the first use. > > > > > > Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap allocation") > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > --- > > > mm/vmalloc.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > > index a9213fc3802d..42a6f795c3ee 100644 > > > --- a/mm/vmalloc.c > > > +++ b/mm/vmalloc.c > > > @@ -913,7 +913,12 @@ adjust_va_to_fit_type(struct vmap_area *va, > > > unsigned long nva_start_addr, unsigned long size, > > > enum fit_type type) > > > { > > > - struct vmap_area *lva; > > > + /* > > > + * GCC cannot always keep track of whether this variable > > > + * was initialized across many branches, therefore set > > > + * it NULL here to avoid a warning. > > > + */ > > > + struct vmap_area *lva = NULL; > > > > Fair enough, but is this 5-line comment really needed here? > > > How it is rewritten now, probably not. I would just set it NULL and > leave the comment, but that is IMHO. Anyway > I agree - given that the patch does this: @@ -972,7 +977,7 @@ adjust_va_to_fit_type(struct vmap_area * if (type != FL_FIT_TYPE) { augment_tree_propagate_from(va); - if (type == NE_FIT_TYPE) + if (lva) insert_vmap_area_augment(lva, &va->rb_node, &free_vmap_area_root, &free_vmap_area_list); } the comment simply isn't relevant any more. Although I guess this might be a bit helpful: @@ -977,7 +972,7 @@ adjust_va_to_fit_type(struct vmap_area * if (type != FL_FIT_TYPE) { augment_tree_propagate_from(va); - if (lva) + if (lva) /* type == NE_FIT_TYPE */ insert_vmap_area_augment(lva, &va->rb_node, &free_vmap_area_root, &free_vmap_area_list); }
On Tue, Jun 18, 2019 at 01:59:20PM -0700, Andrew Morton wrote: > On Tue, 18 Jun 2019 16:06:22 +0200 Uladzislau Rezki <urezki@gmail.com> wrote: > > > On Tue, Jun 18, 2019 at 09:40:28AM -0400, Joel Fernandes wrote: > > > On Tue, Jun 18, 2019 at 5:27 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > > > > > > gcc gets confused in pcpu_get_vm_areas() because there are too many > > > > branches that affect whether 'lva' was initialized before it gets > > > > used: > > > > > > > > mm/vmalloc.c: In function 'pcpu_get_vm_areas': > > > > mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > > insert_vmap_area_augment(lva, &va->rb_node, > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > &free_vmap_area_root, &free_vmap_area_list); > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > mm/vmalloc.c:916:20: note: 'lva' was declared here > > > > struct vmap_area *lva; > > > > ^~~ > > > > > > > > Add an intialization to NULL, and check whether this has changed > > > > before the first use. > > > > > > > > Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap allocation") > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > --- > > > > mm/vmalloc.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > > > > index a9213fc3802d..42a6f795c3ee 100644 > > > > --- a/mm/vmalloc.c > > > > +++ b/mm/vmalloc.c > > > > @@ -913,7 +913,12 @@ adjust_va_to_fit_type(struct vmap_area *va, > > > > unsigned long nva_start_addr, unsigned long size, > > > > enum fit_type type) > > > > { > > > > - struct vmap_area *lva; > > > > + /* > > > > + * GCC cannot always keep track of whether this variable > > > > + * was initialized across many branches, therefore set > > > > + * it NULL here to avoid a warning. > > > > + */ > > > > + struct vmap_area *lva = NULL; > > > > > > Fair enough, but is this 5-line comment really needed here? > > > > > How it is rewritten now, probably not. I would just set it NULL and > > leave the comment, but that is IMHO. Anyway > > > > I agree - given that the patch does this: > > @@ -972,7 +977,7 @@ adjust_va_to_fit_type(struct vmap_area * > if (type != FL_FIT_TYPE) { > augment_tree_propagate_from(va); > > - if (type == NE_FIT_TYPE) > + if (lva) > insert_vmap_area_augment(lva, &va->rb_node, > &free_vmap_area_root, &free_vmap_area_list); > } > > the comment simply isn't relevant any more. Although I guess this > might be a bit helpful: > > @@ -977,7 +972,7 @@ adjust_va_to_fit_type(struct vmap_area * > if (type != FL_FIT_TYPE) { > augment_tree_propagate_from(va); > > - if (lva) > + if (lva) /* type == NE_FIT_TYPE */ > insert_vmap_area_augment(lva, &va->rb_node, > &free_vmap_area_root, &free_vmap_area_list); > } > That comment makes it much clear, thanks! -- Vlad Rezki
diff --git a/mm/vmalloc.c b/mm/vmalloc.c index a9213fc3802d..42a6f795c3ee 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -913,7 +913,12 @@ adjust_va_to_fit_type(struct vmap_area *va, unsigned long nva_start_addr, unsigned long size, enum fit_type type) { - struct vmap_area *lva; + /* + * GCC cannot always keep track of whether this variable + * was initialized across many branches, therefore set + * it NULL here to avoid a warning. + */ + struct vmap_area *lva = NULL; if (type == FL_FIT_TYPE) { /* @@ -987,7 +992,7 @@ adjust_va_to_fit_type(struct vmap_area *va, if (type != FL_FIT_TYPE) { augment_tree_propagate_from(va); - if (type == NE_FIT_TYPE) + if (lva) insert_vmap_area_augment(lva, &va->rb_node, &free_vmap_area_root, &free_vmap_area_list); }
gcc gets confused in pcpu_get_vm_areas() because there are too many branches that affect whether 'lva' was initialized before it gets used: mm/vmalloc.c: In function 'pcpu_get_vm_areas': mm/vmalloc.c:991:4: error: 'lva' may be used uninitialized in this function [-Werror=maybe-uninitialized] insert_vmap_area_augment(lva, &va->rb_node, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ &free_vmap_area_root, &free_vmap_area_list); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mm/vmalloc.c:916:20: note: 'lva' was declared here struct vmap_area *lva; ^~~ Add an intialization to NULL, and check whether this has changed before the first use. Fixes: 68ad4a330433 ("mm/vmalloc.c: keep track of free blocks for vmap allocation") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- mm/vmalloc.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) -- 2.20.0