| Message ID | 20160919123359.1225305-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/19/2016 06:33 AM, Arnd Bergmann wrote: > The sbitmap code that has just been turned into a library module > returns uninitialized data for sbitmap_weight(), as pointed out by > gcc when building with -Wmaybe-uninitialized: > > lib/sbitmap.c: In function 'sbitmap_weight': > lib/sbitmap.c:179:9: error: 'weight' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > Note that the value is never initialized, we just add data on > top, so it is wrong regardless of sb->map_nr. > > This adds the missing initialization. Thanks Arnd, Colin sent the same patch and I applied it. Note that: > Fixes: 88459642cba4 ("blk-mq: abstract tag allocation out into sbitmap library") that isn't truly correct, that patch is just what moved the code. The bug predates that commit.
On Monday, September 19, 2016 8:43:12 AM CEST Jens Axboe wrote: > On 09/19/2016 06:33 AM, Arnd Bergmann wrote: > > The sbitmap code that has just been turned into a library module > > returns uninitialized data for sbitmap_weight(), as pointed out by > > gcc when building with -Wmaybe-uninitialized: > > > > lib/sbitmap.c: In function 'sbitmap_weight': > > lib/sbitmap.c:179:9: error: 'weight' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > > Note that the value is never initialized, we just add data on > > top, so it is wrong regardless of sb->map_nr. > > > > This adds the missing initialization. > > Thanks Arnd, Colin sent the same patch and I applied it. Note that: Ok, thanks! > > Fixes: 88459642cba4 ("blk-mq: abstract tag allocation out into sbitmap library") > > that isn't truly correct, that patch is just what moved the code. The > bug predates that commit. It's not important as long as the bug is fixed now, but I think it was correct before that move, we just lost the 'used=0' initialization, which also triggered the warning: -static unsigned int bt_unused_tags(struct blk_mq_bitmap_tags *bt) -{ - unsigned int i, used; - - for (i = 0, used = 0; i < bt->map_nr; i++) { - struct blk_align_bitmap *bm = &bt->map[i]; - - used += bitmap_weight(&bm->word, bm->depth); } - return bt->depth - used; } Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/19/2016 09:14 AM, Arnd Bergmann wrote: > On Monday, September 19, 2016 8:43:12 AM CEST Jens Axboe wrote: >> On 09/19/2016 06:33 AM, Arnd Bergmann wrote: >>> The sbitmap code that has just been turned into a library module >>> returns uninitialized data for sbitmap_weight(), as pointed out by >>> gcc when building with -Wmaybe-uninitialized: >>> >>> lib/sbitmap.c: In function 'sbitmap_weight': >>> lib/sbitmap.c:179:9: error: 'weight' may be used uninitialized in this function [-Werror=maybe-uninitialized] >>> >>> Note that the value is never initialized, we just add data on >>> top, so it is wrong regardless of sb->map_nr. >>> >>> This adds the missing initialization. >> >> Thanks Arnd, Colin sent the same patch and I applied it. Note that: > > Ok, thanks! > >>> Fixes: 88459642cba4 ("blk-mq: abstract tag allocation out into sbitmap library") >> >> that isn't truly correct, that patch is just what moved the code. The >> bug predates that commit. > > It's not important as long as the bug is fixed now, but I think it was > correct before that move, we just lost the 'used=0' initialization, > which also triggered the warning: > > -static unsigned int bt_unused_tags(struct blk_mq_bitmap_tags *bt) > -{ > - unsigned int i, used; > - > - for (i = 0, used = 0; i < bt->map_nr; i++) { > - struct blk_align_bitmap *bm = &bt->map[i]; > - > - used += bitmap_weight(&bm->word, bm->depth); > } > > - return bt->depth - used; > } I missed that, as I generally never use double inits in for loops, but I guess I did for this one. But you are right, the bug was introduced with the recent move, so your Fixes was completely correct. Omar loses a cookie.
On Mon, Sep 19, 2016 at 09:22:34AM -0600, Jens Axboe wrote: > On 09/19/2016 09:14 AM, Arnd Bergmann wrote: > > On Monday, September 19, 2016 8:43:12 AM CEST Jens Axboe wrote: > > > On 09/19/2016 06:33 AM, Arnd Bergmann wrote: > > > > The sbitmap code that has just been turned into a library module > > > > returns uninitialized data for sbitmap_weight(), as pointed out by > > > > gcc when building with -Wmaybe-uninitialized: > > > > > > > > lib/sbitmap.c: In function 'sbitmap_weight': > > > > lib/sbitmap.c:179:9: error: 'weight' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > > > > > > > Note that the value is never initialized, we just add data on > > > > top, so it is wrong regardless of sb->map_nr. > > > > > > > > This adds the missing initialization. > > > > > > Thanks Arnd, Colin sent the same patch and I applied it. Note that: > > > > Ok, thanks! > > > > > > Fixes: 88459642cba4 ("blk-mq: abstract tag allocation out into sbitmap library") > > > > > > that isn't truly correct, that patch is just what moved the code. The > > > bug predates that commit. > > > > It's not important as long as the bug is fixed now, but I think it was > > correct before that move, we just lost the 'used=0' initialization, > > which also triggered the warning: > > > > -static unsigned int bt_unused_tags(struct blk_mq_bitmap_tags *bt) > > -{ > > - unsigned int i, used; > > - > > - for (i = 0, used = 0; i < bt->map_nr; i++) { > > - struct blk_align_bitmap *bm = &bt->map[i]; > > - > > - used += bitmap_weight(&bm->word, bm->depth); > > } > > > > - return bt->depth - used; > > } > > I missed that, as I generally never use double inits in for loops, but I > guess I did for this one. But you are right, the bug was introduced with > the recent move, so your Fixes was completely correct. > > Omar loses a cookie. :( Apparently we disable -Wmaybe-uninitialized by default since 6e8d666e9253 ("Disable "maybe-uninitialized" warning globally"), that's probably why I didn't see this. My mistake, thanks Arnd and Colin.
diff --git a/lib/sbitmap.c b/lib/sbitmap.c index e40808921544..2cecf05c82fd 100644 --- a/lib/sbitmap.c +++ b/lib/sbitmap.c @@ -169,7 +169,7 @@ EXPORT_SYMBOL_GPL(sbitmap_any_bit_clear); unsigned int sbitmap_weight(const struct sbitmap *sb) { - unsigned int i, weight; + unsigned int i, weight = 0; for (i = 0; i < sb->map_nr; i++) { const struct sbitmap_word *word = &sb->map[i];
The sbitmap code that has just been turned into a library module returns uninitialized data for sbitmap_weight(), as pointed out by gcc when building with -Wmaybe-uninitialized: lib/sbitmap.c: In function 'sbitmap_weight': lib/sbitmap.c:179:9: error: 'weight' may be used uninitialized in this function [-Werror=maybe-uninitialized] Note that the value is never initialized, we just add data on top, so it is wrong regardless of sb->map_nr. This adds the missing initialization. Fixes: 88459642cba4 ("blk-mq: abstract tag allocation out into sbitmap library") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- lib/sbitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)