[PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Maciej W. Rozycki]

 Add error messages to the probe call.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
 While they may rarely trigger, they may be useful when something weird is 
going on.  Also this is good style.

 Checked with checkpatch.pl and at the runtime.

 Please apply; don't be worried about the old version number.

  Maciej

patch-mips-2.6.18-20060920-pmag-ba-err-1
diff -up --recursive --new-file linux-mips-2.6.18-20060920.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.18-20060920/drivers/video/pmag-ba-fb.c
--- linux-mips-2.6.18-20060920.macro/drivers/video/pmag-ba-fb.c	2006-12-16 16:45:08.000000000 +0000
+++ linux-mips-2.6.18-20060920/drivers/video/pmag-ba-fb.c	2006-12-16 16:45:23.000000000 +0000
@@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 	resource_size_t start, len;
 	struct fb_info *info;
 	struct pmagbafb_par *par;
+	int err = 0;
 
 	info = framebuffer_alloc(sizeof(struct pmagbafb_par), dev);
-	if (!info)
+	if (!info) {
+		printk(KERN_ERR "%s: Cannot allocate memory\n", dev->bus_id);
 		return -ENOMEM;
+	}
 
 	par = info->par;
 	dev_set_drvdata(dev, info);
 
-	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0)
+	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
+		printk(KERN_ERR "%s: Cannot allocate color map\n",
+		       dev->bus_id);
+		err = -ENOMEM;
 		goto err_alloc;
+	}
 
 	info->fbops = &pmagbafb_ops;
 	info->fix = pmagbafb_fix;
@@ -166,28 +173,41 @@ static int __init pmagbafb_probe(struct 
 	/* Request the I/O MEM resource.  */
 	start = tdev->resource.start;
 	len = tdev->resource.end - start + 1;
-	if (!request_mem_region(start, len, dev->bus_id))
+	if (!request_mem_region(start, len, dev->bus_id)) {
+		printk(KERN_ERR "%s: Cannot reserve FB region\n", dev->bus_id);
+		err = -EBUSY;
 		goto err_cmap;
+	}
 
 	/* MMIO mapping setup.  */
 	info->fix.mmio_start = start;
 	par->mmio = ioremap_nocache(info->fix.mmio_start, info->fix.mmio_len);
-	if (!par->mmio)
+	if (!par->mmio) {
+		printk(KERN_ERR "%s: Cannot map MMIO\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_resource;
+	}
 	par->dac = par->mmio + PMAG_BA_BT459;
 
 	/* Frame buffer mapping setup.  */
 	info->fix.smem_start = start + PMAG_BA_FBMEM;
 	info->screen_base = ioremap_nocache(info->fix.smem_start,
 					    info->fix.smem_len);
-	if (!info->screen_base)
+	if (!info->screen_base) {
+		printk(KERN_ERR "%s: Cannot map FB\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_mmio_map;
+	}
 	info->screen_size = info->fix.smem_len;
 
 	pmagbafb_erase_cursor(info);
 
-	if (register_framebuffer(info) < 0)
+	err = register_framebuffer(info);
+	if (err < 0) {
+		printk(KERN_ERR "%s: Cannot register framebuffer\n",
+		       dev->bus_id);
 		goto err_smem_map;
+	}
 
 	get_device(dev);
 

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Maciej W. Rozycki]

 Add error messages to the probe call.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
 While they may rarely trigger, they may be useful when something weird is 
going on.  Also this is good style.

 This is an updated version that addresses an issue raised by Mariusz 
Kozlowski for the sibling patch.  Checked with checkpatch.pl.

 Please apply.

  Maciej

patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
--- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
+++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
@@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 	resource_size_t start, len;
 	struct fb_info *info;
 	struct pmagbafb_par *par;
+	int err = 0;
 
 	info = framebuffer_alloc(sizeof(struct pmagbafb_par), dev);
-	if (!info)
+	if (!info) {
+		printk(KERN_ERR "%s: Cannot allocate memory\n", dev->bus_id);
 		return -ENOMEM;
+	}
 
 	par = info->par;
 	dev_set_drvdata(dev, info);
 
-	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0)
+	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
+		printk(KERN_ERR "%s: Cannot allocate color map\n",
+		       dev->bus_id);
+		err = -ENOMEM;
 		goto err_alloc;
+	}
 
 	info->fbops = &pmagbafb_ops;
 	info->fix = pmagbafb_fix;
