* [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning
@ 2017-08-01 11:48 Arnd Bergmann
2017-08-01 18:00 ` Ross Zwisler
2017-08-01 21:45 ` Andrew Morton
0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-08-01 11:48 UTC (permalink / raw)
To: Ross Zwisler, Andrew Morton, Dan Williams
Cc: Christoph Hellwig, Arnd Bergmann, Vishal Verma, Toshi Kani,
Johannes Thumshirn, linux-nvdimm, linux-kernel
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.
Fixes: 503a5e89b1de ("drivers/nvdimm/btt.c: remove btt_rw_page()")
Fixes: 58100d6e735e ("drivers/nvdimm/pmem.c: remove pmem_rw_page()")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/nvdimm/nd.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/nvdimm/nd.h b/drivers/nvdimm/nd.h
index e1b5715bd91f..64f79a156456 100644
--- 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),
--
2.9.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning
2017-08-01 11:48 [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning Arnd Bergmann
@ 2017-08-01 18:00 ` Ross Zwisler
2017-08-01 18:07 ` Dan Williams
2017-08-01 21:45 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Ross Zwisler @ 2017-08-01 18:00 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ross Zwisler, Andrew Morton, Dan Williams, Christoph Hellwig,
Vishal Verma, Toshi Kani, Johannes Thumshirn, linux-nvdimm,
linux-kernel
On Tue, Aug 01, 2017 at 01:48:48PM +0200, Arnd Bergmann 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.
>
> Fixes: 503a5e89b1de ("drivers/nvdimm/btt.c: remove btt_rw_page()")
> Fixes: 58100d6e735e ("drivers/nvdimm/pmem.c: remove pmem_rw_page()")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
This change looks fine:
Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
I believe the patches removing the btt_rw_page() and btt_rw_page() are on hold
until I can get some performance numbers to justify them.
Dan, do you want to take this as is, or do you want me to include it in my
larger rw_page() series if/when that gets revived?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning
2017-08-01 18:00 ` Ross Zwisler
@ 2017-08-01 18:07 ` Dan Williams
0 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2017-08-01 18:07 UTC (permalink / raw)
To: Ross Zwisler
Cc: Arnd Bergmann, Andrew Morton, Christoph Hellwig, Vishal Verma,
Toshi Kani, Johannes Thumshirn, linux-nvdimm@lists.01.org,
linux-kernel@vger.kernel.org
On Tue, Aug 1, 2017 at 11:00 AM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Tue, Aug 01, 2017 at 01:48:48PM +0200, Arnd Bergmann 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.
>>
>> Fixes: 503a5e89b1de ("drivers/nvdimm/btt.c: remove btt_rw_page()")
>> Fixes: 58100d6e735e ("drivers/nvdimm/pmem.c: remove pmem_rw_page()")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> This change looks fine:
>
> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>
> I believe the patches removing the btt_rw_page() and btt_rw_page() are on hold
> until I can get some performance numbers to justify them.
>
> Dan, do you want to take this as is, or do you want me to include it in my
> larger rw_page() series if/when that gets revived?
I'd say include it with your set.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning
2017-08-01 11:48 [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning Arnd Bergmann
2017-08-01 18:00 ` Ross Zwisler
@ 2017-08-01 21:45 ` Andrew Morton
2017-08-01 22:23 ` Ross Zwisler
1 sibling, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2017-08-01 21:45 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ross Zwisler, Dan Williams, Christoph Hellwig, Vishal Verma,
Toshi Kani, Johannes Thumshirn, linux-nvdimm, linux-kernel
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?
--- 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;
}
_
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning
2017-08-01 21:45 ` Andrew Morton
@ 2017-08-01 22:23 ` Ross Zwisler
2017-08-02 10:54 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Ross Zwisler @ 2017-08-01 22:23 UTC (permalink / raw)
To: Andrew Morton
Cc: Arnd Bergmann, Ross Zwisler, Dan Williams, Christoph Hellwig,
Vishal Verma, Toshi Kani, Johannes Thumshirn, linux-nvdimm,
linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning
2017-08-01 22:23 ` Ross Zwisler
@ 2017-08-02 10:54 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-08-02 10:54 UTC (permalink / raw)
To: Ross Zwisler
Cc: Andrew Morton, Dan Williams, Christoph Hellwig, Vishal Verma,
Toshi Kani, Johannes Thumshirn, linux-nvdimm,
Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-02 10:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-01 11:48 [PATCH] nvdimm: avoid bogus -Wmaybe-uninitialized warning Arnd Bergmann
2017-08-01 18:00 ` Ross Zwisler
2017-08-01 18:07 ` Dan Williams
2017-08-01 21:45 ` Andrew Morton
2017-08-01 22:23 ` Ross Zwisler
2017-08-02 10:54 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox