* [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