nvdimm: avoid bogus -Wmaybe-uninitialized warning
diff mbox

Message ID 20170801144534.2a1e1def29e68eb6c83e203c@linux-foundation.org
State New, archived
Headers show

Commit Message

Andrew Morton Aug. 1, 2017, 9:45 p.m. UTC
On Tue,  1 Aug 2017 13:48:48 +0200 Arnd Bergmann <arnd@arndb.de> wrote:

> Removing the btt_rw_page/pmem_rw_page functions had a surprising
> side-effect of introducing a false-positive warning in another
> function, due to changed inlining decisions in gcc:
> 
> In file included from drivers/nvdimm/pmem.c:36:0:
> drivers/nvdimm/pmem.c: In function 'pmem_make_request':
> drivers/nvdimm/nd.h:407:2: error: 'start' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/nvdimm/pmem.c:174:16: note: 'start' was declared here
> In file included from drivers/nvdimm/btt.c:27:0:
> drivers/nvdimm/btt.c: In function 'btt_make_request':
> drivers/nvdimm/nd.h:407:2: error: 'start' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> drivers/nvdimm/btt.c:1202:16: note: 'start' was declared here
> 
> The problem is that gcc fails to track the value of the 'do_acct'
> variable here and has to read it back from stack, but it does
> remember that 'start' may be uninitialized sometimes.
> 
> This shuts up the warning by making nd_iostat_start() always
> initialize the 'start' variable. In those cases that gcc successfully
> tracks the state of the variable, this will have no effect.
> 
> ...
>
> --- a/drivers/nvdimm/nd.h
> +++ b/drivers/nvdimm/nd.h
> @@ -392,8 +392,10 @@ static inline bool nd_iostat_start(struct bio *bio, unsigned long *start)
>  {
>  	struct gendisk *disk = bio->bi_bdev->bd_disk;
>  
> -	if (!blk_queue_io_stat(disk->queue))
> +	if (!blk_queue_io_stat(disk->queue)) {
> +		*start = 0;
>  		return false;
> +	}
>  
>  	*start = jiffies;
>  	generic_start_io_acct(bio_data_dir(bio),

Well that's sad.

The future of btt-remove-btt_rw_page.patch and friends is shrouded in
mystery, but if we proceed that way then yes, I guess we'll need to
work around such gcc glitches.

But let's not leave apparently-unneeded code in place without telling
people why it is in fact needed?

Comments

Ross Zwisler Aug. 1, 2017, 10:23 p.m. UTC | #1
On Tue, Aug 01, 2017 at 02:45:34PM -0700, Andrew Morton wrote:
> On Tue,  1 Aug 2017 13:48:48 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > Removing the btt_rw_page/pmem_rw_page functions had a surprising
> > side-effect of introducing a false-positive warning in another
> > function, due to changed inlining decisions in gcc:
> > 
> > In file included from drivers/nvdimm/pmem.c:36:0:
> > drivers/nvdimm/pmem.c: In function 'pmem_make_request':
> > drivers/nvdimm/nd.h:407:2: error: 'start' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > drivers/nvdimm/pmem.c:174:16: note: 'start' was declared here
> > In file included from drivers/nvdimm/btt.c:27:0:
> > drivers/nvdimm/btt.c: In function 'btt_make_request':
> > drivers/nvdimm/nd.h:407:2: error: 'start' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > drivers/nvdimm/btt.c:1202:16: note: 'start' was declared here
> > 
> > The problem is that gcc fails to track the value of the 'do_acct'
> > variable here and has to read it back from stack, but it does
> > remember that 'start' may be uninitialized sometimes.
> > 
> > This shuts up the warning by making nd_iostat_start() always
> > initialize the 'start' variable. In those cases that gcc successfully
> > tracks the state of the variable, this will have no effect.
> > 
> > ...
> >
> > --- a/drivers/nvdimm/nd.h
> > +++ b/drivers/nvdimm/nd.h
> > @@ -392,8 +392,10 @@ static inline bool nd_iostat_start(struct bio *bio, unsigned long *start)
> >  {
> >  	struct gendisk *disk = bio->bi_bdev->bd_disk;
> >  
> > -	if (!blk_queue_io_stat(disk->queue))
> > +	if (!blk_queue_io_stat(disk->queue)) {
> > +		*start = 0;
> >  		return false;
> > +	}
> >  
> >  	*start = jiffies;
> >  	generic_start_io_acct(bio_data_dir(bio),
> 
> Well that's sad.
> 
> The future of btt-remove-btt_rw_page.patch and friends is shrouded in
> mystery, but if we proceed that way then yes, I guess we'll need to
> work around such gcc glitches.
> 
> But let's not leave apparently-unneeded code in place without telling
> people why it is in fact needed?

Maybe it's just cleaner to initialize 'start' in all the callers, so we don't
have a mysterious line and have to remember why it's there / comment it?

I'll throw a patch like that in my series if/when I repost.
Arnd Bergmann Aug. 2, 2017, 10:54 a.m. UTC | #2
On Wed, Aug 2, 2017 at 12:23 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Tue, Aug 01, 2017 at 02:45:34PM -0700, Andrew Morton wrote:
>> On Tue,  1 Aug 2017 13:48:48 +0200 Arnd Bergmann <arnd@arndb.de> wrote:
>> > --- a/drivers/nvdimm/nd.h
>> > +++ b/drivers/nvdimm/nd.h
>> > @@ -392,8 +392,10 @@ static inline bool nd_iostat_start(struct bio *bio, unsigned long *start)
>> >  {
>> >     struct gendisk *disk = bio->bi_bdev->bd_disk;
>> >
>> > -   if (!blk_queue_io_stat(disk->queue))
>> > +   if (!blk_queue_io_stat(disk->queue)) {
>> > +           *start = 0;
>> >             return false;
>> > +   }
>> >
>> >     *start = jiffies;
>> >     generic_start_io_acct(bio_data_dir(bio),
>>
>> Well that's sad.
>>
>> The future of btt-remove-btt_rw_page.patch and friends is shrouded in
>> mystery, but if we proceed that way then yes, I guess we'll need to
>> work around such gcc glitches.
>>
>> But let's not leave apparently-unneeded code in place without telling
>> people why it is in fact needed?
>
> Maybe it's just cleaner to initialize 'start' in all the callers, so we don't
> have a mysterious line and have to remember why it's there / comment it?

I considered that but decided that would be worse, since it shuts up more
potential warnings about actual uninitialized use of the variable, and is
slightly harder for the compiler to optimize away. You also end up having
to add a comment in multiple places. Note that Andrew already added
a comment when he applied my patch to his mmotm tree.

     Arnd

Patch
diff mbox

--- a/drivers/nvdimm/nd.h~nvdimm-avoid-bogus-wmaybe-uninitialized-warning-fix
+++ a/drivers/nvdimm/nd.h
@@ -393,7 +393,7 @@  static inline bool nd_iostat_start(struc
 	struct gendisk *disk = bio->bi_bdev->bd_disk;
 
 	if (!blk_queue_io_stat(disk->queue)) {
-		*start = 0;
+		*start = 0;	/* Suppress bogus warning */
 		return false;
 	}