@@ -166,28 +173,41 @@ static int __init pmagbafb_probe(struct 
 	/* Request the I/O MEM resource.  */
 	start = tdev->resource.start;
 	len = tdev->resource.end - start + 1;
-	if (!request_mem_region(start, len, dev->bus_id))
+	if (!request_mem_region(start, len, dev->bus_id)) {
+		printk(KERN_ERR "%s: Cannot reserve FB region\n", dev->bus_id);
+		err = -EBUSY;
 		goto err_cmap;
+	}
 
 	/* MMIO mapping setup.  */
 	info->fix.mmio_start = start;
 	par->mmio = ioremap_nocache(info->fix.mmio_start, info->fix.mmio_len);
-	if (!par->mmio)
+	if (!par->mmio) {
+		printk(KERN_ERR "%s: Cannot map MMIO\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_resource;
+	}
 	par->dac = par->mmio + PMAG_BA_BT459;
 
 	/* Frame buffer mapping setup.  */
 	info->fix.smem_start = start + PMAG_BA_FBMEM;
 	info->screen_base = ioremap_nocache(info->fix.smem_start,
 					    info->fix.smem_len);
-	if (!info->screen_base)
+	if (!info->screen_base) {
+		printk(KERN_ERR "%s: Cannot map FB\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_mmio_map;
+	}
 	info->screen_size = info->fix.smem_len;
 
 	pmagbafb_erase_cursor(info);
 
-	if (register_framebuffer(info) < 0)
+	err = register_framebuffer(info);
+	if (err < 0) {
+		printk(KERN_ERR "%s: Cannot register framebuffer\n",
+		       dev->bus_id);
 		goto err_smem_map;
+	}
 
 	get_device(dev);
 
@@ -211,7 +231,7 @@ err_cmap:
 
 err_alloc:
 	framebuffer_release(info);
-	return -ENXIO;
+	return err;
 }
 
 static int __exit pmagbafb_remove(struct device *dev)

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Maciej W. Rozycki]

 Add error messages to the probe call.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
---
 While they may rarely trigger, they may be useful when something weird is 
going on.  Also this is good style.

 This is an updated version that addresses an issue raised by Mariusz 
Kozlowski for the sibling patch.  Checked with checkpatch.pl.

 Please apply.

  Maciej

patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
--- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
+++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
@@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
 	resource_size_t start, len;
 	struct fb_info *info;
 	struct pmagbafb_par *par;
+	int err = 0;
 
 	info = framebuffer_alloc(sizeof(struct pmagbafb_par), dev);
-	if (!info)
+	if (!info) {
+		printk(KERN_ERR "%s: Cannot allocate memory\n", dev->bus_id);
 		return -ENOMEM;
+	}
 
 	par = info->par;
 	dev_set_drvdata(dev, info);
 
-	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0)
+	if (fb_alloc_cmap(&info->cmap, 256, 0) < 0) {
+		printk(KERN_ERR "%s: Cannot allocate color map\n",
+		       dev->bus_id);
+		err = -ENOMEM;
 		goto err_alloc;
+	}
 
 	info->fbops = &pmagbafb_ops;
 	info->fix = pmagbafb_fix;
