Linux-NVME Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] NVMe: silence GCC warning on 32 bit
       [not found] <1392714172-2712-1-git-send-email-geert@linux-m68k.org>
@ 2014-02-20 22:11 ` Paul Bolle
  2014-02-21 16:37   ` Keith Busch
  0 siblings, 1 reply; 11+ messages in thread
From: Paul Bolle @ 2014-02-20 22:11 UTC (permalink / raw)


On Tue, 2014-02-18@10:02 +0100, Geert Uytterhoeven wrote:
> *** WARNINGS ***
> 
> 188 regressions:
>   [...]
>   + /scratch/kisskb/src/drivers/block/nvme-core.c: warning: 'bvprv.bv_len' may be used uninitialized in this function [-Wuninitialized]:  => 514:18
>   + /scratch/kisskb/src/drivers/block/nvme-core.c: warning: 'bvprv.bv_offset' may be used uninitialized in this function [-Wuninitialized]:  => 514:18
>   + /scratch/kisskb/src/drivers/block/nvme-core.c: warning: 'bvprv.bv_page' may be used uninitialized in this function [-Wuninitialized]:  => 511:17

And these popped up in v3.14-rc1 on 32 bit x86. This patch makes these
warnings go away. Compile tested only (on 32 and 64 bit x86).

Review is appreciated, because the code I'm touching here is far from
obvious to me.
-------->8--------
From: Paul Bolle <pebolle@tiscali.nl>

Building nvme-core.o on 32 bit x86 triggers a rather impressive set of
GCC warnings:
    In file included from drivers/block/nvme-core.c:20:0:
    drivers/block/nvme-core.c: In function 'nvme_submit_bio_queue':
    include/linux/bio.h:154:55: warning: 'bvprv.bv_offset' may be used uninitialized in this function [-Wmaybe-uninitialized]
     #define bvec_to_phys(bv) (page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
                                                           ^
    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_offset' was declared here
      struct bio_vec bvec, bvprv;
                           ^
    In file included from drivers/block/nvme-core.c:20:0:
    include/linux/bio.h:154:55: warning: 'bvprv.bv_len' may be used uninitialized in this function [-Wmaybe-uninitialized]
     #define bvec_to_phys(bv) (page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
                                                           ^
    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_len' was declared here
      struct bio_vec bvec, bvprv;
                           ^
    In file included from [...]/arch/x86/include/asm/page.h:70:0,
                     from [...]/arch/x86/include/asm/processor.h:17,
                     from [...]/arch/x86/include/asm/atomic.h:6,
                     from include/linux/atomic.h:4,
                     from include/linux/mutex.h:18,
                     from include/linux/kernfs.h:13,
                     from include/linux/sysfs.h:15,
                     from include/linux/kobject.h:21,
                     from include/linux/pci.h:28,
                     from include/linux/nvme.h:23,
                     from drivers/block/nvme-core.c:19:
    include/asm-generic/memory_model.h:31:53: warning: 'bvprv.bv_page' may be used uninitialized in this function [-Wmaybe-uninitialized]
     #define __page_to_pfn(page) ((unsigned long)((page) - mem_map) + \
                                                         ^
    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_page' was declared here
      struct bio_vec bvec, bvprv;
                           ^

These are false positives. A bit of staring at the code reveals that
"struct bio_vec bvprv" and "int first" operate in lockstep: if first is
1 bvprv isn't yet initialized and if first is 0 bvprv will be
initialized. But if we convert bvprv to a pointer and initialize it to
NULL we can do away with first. And it turns out the warning is gone if
we do that. So that appears to be enough to help GCC understand the
flow of this code.

Signed-off-by: Paul Bolle <pebolle at tiscali.nl>
---
 drivers/block/nvme-core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 51824d1..f9fb28b 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -495,11 +495,10 @@ static int nvme_split_and_submit(struct bio *bio, struct nvme_queue *nvmeq,
 static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 		struct bio *bio, enum dma_data_direction dma_dir, int psegs)
 {
-	struct bio_vec bvec, bvprv;
+	struct bio_vec bvec, *bvprv = NULL;
 	struct bvec_iter iter;
 	struct scatterlist *sg = NULL;
 	int length = 0, nsegs = 0, split_len = bio->bi_iter.bi_size;
-	int first = 1;
 
 	if (nvmeq->dev->stripe_size)
 		split_len = nvmeq->dev->stripe_size -
@@ -508,10 +507,10 @@ static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 
 	sg_init_table(iod->sg, psegs);
 	bio_for_each_segment(bvec, bio, iter) {
-		if (!first && BIOVEC_PHYS_MERGEABLE(&bvprv, &bvec)) {
+		if (bvprv && BIOVEC_PHYS_MERGEABLE(bvprv, &bvec)) {
 			sg->length += bvec.bv_len;
 		} else {
-			if (!first && BIOVEC_NOT_VIRT_MERGEABLE(&bvprv, &bvec))
+			if (bvprv && BIOVEC_NOT_VIRT_MERGEABLE(bvprv, &bvec))
 				return nvme_split_and_submit(bio, nvmeq,
 							     length);
 
@@ -524,8 +523,7 @@ static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 		if (split_len - length < bvec.bv_len)
 			return nvme_split_and_submit(bio, nvmeq, split_len);
 		length += bvec.bv_len;
-		bvprv = bvec;
-		first = 0;
+		bvprv = &bvec;
 	}
 	iod->nents = nsegs;
 	sg_mark_end(sg);
-- 
1.8.5.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH] NVMe: silence GCC warning on 32 bit
  2014-02-20 22:11 ` [PATCH] NVMe: silence GCC warning on 32 bit Paul Bolle
@ 2014-02-21 16:37   ` Keith Busch
  2014-03-04  9:36     ` [PATCH v2] " Paul Bolle
  0 siblings, 1 reply; 11+ messages in thread
From: Keith Busch @ 2014-02-21 16:37 UTC (permalink / raw)


On Thu, 20 Feb 2014, Paul Bolle wrote:
> On Tue, 2014-02-18@10:02 +0100, Geert Uytterhoeven wrote:

> And these popped up in v3.14-rc1 on 32 bit x86. This patch makes these
> warnings go away. Compile tested only (on 32 and 64 bit x86).
>
> Review is appreciated, because the code I'm touching here is far from
> obvious to me.
> -------->8--------
> From: Paul Bolle <pebolle at tiscali.nl>

> These are false positives. A bit of staring at the code reveals that
> "struct bio_vec bvprv" and "int first" operate in lockstep: if first is
> 1 bvprv isn't yet initialized and if first is 0 bvprv will be
> initialized. But if we convert bvprv to a pointer and initialize it to
> NULL we can do away with first. And it turns out the warning is gone if
> we do that. So that appears to be enough to help GCC understand the
> flow of this code.

That's pretty much how it was done before the bio_vec iterators were
merged, but I think there's a problem with this approach for this patch
(see below).

>
> Signed-off-by: Paul Bolle <pebolle at tiscali.nl>
> ---
> drivers/block/nvme-core.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index 51824d1..f9fb28b 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -495,11 +495,10 @@ static int nvme_split_and_submit(struct bio *bio, struct nvme_queue *nvmeq,
> static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
> 		struct bio *bio, enum dma_data_direction dma_dir, int psegs)
> {
> -	struct bio_vec bvec, bvprv;
> +	struct bio_vec bvec, *bvprv = NULL;
> 	struct bvec_iter iter;
> 	struct scatterlist *sg = NULL;
> 	int length = 0, nsegs = 0, split_len = bio->bi_iter.bi_size;
> -	int first = 1;
>
> 	if (nvmeq->dev->stripe_size)
> 		split_len = nvmeq->dev->stripe_size -
> @@ -508,10 +507,10 @@ static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
>
> 	sg_init_table(iod->sg, psegs);
> 	bio_for_each_segment(bvec, bio, iter) {
> -		if (!first && BIOVEC_PHYS_MERGEABLE(&bvprv, &bvec)) {
> +		if (bvprv && BIOVEC_PHYS_MERGEABLE(bvprv, &bvec)) {
> 			sg->length += bvec.bv_len;
> 		} else {
> -			if (!first && BIOVEC_NOT_VIRT_MERGEABLE(&bvprv, &bvec))
> +			if (bvprv && BIOVEC_NOT_VIRT_MERGEABLE(bvprv, &bvec))
> 				return nvme_split_and_submit(bio, nvmeq,
> 							     length);
>
> @@ -524,8 +523,7 @@ static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
> 		if (split_len - length < bvec.bv_len)
> 			return nvme_split_and_submit(bio, nvmeq, split_len);
> 		length += bvec.bv_len;
> -		bvprv = bvec;
> -		first = 0;
> +		bvprv = &bvec;

The address of bvec doesn't change, so bvprv is still going to point
to bvec on the next iteration instead of the previous bio_vec like we
want. When the next iteration gets to this comparison:

> +		if (bvprv && BIOVEC_PHYS_MERGEABLE(bvprv, &bvec)) {

both bio_vec's have the same address.

> 	}
> 	iod->nents = nsegs;
> 	sg_mark_end(sg);
> -- 
> 1.8.5.3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-02-21 16:37   ` Keith Busch
@ 2014-03-04  9:36     ` Paul Bolle
  2014-03-05 15:09       ` Keith Busch
  2014-03-24 13:11       ` Matthew Wilcox
  0 siblings, 2 replies; 11+ messages in thread
From: Paul Bolle @ 2014-03-04  9:36 UTC (permalink / raw)


Building nvme-core.o on 32 bit x86 triggers a rather impressive set of
GCC warnings:
    In file included from drivers/block/nvme-core.c:20:0:
    drivers/block/nvme-core.c: In function 'nvme_submit_bio_queue':
    include/linux/bio.h:154:55: warning: 'bvprv.bv_offset' may be used uninitialized in this function [-Wmaybe-uninitialized]
     #define bvec_to_phys(bv) (page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
                                                           ^
    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_offset' was declared here
      struct bio_vec bvec, bvprv;
                           ^
    In file included from drivers/block/nvme-core.c:20:0:
    include/linux/bio.h:154:55: warning: 'bvprv.bv_len' may be used uninitialized in this function [-Wmaybe-uninitialized]
     #define bvec_to_phys(bv) (page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
                                                           ^
    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_len' was declared here
      struct bio_vec bvec, bvprv;
                           ^
    In file included from [...]/arch/x86/include/asm/page.h:70:0,
                     from [...]/arch/x86/include/asm/processor.h:17,
                     from [...]/arch/x86/include/asm/atomic.h:6,
                     from include/linux/atomic.h:4,
                     from include/linux/mutex.h:18,
                     from include/linux/kernfs.h:13,
                     from include/linux/sysfs.h:15,
                     from include/linux/kobject.h:21,
                     from include/linux/pci.h:28,
                     from include/linux/nvme.h:23,
                     from drivers/block/nvme-core.c:19:
    include/asm-generic/memory_model.h:31:53: warning: 'bvprv.bv_page' may be used uninitialized in this function [-Wmaybe-uninitialized]
     #define __page_to_pfn(page) ((unsigned long)((page) - mem_map) + \
                                                         ^
    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_page' was declared here
      struct bio_vec bvec, bvprv;
                           ^

These are false positives. Apparently GCC can't determine that bvprv
will only be used if first is false and has, therefore, been
initialized. It turned out hard to reorganize the code to help GCC
understand the flow of this code. So take the easy way out and
initialize bvprv to { NULL }.

And, since we're touching this code, make first a bool.

Signed-off-by: Paul Bolle <pebolle at tiscali.nl>
---
v2: redone, as required by Keith's review. Note that initializing bvprv
to { NULL } is already done twice in block/blk-merge.c, which inspired
me to take the easy way out here.

Also note that it's actually not clear to me why these warnings only
trigger on 32 bit. I guess there's some int/long conversion lurking
somewhere. I haven't found it. 

bvprv? Would that mean bio_vec private? But it looks like some temporary
variable, so something like tmp may make more sense. Anyhow, still
compile tested only (on 32 and 64 bit x86).

 drivers/block/nvme-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index 51824d1..60f98be 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -495,11 +495,11 @@ static int nvme_split_and_submit(struct bio *bio, struct nvme_queue *nvmeq,
 static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 		struct bio *bio, enum dma_data_direction dma_dir, int psegs)
 {
-	struct bio_vec bvec, bvprv;
+	struct bio_vec bvec, bvprv = { NULL };
 	struct bvec_iter iter;
 	struct scatterlist *sg = NULL;
 	int length = 0, nsegs = 0, split_len = bio->bi_iter.bi_size;
-	int first = 1;
+	bool first = true;
 
 	if (nvmeq->dev->stripe_size)
 		split_len = nvmeq->dev->stripe_size -
@@ -525,7 +525,7 @@ static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
 			return nvme_split_and_submit(bio, nvmeq, split_len);
 		length += bvec.bv_len;
 		bvprv = bvec;
-		first = 0;
+		first = false;
 	}
 	iod->nents = nsegs;
 	sg_mark_end(sg);
-- 
1.8.5.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-03-04  9:36     ` [PATCH v2] " Paul Bolle
@ 2014-03-05 15:09       ` Keith Busch
  2014-03-06  9:56         ` Paul Bolle
  2014-03-24 13:11       ` Matthew Wilcox
  1 sibling, 1 reply; 11+ messages in thread
From: Keith Busch @ 2014-03-05 15:09 UTC (permalink / raw)


Looks good to me. This won't apply in linux-nvme yet and it may be a
little while before it does, so this might be considered to go upstream
through a different tree if you want this in sooner.

On Tue, 4 Mar 2014, Paul Bolle wrote:
> Building nvme-core.o on 32 bit x86 triggers a rather impressive set of
> GCC warnings:
>    In file included from drivers/block/nvme-core.c:20:0:
>    drivers/block/nvme-core.c: In function 'nvme_submit_bio_queue':
>    include/linux/bio.h:154:55: warning: 'bvprv.bv_offset' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     #define bvec_to_phys(bv) (page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
>                                                           ^
>    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_offset' was declared here
>      struct bio_vec bvec, bvprv;
>                           ^
>    In file included from drivers/block/nvme-core.c:20:0:
>    include/linux/bio.h:154:55: warning: 'bvprv.bv_len' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     #define bvec_to_phys(bv) (page_to_phys((bv)->bv_page) + (unsigned long) (bv)->bv_offset)
>                                                           ^
>    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_len' was declared here
>      struct bio_vec bvec, bvprv;
>                           ^
>    In file included from [...]/arch/x86/include/asm/page.h:70:0,
>                     from [...]/arch/x86/include/asm/processor.h:17,
>                     from [...]/arch/x86/include/asm/atomic.h:6,
>                     from include/linux/atomic.h:4,
>                     from include/linux/mutex.h:18,
>                     from include/linux/kernfs.h:13,
>                     from include/linux/sysfs.h:15,
>                     from include/linux/kobject.h:21,
>                     from include/linux/pci.h:28,
>                     from include/linux/nvme.h:23,
>                     from drivers/block/nvme-core.c:19:
>    include/asm-generic/memory_model.h:31:53: warning: 'bvprv.bv_page' may be used uninitialized in this function [-Wmaybe-uninitialized]
>     #define __page_to_pfn(page) ((unsigned long)((page) - mem_map) + \
>                                                         ^
>    drivers/block/nvme-core.c:498:23: note: 'bvprv.bv_page' was declared here
>      struct bio_vec bvec, bvprv;
>                           ^
>
> These are false positives. Apparently GCC can't determine that bvprv
> will only be used if first is false and has, therefore, been
> initialized. It turned out hard to reorganize the code to help GCC
> understand the flow of this code. So take the easy way out and
> initialize bvprv to { NULL }.
>
> And, since we're touching this code, make first a bool.
>
> Signed-off-by: Paul Bolle <pebolle at tiscali.nl>
> ---
> v2: redone, as required by Keith's review. Note that initializing bvprv
> to { NULL } is already done twice in block/blk-merge.c, which inspired
> me to take the easy way out here.
>
> Also note that it's actually not clear to me why these warnings only
> trigger on 32 bit. I guess there's some int/long conversion lurking
> somewhere. I haven't found it.
>
> bvprv? Would that mean bio_vec private? But it looks like some temporary
> variable, so something like tmp may make more sense. Anyhow, still
> compile tested only (on 32 and 64 bit x86).

bvprv == bio_vec previous

> drivers/block/nvme-core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
> index 51824d1..60f98be 100644
> --- a/drivers/block/nvme-core.c
> +++ b/drivers/block/nvme-core.c
> @@ -495,11 +495,11 @@ static int nvme_split_and_submit(struct bio *bio, struct nvme_queue *nvmeq,
> static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
> 		struct bio *bio, enum dma_data_direction dma_dir, int psegs)
> {
> -	struct bio_vec bvec, bvprv;
> +	struct bio_vec bvec, bvprv = { NULL };
> 	struct bvec_iter iter;
> 	struct scatterlist *sg = NULL;
> 	int length = 0, nsegs = 0, split_len = bio->bi_iter.bi_size;
> -	int first = 1;
> +	bool first = true;
>
> 	if (nvmeq->dev->stripe_size)
> 		split_len = nvmeq->dev->stripe_size -
> @@ -525,7 +525,7 @@ static int nvme_map_bio(struct nvme_queue *nvmeq, struct nvme_iod *iod,
> 			return nvme_split_and_submit(bio, nvmeq, split_len);
> 		length += bvec.bv_len;
> 		bvprv = bvec;
> -		first = 0;
> +		first = false;
> 	}
> 	iod->nents = nsegs;
> 	sg_mark_end(sg);
> -- 
> 1.8.5.3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-03-05 15:09       ` Keith Busch
@ 2014-03-06  9:56         ` Paul Bolle
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Bolle @ 2014-03-06  9:56 UTC (permalink / raw)


On Wed, 2014-03-05@08:09 -0700, Keith Busch wrote:
> Looks good to me.

Thanks.

> This won't apply in linux-nvme yet and it may be a
> little while before it does, so this might be considered to go upstream
> through a different tree if you want this in sooner.

These warnings popped up in v3.14-rc1. (In v3.13 bvprv was a pointer
initialized to NULL.) This patch is not urgent, but it would still be
nice if the warnings were silenced in v3.14, since they are so noisy. Is
there any tree that queues NVMe fixes to be applied during this release
cycle?

> On Tue, 4 Mar 2014, Paul Bolle wrote:
> > bvprv? Would that mean bio_vec private? But it looks like some temporary
> > variable, so something like tmp may make more sense. Anyhow, still
> > compile tested only (on 32 and 64 bit x86).
> 
> bvprv == bio_vec previous

Thanks. So perhaps that variable could be renamed to previous (or its
more common alias prev),


Paul Bolle

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-03-04  9:36     ` [PATCH v2] " Paul Bolle
  2014-03-05 15:09       ` Keith Busch
@ 2014-03-24 13:11       ` Matthew Wilcox
  2014-03-24 13:31         ` Matthew Wilcox
  1 sibling, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2014-03-24 13:11 UTC (permalink / raw)


On Tue, Mar 04, 2014@10:36:07AM +0100, Paul Bolle wrote:
> Also note that it's actually not clear to me why these warnings only
> trigger on 32 bit. I guess there's some int/long conversion lurking
> somewhere. I haven't found it. 

I bet you're using a different revision of the compiler on 32 and 64 bit.
Different gcc revisions have different abilities to track initialisation.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-03-24 13:11       ` Matthew Wilcox
@ 2014-03-24 13:31         ` Matthew Wilcox
  2014-03-24 15:36           ` Paul Bolle
  0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2014-03-24 13:31 UTC (permalink / raw)


On Mon, Mar 24, 2014@09:11:04AM -0400, Matthew Wilcox wrote:
> On Tue, Mar 04, 2014@10:36:07AM +0100, Paul Bolle wrote:
> > Also note that it's actually not clear to me why these warnings only
> > trigger on 32 bit. I guess there's some int/long conversion lurking
> > somewhere. I haven't found it. 
> 
> I bet you're using a different revision of the compiler on 32 and 64 bit.
> Different gcc revisions have different abilities to track initialisation.

I should try things myself before opening my big mouth.  Weird.  Using
gcc-4.8, I see the same thing.  Guess I should just apply the patch,
though it feels wrong to be initialising an entire struct just to silence
a bogus compiler warning :-(

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-03-24 13:31         ` Matthew Wilcox
@ 2014-03-24 15:36           ` Paul Bolle
  2014-03-24 15:49             ` Geert Uytterhoeven
  2014-05-08  7:12             ` Paul Bolle
  0 siblings, 2 replies; 11+ messages in thread
From: Paul Bolle @ 2014-03-24 15:36 UTC (permalink / raw)


On Mon, 2014-03-24@09:31 -0400, Matthew Wilcox wrote:
> I should try things myself before opening my big mouth.  Weird.  Using
> gcc-4.8, I see the same thing.  Guess I should just apply the patch,
> though it feels wrong to be initialising an entire struct just to silence
> a bogus compiler warning :-(

I noticed this difference on a 32 bit x86 machine and a 64 bit x86
machine that are both running Fedora 20. They both should be at
gcc-4.8.2 for quite some time now (if I grepped the yum log correctly).

Anyhow, the warning on 32 bit is rather noisy, so I wanted it gone. But
my comments should make clear I'm not really happy with this patch. 

And as this is now unlikely to be in time for v3.14, we might decide to
dig deeper. It won't be the first time that a rather small change (say,
converting a variable from signed to unsigned) turns out be enough to
make GCC understand the flow of the code.

Thanks,


Paul Bolle

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-03-24 15:36           ` Paul Bolle
@ 2014-03-24 15:49             ` Geert Uytterhoeven
  2014-03-24 15:57               ` Paul Bolle
  2014-05-08  7:12             ` Paul Bolle
  1 sibling, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2014-03-24 15:49 UTC (permalink / raw)


Hi Paul,

On Mon, Mar 24, 2014@4:36 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> And as this is now unlikely to be in time for v3.14, we might decide to
> dig deeper. It won't be the first time that a rather small change (say,
> converting a variable from signed to unsigned) turns out be enough to
> make GCC understand the flow of the code.

Anything we can do with factoring out the "if (!first)" in both branches?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-03-24 15:49             ` Geert Uytterhoeven
@ 2014-03-24 15:57               ` Paul Bolle
  0 siblings, 0 replies; 11+ messages in thread
From: Paul Bolle @ 2014-03-24 15:57 UTC (permalink / raw)


On Mon, 2014-03-24@16:49 +0100, Geert Uytterhoeven wrote:
> On Mon, Mar 24, 2014@4:36 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> > And as this is now unlikely to be in time for v3.14, we might decide to
> > dig deeper. It won't be the first time that a rather small change (say,
> > converting a variable from signed to unsigned) turns out be enough to
> > make GCC understand the flow of the code.
> 
> Anything we can do with factoring out the "if (!first)" in both branches?

That was basically my approach in version one: drop "first" all
together. Worked great: the warning was gone. But Keith noted that I
also completely broke the logic of the code. So I decided, after some
further attempts, to take the easy way out and and initialize "bvprv" to
{ NULL }.


Paul Bolle

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] NVMe: silence GCC warning on 32 bit
  2014-03-24 15:36           ` Paul Bolle
  2014-03-24 15:49             ` Geert Uytterhoeven
