| Message ID | 20181102153138.1399758-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | mm: fix uninitialized variable warnings | expand |
On Fri 02-11-18 16:31:06, Arnd Bergmann wrote: > In a rare randconfig build, I got a warning about possibly uninitialized > variables: > > mm/page-writeback.c: In function 'balance_dirty_pages': > mm/page-writeback.c:1623:16: error: 'writeback' may be used uninitialized in this function [-Werror=maybe-uninitialized] > mdtc->dirty += writeback; > ^~ > mm/page-writeback.c:1624:4: error: 'filepages' may be used uninitialized in this function [-Werror=maybe-uninitialized] > mdtc_calc_avail(mdtc, filepages, headroom); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > mm/page-writeback.c:1624:4: error: 'headroom' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > The compiler evidently fails to notice that the usage is in dead code > after 'mdtc' is set to NULL when CONFIG_CGROUP_WRITEBACK is disabled. > Adding an IS_ENABLED() check makes this clear to the compiler. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> I'm surprised the compiler was not able to infer this since: struct dirty_throttle_control * const mdtc = mdtc_valid(&mdtc_stor) ? &mdtc_stor : NULL; and if CONFIG_CGROUP_WRITEBACK is disabled, mdtc_valid() is defined to 'false'. But possibly the function is just too big and the problematic condition is in the loop so maybe it all confuses the compiler too much. > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index 3f690bae6b78..f02535b7731a 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -1611,7 +1611,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb, > bg_thresh = gdtc->bg_thresh; > } > > - if (mdtc) { > + if (IS_ENABLED(CONFIG_CGROUP_WRITEBACK) && mdtc) { > unsigned long filepages, headroom, writeback; Honestly, I don't like the IS_ENABLED(CONFIG_CGROUP_WRITEBACK) check here. It just looks too arbitrary. Could we perhaps change the code like struct dirty_throttle_control * const mdtc = &mdtc_stor; And then replace checks for !mtdc in the function to !mdtc_valid(mdtc)? That is the same thing as currently and it should make it obvious to the compiler as well as human what is going on... Tejun? Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR
On 11/5/18, Jan Kara <jack@suse.cz> wrote: > On Fri 02-11-18 16:31:06, Arnd Bergmann wrote: >> In a rare randconfig build, I got a warning about possibly uninitialized >> variables: >> >> mm/page-writeback.c: In function 'balance_dirty_pages': >> mm/page-writeback.c:1623:16: error: 'writeback' may be used uninitialized >> in this function [-Werror=maybe-uninitialized] >> mdtc->dirty += writeback; >> ^~ >> mm/page-writeback.c:1624:4: error: 'filepages' may be used uninitialized >> in this function [-Werror=maybe-uninitialized] >> mdtc_calc_avail(mdtc, filepages, headroom); >> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> mm/page-writeback.c:1624:4: error: 'headroom' may be used uninitialized in >> this function [-Werror=maybe-uninitialized] >> >> The compiler evidently fails to notice that the usage is in dead code >> after 'mdtc' is set to NULL when CONFIG_CGROUP_WRITEBACK is disabled. >> Adding an IS_ENABLED() check makes this clear to the compiler. >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > I'm surprised the compiler was not able to infer this since: > > struct dirty_throttle_control * const mdtc = mdtc_valid(&mdtc_stor) ? > &mdtc_stor : NULL; > > and if CONFIG_CGROUP_WRITEBACK is disabled, mdtc_valid() is defined to > 'false'. But possibly the function is just too big and the problematic > condition is in the loop so maybe it all confuses the compiler too much. On second thought, I suspect this started with the introduction of CONFIG_NO_AUTO_INLINE in linux-next. That also caused a similar issue in 28 other files that I patched later. I wrote this patch before I saw the others, and then didn't make the connection. Let's drop the patch for now, and decide what we want to do for the others. I fixed those by adding 'inline' markers for whatever function needed it. Arnd
diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 3f690bae6b78..f02535b7731a 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -1611,7 +1611,7 @@ static void balance_dirty_pages(struct bdi_writeback *wb, bg_thresh = gdtc->bg_thresh; } - if (mdtc) { + if (IS_ENABLED(CONFIG_CGROUP_WRITEBACK) && mdtc) { unsigned long filepages, headroom, writeback; /* @@ -1944,7 +1944,7 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb) wb_calc_thresh(gdtc->wb, gdtc->bg_thresh)) return true; - if (mdtc) { + if (IS_ENABLED(CONFIG_CGROUP_WRITEBACK) && mdtc) { unsigned long filepages, headroom, writeback; mem_cgroup_wb_stats(wb, &filepages, &headroom, &mdtc->dirty,
In a rare randconfig build, I got a warning about possibly uninitialized variables: mm/page-writeback.c: In function 'balance_dirty_pages': mm/page-writeback.c:1623:16: error: 'writeback' may be used uninitialized in this function [-Werror=maybe-uninitialized] mdtc->dirty += writeback; ^~ mm/page-writeback.c:1624:4: error: 'filepages' may be used uninitialized in this function [-Werror=maybe-uninitialized] mdtc_calc_avail(mdtc, filepages, headroom); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ mm/page-writeback.c:1624:4: error: 'headroom' may be used uninitialized in this function [-Werror=maybe-uninitialized] The compiler evidently fails to notice that the usage is in dead code after 'mdtc' is set to NULL when CONFIG_CGROUP_WRITEBACK is disabled. Adding an IS_ENABLED() check makes this clear to the compiler. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- mm/page-writeback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.18.0