@@ -166,28 +173,41 @@ static int __init pmagbafb_probe(struct 
 	/* Request the I/O MEM resource.  */
 	start = tdev->resource.start;
 	len = tdev->resource.end - start + 1;
-	if (!request_mem_region(start, len, dev->bus_id))
+	if (!request_mem_region(start, len, dev->bus_id)) {
+		printk(KERN_ERR "%s: Cannot reserve FB region\n", dev->bus_id);
+		err = -EBUSY;
 		goto err_cmap;
+	}
 
 	/* MMIO mapping setup.  */
 	info->fix.mmio_start = start;
 	par->mmio = ioremap_nocache(info->fix.mmio_start, info->fix.mmio_len);
-	if (!par->mmio)
+	if (!par->mmio) {
+		printk(KERN_ERR "%s: Cannot map MMIO\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_resource;
+	}
 	par->dac = par->mmio + PMAG_BA_BT459;
 
 	/* Frame buffer mapping setup.  */
 	info->fix.smem_start = start + PMAG_BA_FBMEM;
 	info->screen_base = ioremap_nocache(info->fix.smem_start,
 					    info->fix.smem_len);
-	if (!info->screen_base)
+	if (!info->screen_base) {
+		printk(KERN_ERR "%s: Cannot map FB\n", dev->bus_id);
+		err = -ENOMEM;
 		goto err_mmio_map;
+	}
 	info->screen_size = info->fix.smem_len;
 
 	pmagbafb_erase_cursor(info);
 
-	if (register_framebuffer(info) < 0)
+	err = register_framebuffer(info);
+	if (err < 0) {
+		printk(KERN_ERR "%s: Cannot register framebuffer\n",
+		       dev->bus_id);
 		goto err_smem_map;
+	}
 
 	get_device(dev);
 
@@ -211,7 +231,7 @@ err_cmap:
 
 err_alloc:
 	framebuffer_release(info);
-	return -ENXIO;
+	return err;
 }
 
 static int __exit pmagbafb_remove(struct device *dev)

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Andrew Morton]

On Tue, 18 Sep 2007 13:18:34 +0100 (BST)
"Maciej W. Rozycki" <macro@linux-mips.org> wrote:

>  Add error messages to the probe call.
> 
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> ---
>  While they may rarely trigger, they may be useful when something weird is 
> going on.  Also this is good style.
> 
>  This is an updated version that addresses an issue raised by Mariusz 
> Kozlowski for the sibling patch.  Checked with checkpatch.pl.
> 
>  Please apply.
> 
>   Maciej
> 
> patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
> diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
> --- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
> +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
> @@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
>  	resource_size_t start, len;
>  	struct fb_info *info;
>  	struct pmagbafb_par *par;
> +	int err = 0;

This initialisation to zero is not good.

Because if some error-path code forgot to do `err = -EFOO' then probe()
will return zero and the driver will leave things in half-initialised state
and will then proceed as if things had succeeded.  It will crash.

So it's better to leave this local uninitialised, because we really want to
get that compiler warning if someone forgot to set the return value.

I made that change, but am too stupid to be able to work out how to create
a config which will let me compile this thing.

akpm:/usr/src/25> grep PMAG arch/arm/configs/*
akpm:/usr/src/25> 

bah.

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Andrew Morton]

On Tue, 18 Sep 2007 13:18:34 +0100 (BST)
"Maciej W. Rozycki" <macro@linux-mips.org> wrote:

>  Add error messages to the probe call.
> 
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
> ---
>  While they may rarely trigger, they may be useful when something weird is 
> going on.  Also this is good style.
> 
>  This is an updated version that addresses an issue raised by Mariusz 
> Kozlowski for the sibling patch.  Checked with checkpatch.pl.
> 
>  Please apply.
> 
>   Maciej
> 
> patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
> diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
> --- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
> +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
> @@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
>  	resource_size_t start, len;
>  	struct fb_info *info;
>  	struct pmagbafb_par *par;
> +	int err = 0;

This initialisation to zero is not good.

Because if some error-path code forgot to do `err = -EFOO' then probe()
will return zero and the driver will leave things in half-initialised state
and will then proceed as if things had succeeded.  It will crash.

So it's better to leave this local uninitialised, because we really want to
get that compiler warning if someone forgot to set the return value.

I made that change, but am too stupid to be able to work out how to create
a config which will let me compile this thing.

akpm:/usr/src/25> grep PMAG arch/arm/configs/*
akpm:/usr/src/25> 

bah.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Martin Michlmayr]

* Andrew Morton <akpm@linux-foundation.org> [2007-09-19 17:24]:
> I made that change, but am too stupid to be able to work out how to create
> a config which will let me compile this thing.
> 
> akpm:/usr/src/25> grep PMAG arch/arm/configs/*
> akpm:/usr/src/25> 

It's a driver for mips.
-- 
Martin Michlmayr
http://www.cyrius.com/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Maciej W. Rozycki]

On Wed, 19 Sep 2007, Andrew Morton wrote:

> > patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
> > diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
> > --- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
> > +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
> > @@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
> >  	resource_size_t start, len;
> >  	struct fb_info *info;
> >  	struct pmagbafb_par *par;
> > +	int err = 0;
> 
> This initialisation to zero is not good.
> 
> Because if some error-path code forgot to do `err = -EFOO' then probe()
> will return zero and the driver will leave things in half-initialised state
> and will then proceed as if things had succeeded.  It will crash.

 GCC used to complain: "`foo' might be used uninitialized..." and this is 
the usual cure; let me see if this not the case anymore (I have 4.1.2).

> So it's better to leave this local uninitialised, because we really want to
> get that compiler warning if someone forgot to set the return value.

 Yes of course, barring the issue mentioned.  Note the message above is 
not the same as: "`foo' is used uninitialized..." that would be reported 
in the case which you are concerned of.

> I made that change, but am too stupid to be able to work out how to create
> a config which will let me compile this thing.
> 
> akpm:/usr/src/25> grep PMAG arch/arm/configs/*
> akpm:/usr/src/25> 

 TURBOchannel is currently MIPS only:

$ grep PMAG arch/mips/configs/*
arch/mips/configs/decstation_defconfig:# CONFIG_FB_PMAG_AA is not set
arch/mips/configs/decstation_defconfig:CONFIG_FB_PMAG_BA=y
arch/mips/configs/decstation_defconfig:CONFIG_FB_PMAGB_B=y
$

 Thanks for your review.

  Maciej

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Maciej W. Rozycki]

On Wed, 19 Sep 2007, Andrew Morton wrote:

> > patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
> > diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
> > --- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
> > +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
> > @@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
> >  	resource_size_t start, len;
> >  	struct fb_info *info;
> >  	struct pmagbafb_par *par;
> > +	int err = 0;
> 
> This initialisation to zero is not good.
> 
> Because if some error-path code forgot to do `err = -EFOO' then probe()
> will return zero and the driver will leave things in half-initialised state
> and will then proceed as if things had succeeded.  It will crash.

 GCC used to complain: "`foo' might be used uninitialized..." and this is 
the usual cure; let me see if this not the case anymore (I have 4.1.2).

> So it's better to leave this local uninitialised, because we really want to
> get that compiler warning if someone forgot to set the return value.

 Yes of course, barring the issue mentioned.  Note the message above is 
not the same as: "`foo' is used uninitialized..." that would be reported 
in the case which you are concerned of.

> I made that change, but am too stupid to be able to work out how to create
> a config which will let me compile this thing.
> 
> akpm:/usr/src/25> grep PMAG arch/arm/configs/*
> akpm:/usr/src/25> 

 TURBOchannel is currently MIPS only:

$ grep PMAG arch/mips/configs/*
arch/mips/configs/decstation_defconfig:# CONFIG_FB_PMAG_AA is not set
arch/mips/configs/decstation_defconfig:CONFIG_FB_PMAG_BA=y
arch/mips/configs/decstation_defconfig:CONFIG_FB_PMAGB_B=y
$

 Thanks for your review.

  Maciej

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

Hi Maciej,


On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
> 
> On Wed, 19 Sep 2007, Andrew Morton wrote:
> 
> > > patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2
> > > diff -up --recursive --new-file linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
> > > --- linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c	2007-02-21 05:56:47.000000000 +0000
> > > +++ linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c	2007-09-18 10:56:51.000000000 +0000
> > > @@ -147,16 +147,23 @@ static int __init pmagbafb_probe(struct 
> > >  	resource_size_t start, len;
> > >  	struct fb_info *info;
> > >  	struct pmagbafb_par *par;
> > > +	int err = 0;
> > 
> > This initialisation to zero is not good.
> > 
> > Because if some error-path code forgot to do `err = -EFOO' then probe()
> > will return zero and the driver will leave things in half-initialised state
> > and will then proceed as if things had succeeded.  It will crash.
> 
>  GCC used to complain: "`foo' might be used uninitialized..." and this is 
> the usual cure; let me see if this not the case anymore (I have 4.1.2).

Even so, initializing to zero isn't quite good. You could use the
uninitialized_var() (once you've confirmed that the warning is bogus).
However, some maintainers may still nack uninitialized_var() usage,
quite legitimately.


> > So it's better to leave this local uninitialised, because we really want to
> > get that compiler warning if someone forgot to set the return value.
> 
>  Yes of course, barring the issue mentioned.  Note the message above is 
> not the same as: "`foo' is used uninitialized..." that would be reported 
> in the case which you are concerned of.

Firstly, "may be used uninitialized" can still be a bug.

Secondly, latest gcc is *horribly* buggy (and has been so for last several
releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501

We'd been hurling all sorts of abuses on gcc for quite long (when it fails
to detect these "false positive" cases), but now, it turns out it is quite
easy to write *genuinely* buggy code that still won't get any warnings,
neither the "is used" nor "may be used" one!

In short, there are three ways to fix these false positive warnings:

1. Do nothing, there are enough "uninitialized variable" warnings anyway,
   and hopefully, one day GCC would clean up its act.

2. Use uninitialized_var() to shut it up (only if it's genuinely bogus).

3. Do something like the following legendary patch [1]:

http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch

i.e., explicitly change the structure/logic of the function to make it
obvious enough to gcc that the variable will not be used uninitialized.


Satyam

[1] That was a funny case -- the alpha linux maintainer is also a gcc
    maintainer. Alpha even sets -Werror, so either he had to fix the
    kernel code that produced the warning, or go fix GCC to not warn
    about it -- he chose the former :-)

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Markus Gothe]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

GCC 4.1.2 has been stable for a long time now, maybe you better
upgrade your binutils instead...

//Markus

Satyam Sharma wrote:
> Hi Maciej,
>
>
> On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
>> On Wed, 19 Sep 2007, Andrew Morton wrote:
>>
>>>> patch-mips-2.6.23-rc5-20070904-pmag-ba-err-2 diff -up
>>>> --recursive --new-file
>>>> linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c
>>>> linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c ---
>>>> linux-mips-2.6.23-rc5-20070904.macro/drivers/video/pmag-ba-fb.c
>>>> 2007-02-21 05:56:47.000000000 +0000 +++
>>>> linux-mips-2.6.23-rc5-20070904/drivers/video/pmag-ba-fb.c
>>>> 2007-09-18 10:56:51.000000000 +0000 @@ -147,16 +147,23 @@
>>>> static int __init pmagbafb_probe(struct resource_size_t
>>>> start, len; struct fb_info *info; struct pmagbafb_par *par; +
>>>> int err = 0;
>>> This initialisation to zero is not good.
>>>
>>> Because if some error-path code forgot to do `err = -EFOO' then
>>> probe() will return zero and the driver will leave things in
>>> half-initialised state and will then proceed as if things had
>>> succeeded.  It will crash.
>> GCC used to complain: "`foo' might be used uninitialized..." and
>> this is the usual cure; let me see if this not the case anymore
>> (I have 4.1.2).
>
> Even so, initializing to zero isn't quite good. You could use the
> uninitialized_var() (once you've confirmed that the warning is
> bogus). However, some maintainers may still nack
> uninitialized_var() usage, quite legitimately.
>
>
>>> So it's better to leave this local uninitialised, because we
>>> really want to get that compiler warning if someone forgot to
>>> set the return value.
>> Yes of course, barring the issue mentioned.  Note the message
>> above is not the same as: "`foo' is used uninitialized..." that
>> would be reported in the case which you are concerned of.
>
> Firstly, "may be used uninitialized" can still be a bug.
>
> Secondly, latest gcc is *horribly* buggy (and has been so for last
> several releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501
>
> We'd been hurling all sorts of abuses on gcc for quite long (when
> it fails to detect these "false positive" cases), but now, it turns
> out it is quite easy to write *genuinely* buggy code that still
> won't get any warnings, neither the "is used" nor "may be used"
> one!
>
> In short, there are three ways to fix these false positive
> warnings:
>
> 1. Do nothing, there are enough "uninitialized variable" warnings
> anyway, and hopefully, one day GCC would clean up its act.
>
> 2. Use uninitialized_var() to shut it up (only if it's genuinely
> bogus).
>
> 3. Do something like the following legendary patch [1]:
>
> http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch
>
>
> i.e., explicitly change the structure/logic of the function to make
> it obvious enough to gcc that the variable will not be used
> uninitialized.
>
>
> Satyam
>
> [1] That was a funny case -- the alpha linux maintainer is also a
> gcc maintainer. Alpha even sets -Werror, so either he had to fix
> the kernel code that produced the warning, or go fix GCC to not
> warn about it -- he chose the former :-)
>


- --
_______________________________________

Mr Markus Gothe
Software Engineer

Phone: +46 (0)13 21 81 20 (ext. 1046)
Fax: +46 (0)13 21 21 15
Mobile: +46 (0)73 718 72 80
Diskettgatan 11, SE-583 35 Linköping, Sweden
www.27m.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFG8nuh6I0XmJx2NrwRCIX3AJ9c1HwLMWXV6SFb/WmJ2zFR0xbJxgCeLrIb
g7N9BsOQhRre5iDf6hFcXF0=
=VXuc
-----END PGP SIGNATURE-----

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Maciej W. Rozycki]

Hi Satyam,

> Firstly, "may be used uninitialized" can still be a bug.

 Of course -- essentially GCC cannot really figure out whether all the 
possible paths of execution include initialisation or not and complains 
just in case.

> Secondly, latest gcc is *horribly* buggy (and has been so for last several
> releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501
> 
> We'd been hurling all sorts of abuses on gcc for quite long (when it fails
> to detect these "false positive" cases), but now, it turns out it is quite
> easy to write *genuinely* buggy code that still won't get any warnings,
> neither the "is used" nor "may be used" one!

 GCC for MIPS used to be problematic enough elsewhere I do not want to 
turn back.  Even 4.0.x generates bad code, e.g. fs/partitions/msdos.c gets 
miscompiled for the big endianness (but not for the little one!).  
Compared to that some useless warnings are negligible.  This 4.1.2 version 
has triggered no problems with the kernel yet (though I suspect it is 
still so-so -- e.g. gmp gets miscompiled; which used to be fine with 
4.0.x, oddly enough).

> In short, there are three ways to fix these false positive warnings:
> 
> 1. Do nothing, there are enough "uninitialized variable" warnings anyway,
>    and hopefully, one day GCC would clean up its act.
> 
> 2. Use uninitialized_var() to shut it up (only if it's genuinely bogus).
> 
> 3. Do something like the following legendary patch [1]:
> 
> http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch
> 
> i.e., explicitly change the structure/logic of the function to make it
> obvious enough to gcc that the variable will not be used uninitialized.

 Perhaps preinitialising to an error value such as -EINVAL would be of 
more sense.  This way any error paths lacking initialisation are still 
reported as errors, even though the classification might be wrong.  In 
fact more exotic one might be chosen (the glibc manual has some nice 
proposals if none of these we currently define fits) so the mistake is 
more obvious.

  Maciej

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Maciej W. Rozycki]

Hi Satyam,

> Firstly, "may be used uninitialized" can still be a bug.

 Of course -- essentially GCC cannot really figure out whether all the 
possible paths of execution include initialisation or not and complains 
just in case.

> Secondly, latest gcc is *horribly* buggy (and has been so for last several
> releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501
> 
> We'd been hurling all sorts of abuses on gcc for quite long (when it fails
> to detect these "false positive" cases), but now, it turns out it is quite
> easy to write *genuinely* buggy code that still won't get any warnings,
> neither the "is used" nor "may be used" one!

 GCC for MIPS used to be problematic enough elsewhere I do not want to 
turn back.  Even 4.0.x generates bad code, e.g. fs/partitions/msdos.c gets 
miscompiled for the big endianness (but not for the little one!).  
Compared to that some useless warnings are negligible.  This 4.1.2 version 
has triggered no problems with the kernel yet (though I suspect it is 
still so-so -- e.g. gmp gets miscompiled; which used to be fine with 
4.0.x, oddly enough).

> In short, there are three ways to fix these false positive warnings:
> 
> 1. Do nothing, there are enough "uninitialized variable" warnings anyway,
>    and hopefully, one day GCC would clean up its act.
> 
> 2. Use uninitialized_var() to shut it up (only if it's genuinely bogus).
> 
> 3. Do something like the following legendary patch [1]:
> 
> http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch
> 
> i.e., explicitly change the structure/logic of the function to make it
> obvious enough to gcc that the variable will not be used uninitialized.

 Perhaps preinitialising to an error value such as -EINVAL would be of 
more sense.  This way any error paths lacking initialisation are still 
reported as errors, even though the classification might be wrong.  In 
fact more exotic one might be chosen (the glibc manual has some nice 
proposals if none of these we currently define fits) so the mistake is 
more obvious.

  Maciej

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

On Thu, 20 Sep 2007, Markus Gothe wrote:
> 
> GCC 4.1.2 has been stable for a long time now, maybe you better
> upgrade your binutils instead...

I'd been using 4.2.1 -- I don't want to downgrade to 4.1.2. (btw from
the discussion on gcc's bugzilla it appears the bug wasn't resolved
in 4.1.2 either?)

Satyam

> Satyam Sharma wrote:
> > Hi Maciej,
> >
> >
> > On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
> > > 
> > > On Wed, 19 Sep 2007, Andrew Morton wrote:
> > > > 
> > > > This initialisation to zero is not good.
> > > > 
> > > > Because if some error-path code forgot to do `err = -EFOO' then
> > > > probe() will return zero and the driver will leave things in
> > > > half-initialised state and will then proceed as if things had
> > > > succeeded.  It will crash.
> > > 
> > > GCC used to complain: "`foo' might be used uninitialized..." and
> > > this is the usual cure; let me see if this not the case anymore
> > > (I have 4.1.2).
> >
> > Even so, initializing to zero isn't quite good. You could use the
> > uninitialized_var() (once you've confirmed that the warning is
> > bogus). However, some maintainers may still nack
> > uninitialized_var() usage, quite legitimately.
> >
> >
> > > > So it's better to leave this local uninitialised, because we
> > > > really want to get that compiler warning if someone forgot to
> > > > set the return value.
> > > Yes of course, barring the issue mentioned.  Note the message
> > > above is not the same as: "`foo' is used uninitialized..." that
> > > would be reported in the case which you are concerned of.
> >
> > Firstly, "may be used uninitialized" can still be a bug.
> >
> > Secondly, latest gcc is *horribly* buggy (and has been so for last
> > several releases including 4.1, 4.2 and 4.3 -- 3.x was good). See:
> >
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33327
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=18501
> >
> > We'd been hurling all sorts of abuses on gcc for quite long (when
> > it fails to detect these "false positive" cases), but now, it turns
> > out it is quite easy to write *genuinely* buggy code that still
> > won't get any warnings, neither the "is used" nor "may be used"
> > one!
> >
> > In short, there are three ways to fix these false positive
> > warnings:
> >
> > 1. Do nothing, there are enough "uninitialized variable" warnings
> > anyway, and hopefully, one day GCC would clean up its act.
> >
> > 2. Use uninitialized_var() to shut it up (only if it's genuinely
> > bogus).
> >
> > 3. Do something like the following legendary patch [1]:
> >
> > http://kegel.com/crosstool/crosstool-0.43/patches/linux-2.6.11.3/arch_alpha_kernel_srcons.patch
> >
> >
> > i.e., explicitly change the structure/logic of the function to make
> > it obvious enough to gcc that the variable will not be used
> > uninitialized.
> >
> >
> > Satyam
> >
> > [1] That was a funny case -- the alpha linux maintainer is also a
> > gcc maintainer. Alpha even sets -Werror, so either he had to fix
> > the kernel code that produced the warning, or go fix GCC to not
> > warn about it -- he chose the former :-)

Re: [PATCH] drivers/video/pmag-ba-fb.c: Improve diagnostics

[Satyam Sharma]

On Thu, 20 Sep 2007, Maciej W. Rozycki wrote:
> 
>  Perhaps preinitialising to an error value such as -EINVAL would be of
> more sense.  This way any error paths lacking initialisation are still
> reported as errors, even though the classification might be wrong.

Eeee ... at least I wouldn't prefer that. Why not simply use the
"int x = x;" trick (which is what uninitialized_var() does) -- it shuts
up the warning, and does *nothing* else. The bug will not be hidden, if
there's bad misbehaviour happening due to the bug, it will continue to
happen that way -- thus bringing our attention to it. Pre-initializing
to -EINVAL (or whatever) has the problem that when the bug actually
triggers, something unrelated might happen higher up the callchain, and
we'd be scratching our heads in a "why are we getting a -EINVAL here?"
kind of way ... worse still, we might think that this was _really_ an
EINVAL and go about debugging it ...

Plus, pre-initializing to -EINVAL (or even 0) will waste some bytes in
kernel text size, but no such overhead with uninitialized_var() :-)


Satyam