@ 2014-05-08  7:12             ` Paul Bolle
  1 sibling, 0 replies; 11+ messages in thread
From: Paul Bolle @ 2014-05-08  7:12 UTC (permalink / raw)


Matthew,

Paul Bolle schreef op ma 24-03-2014 om 16:36 [+0100]:
> On Mon, 2014-03-24@09:31 -0400, Matthew Wilcox wrote:
> > I should try things myself before opening my big mouth.  Weird.  Using
> > gcc-4.8, I see the same thing.  Guess I should just apply the patch,
> > though it feels wrong to be initialising an entire struct just to silence
> > a bogus compiler warning :-(
> 
> I noticed this difference on a 32 bit x86 machine and a 64 bit x86
> machine that are both running Fedora 20. They both should be at
> gcc-4.8.2 for quite some time now (if I grepped the yum log correctly).
> 
> Anyhow, the warning on 32 bit is rather noisy, so I wanted it gone. But
> my comments should make clear I'm not really happy with this patch. 
> 
> And as this is now unlikely to be in time for v3.14, we might decide to
> dig deeper. It won't be the first time that a rather small change (say,
> converting a variable from signed to unsigned) turns out be enough to
> make GCC understand the flow of the code.

This noisy warning is still seen when compiling v3.15-rc4 for x86 (32
bit, that is). Assuming this patch is not queued anywhere: is the
unsophisticated approach of my v2 acceptable or would you like me to try
and find the cause of this warning?


Paul Bolle

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-05-08  7:12 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1392714172-2712-1-git-send-email-geert@linux-m68k.org>
2014-02-20 22:11 ` [PATCH] NVMe: silence GCC warning on 32 bit Paul Bolle
2014-02-21 16:37   ` Keith Busch
2014-03-04  9:36     ` [PATCH v2] " Paul Bolle
2014-03-05 15:09       ` Keith Busch
2014-03-06  9:56         ` Paul Bolle
2014-03-24 13:11       ` Matthew Wilcox
2014-03-24 13:31         ` Matthew Wilcox
2014-03-24 15:36           ` Paul Bolle
2014-03-24 15:49             ` Geert Uytterhoeven
2014-03-24 15:57               ` Paul Bolle
2014-05-08  7:12             ` Paul Bolle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox