[PATCH] regmap: silence GCC warning

[PATCH] regmap: silence GCC warning

[Paul Bolle]

Building regmap.o triggers this GCC warning:
    drivers/base/regmap/regmap.c: In function ‘regmap_raw_read’:
    drivers/base/regmap/regmap.c:1172:6: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]

It seems 'ret' should always be set when this function returns. See, the
else-branch can leave 'ret' uninitialized only if 'val_count' is zero.
But if 'val_count' is zero regmap_volatile_range() will return true.
That implies that 'ret' will be set in the if-branch. ('val_count' could
be zero if 'val_len' is, for example, zero. That would be useless input,
however.)

Anyhow, initializing 'ret' to -EINVAL silences GCC and is harmless.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
I noticed this warning while building v3.6-rc7 on current Fedora 17,
using Fedora's default config.

 drivers/base/regmap/regmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index c241ae2..025f41c 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1169,12 +1169,12 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 	size_t val_bytes = map->format.val_bytes;
 	size_t val_count = val_len / val_bytes;
 	unsigned int v;
-	int ret, i;
+	int i, ret = -EINVAL;
 
 	if (val_len % map->format.val_bytes)
-		return -EINVAL;
+		return ret;
 	if (reg % map->reg_stride)
-		return -EINVAL;
+		return ret;
 
 	map->lock(map);
 
-- 
1.7.11.4

Re: [PATCH] regmap: silence GCC warning

[Mark Brown]

On Sun, Sep 30, 2012 at 12:15:55PM +0200, Paul Bolle wrote:
> Building regmap.o triggers this GCC warning:
>     drivers/base/regmap/regmap.c: In function ‘regmap_raw_read’:
>     drivers/base/regmap/regmap.c:1172:6: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> 
> It seems 'ret' should always be set when this function returns. See, the
> else-branch can leave 'ret' uninitialized only if 'val_count' is zero.
> But if 'val_count' is zero regmap_volatile_range() will return true.
> That implies that 'ret' will be set in the if-branch. ('val_count' could
> be zero if 'val_len' is, for example, zero. That would be useless input,
> however.)
> 
> Anyhow, initializing 'ret' to -EINVAL silences GCC and is harmless.

Have you reported this bug in GCC?  Their flow analyis just seems to
keep on getting worse and worse.

Re: [PATCH] regmap: silence GCC warning

[Paul Bolle]

On Mon, 2012-10-01 at 11:03 +0100, Mark Brown wrote:
> On Sun, Sep 30, 2012 at 12:15:55PM +0200, Paul Bolle wrote:
> > Building regmap.o triggers this GCC warning:
> >     drivers/base/regmap/regmap.c: In function ‘regmap_raw_read’:
> >     drivers/base/regmap/regmap.c:1172:6: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> > 
> > It seems 'ret' should always be set when this function returns. See, the
> > else-branch can leave 'ret' uninitialized only if 'val_count' is zero.
> > But if 'val_count' is zero regmap_volatile_range() will return true.
> > That implies that 'ret' will be set in the if-branch. ('val_count' could
> > be zero if 'val_len' is, for example, zero. That would be useless input,
> > however.)
> > 
> > Anyhow, initializing 'ret' to -EINVAL silences GCC and is harmless.
> 
> Have you reported this bug in GCC?

No I haven't. Since you ask, I guess you too think 'ret' is always set
when this function returns, don't you? Ie, my analysis isn't obviously
wrong.

> Their flow analyis just seems to keep on getting worse and worse.


Paul Bolle

Re: [PATCH] regmap: silence GCC warning

[Mark Brown]

On Mon, Oct 01, 2012 at 12:16:46PM +0200, Paul Bolle wrote:
> On Mon, 2012-10-01 at 11:03 +0100, Mark Brown wrote:

> > Have you reported this bug in GCC?

> No I haven't. Since you ask, I guess you too think 'ret' is always set
> when this function returns, don't you? Ie, my analysis isn't obviously
> wrong.

I haven't actually stared hard enough at the code to figure this out
yet but given what you're changing I'd hope it's a flow analysis bug.
I'm certainly not seeing a warning here.

Re: [PATCH] regmap: silence GCC warning

[Paul Bolle]

On Mon, 2012-10-01 at 11:19 +0100, Mark Brown wrote:
> I haven't actually stared hard enough at the code to figure this out
> yet but given what you're changing I'd hope it's a flow analysis bug.
> I'm certainly not seeing a warning here.

Well, you may be using another version of gcc, or using different
defaults. I'm using gcc-4.7.2-2.fc17.x86_64 (ie, the gcc currently
shipped in Fedora 17 for x86_64), in what I think to be default
settings.

Note that it's not some local oddity. Looking at the (currently) latest
log for a Fedora build of v3.6-rc7 you'll find an identical warning [0].
Apparently that build was done with gcc-4.7.1-5.fc18.x86_64 [1].


Paul Bolle

[0] http://kojipkgs.fedoraproject.org/packages/kernel/3.6.0/0.rc7.git2.2.fc18/data/logs/x86_64/build.log
[1] http://kojipkgs.fedoraproject.org/packages/kernel/3.6.0/0.rc7.git2.2.fc18/data/logs/x86_64/root.log

Re: [PATCH] regmap: silence GCC warning

[Mark Brown]

On Mon, Oct 01, 2012 at 12:32:57PM +0200, Paul Bolle wrote:
> On Mon, 2012-10-01 at 11:19 +0100, Mark Brown wrote:

> > I haven't actually stared hard enough at the code to figure this out
> > yet but given what you're changing I'd hope it's a flow analysis bug.
> > I'm certainly not seeing a warning here.

> Well, you may be using another version of gcc, or using different
> defaults. I'm using gcc-4.7.2-2.fc17.x86_64 (ie, the gcc currently
> shipped in Fedora 17 for x86_64), in what I think to be default
> settings.

Yes, of course.  Hence my comment about their flow analysis getting
worse and worse.

> Note that it's not some local oddity. Looking at the (currently) latest
> log for a Fedora build of v3.6-rc7 you'll find an identical warning [0].
> Apparently that build was done with gcc-4.7.1-5.fc18.x86_64 [1].

I assume this is a regression they've introduced in 4.7.

Re: [PATCH] regmap: silence GCC warning

[Paul Bolle]

On Mon, 2012-10-01 at 12:39 +0100, Mark Brown wrote:
> I assume this is a regression they've introduced in 4.7.

For what it's worth, GCC 4.6 apparently shows identical behavior.

The code that triggers this warning was (basically) introduced in commit
b8fb5ab156055b745254609f4635fcfd6b7dabc8 ("regmap: Support raw reads
from cached registers"). The first release shipping that commit was
v3.4. The log of a build of v3.4 using the oldest version of GCC (that
Fedora used to build v3.4) I could find also contains this warning [0].
They used gcc-4.6.3-2.fc16.x86_64 in that build [1].

Perhaps I'll stumble on some build logs of v3.4 for which earlier
versions of GCC were used. That might show whether or not earlier
releases of GCC also print a warning for this code.


Paul Bolle

[0] http://kojipkgs.fedoraproject.org/packages/kernel/3.4.2/1.fc16/data/logs/x86_64/build.log
[1] http://kojipkgs.fedoraproject.org/packages/kernel/3.4.2/1.fc16/data/logs/x86_64/root.log

Re: [PATCH] regmap: silence GCC warning

[Mark Brown]

On Mon, Oct 01, 2012 at 09:08:57PM +0200, Paul Bolle wrote:

> from cached registers"). The first release shipping that commit was
> v3.4. The log of a build of v3.4 using the oldest version of GCC (that
> Fedora used to build v3.4) I could find also contains this warning [0].
> They used gcc-4.6.3-2.fc16.x86_64 in that build [1].

> Perhaps I'll stumble on some build logs of v3.4 for which earlier
> versions of GCC were used. That might show whether or not earlier
> releases of GCC also print a warning for this code.

I'm using GCC 4.4.1 on ARM which seems quite happy.

Re: [PATCH] regmap: silence GCC warning

[Valdis.Kletnieks]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 1274 bytes --]

On Mon, 01 Oct 2012 11:03:21 +0100, Mark Brown said:
> On Sun, Sep 30, 2012 at 12:15:55PM +0200, Paul Bolle wrote:
> > Building regmap.o triggers this GCC warning:
> >     drivers/base/regmap/regmap.c: In function ‘regmap_raw_read’:
> >     drivers/base/regmap/regmap.c:1172:6: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
> >
> > It seems 'ret' should always be set when this function returns. See, the
> > else-branch can leave 'ret' uninitialized only if 'val_count' is zero.
> > But if 'val_count' is zero regmap_volatile_range() will return true.

I've not dug into it that deeply - is there a way that gcc is able to intuit
this fact and use it for flow analysis?  If not, it's not going to be able to
include that information in its analysis.

> > That implies that 'ret' will be set in the if-branch. ('val_count' could
> > be zero if 'val_len' is, for example, zero. That would be useless input,
> > however.)

But gcc doesn't know what "useless input" means, semantically.

> > Anyhow, initializing 'ret' to -EINVAL silences GCC and is harmless.
>
> Have you reported this bug in GCC?  Their flow analyis just seems to
> keep on getting worse and worse.

I'm not convinced that it's at fault in this particular case...

[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

Re: [PATCH] regmap: silence GCC warning

[Paul Bolle]

On Tue, 2012-10-02 at 20:11 -0400, Valdis.Kletnieks@vt.edu wrote:
> On Mon, 01 Oct 2012 11:03:21 +0100, Mark Brown said:
> > On Sun, Sep 30, 2012 at 12:15:55PM +0200, Paul Bolle wrote:
> > > Building regmap.o triggers this GCC warning:
> > >     drivers/base/regmap/regmap.c: In function regmap_raw_read:
> > >     drivers/base/regmap/regmap.c:1172:6: warning: ret may be used uninitialized in this function [-Wmaybe-uninitialized]
> > >
> > > It seems 'ret' should always be set when this function returns. See, the
> > > else-branch can leave 'ret' uninitialized only if 'val_count' is zero.
> > > But if 'val_count' is zero regmap_volatile_range() will return true.
> 
> I've not dug into it that deeply - is there a way that gcc is able to intuit
> this fact and use it for flow analysis?  If not, it's not going to be able to
> include that information in its analysis.

I know little about GCC, and even less about the analyses it uses. I
did, however, disassemble regmap_raw_read() with GDB. And the output
this generated does suggest that GCC uses one register to track 'ret'
and that this register is set to -EINVAL quite early. Ie, gdb's
disassembler's output suggests that 'ret' will not be used
uninitialized. (This I need to recheck, though.) But, even assuming I
understood the disassembled code correctly, I have no idea whether we
may expect GCC to understand the code it generates enough to know that,
yes, it did actually set 'ret' itself in that code.

> > > That implies that 'ret' will be set in the if-branch. ('val_count' could
> > > be zero if 'val_len' is, for example, zero. That would be useless input,
> > > however.)
> 
> But gcc doesn't know what "useless input" means, semantically.

Correct. When this function is compiled gcc has to take into account
that 'val_len' will be called with useless input, like zero.

By the way, GCC doesn't warn if I add an early check whether 'val_count'
is non-zero:

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index c241ae2..d41527b 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1171,6 +1171,8 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
        unsigned int v;
        int ret, i;
 
+       if (!val_count)
+               return -EINVAL;
        if (val_len % map->format.val_bytes)
                return -EINVAL;
        if (reg % map->reg_stride)

That is another way to silence GCC here.


Paul Bolle

Re: [PATCH] regmap: silence GCC warning

[Mark Brown]

On Wed, Oct 03, 2012 at 09:23:36AM +0200, Paul Bolle wrote:
> On Tue, 2012-10-02 at 20:11 -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Mon, 01 Oct 2012 11:03:21 +0100, Mark Brown said:

> > > > That implies that 'ret' will be set in the if-branch. ('val_count' could
> > > > be zero if 'val_len' is, for example, zero. That would be useless input,
> > > > however.)

> > But gcc doesn't know what "useless input" means, semantically.

> Correct. When this function is compiled gcc has to take into account
> that 'val_len' will be called with useless input, like zero.

> By the way, GCC doesn't warn if I add an early check whether 'val_count'
> is non-zero:

That's a much more useful fix, bodging things by just forcing things to
be assigned (especially with the way you were converting the immediate
returns) is generally terrible - it's just shutting up the errors
without actually fixing any issues that really exist and means that if
the compiler ever notices actual issues we won't see them.

Re: [PATCH] regmap: silence GCC warning

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On Wed, 03 Oct 2012 09:23:36 +0200, Paul Bolle said:

> By the way, GCC doesn't warn if I add an early check whether 'val_count'
> is non-zero:
>
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index c241ae2..d41527b 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1171,6 +1171,8 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
>         unsigned int v;
>         int ret, i;
>
> +       if (!val_count)
> +               return -EINVAL;

> That is another way to silence GCC here.

That's probably a preferable approach - that way, if a bogus val_count gets
passed in, the caller will be informed of the fact.  Which is a lot better than
just papering over the warning.

[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]

Re: [PATCH] regmap: silence GCC warning

[Mark Brown]

On Fri, Oct 05, 2012 at 06:20:44PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Wed, 03 Oct 2012 09:23:36 +0200, Paul Bolle said:

> > That is another way to silence GCC here.

> That's probably a preferable approach - that way, if a bogus val_count gets
> passed in, the caller will be informed of the fact.  Which is a lot better than
> just papering over the warning.

As I hinted earlier if someone were to send me a patch...

Re: [PATCH] regmap: silence GCC warning

[Paul Bolle]

On Sat, 2012-10-06 at 09:53 +0100, Mark Brown wrote:
> On Fri, Oct 05, 2012 at 06:20:44PM -0400, Valdis.Kletnieks@vt.edu wrote:
> > On Wed, 03 Oct 2012 09:23:36 +0200, Paul Bolle said:
> > > That is another way to silence GCC here.
> 
> > That's probably a preferable approach - that way, if a bogus val_count gets
> > passed in, the caller will be informed of the fact.  Which is a lot better than
> > just papering over the warning.
> 
> As I hinted earlier if someone were to send me a patch...

0) I was hoping to do that. But in the mean time I also filed a bug
report for GCC (at Fedora's bugzilla) [0].

1) In that report (after actually closing it) Jakub Jelinek pointed at
the type mismatch between regmap_volatile_range()'s 'num' (unsigned int)
and its callers (both use size_t, both through 'val_count'). And,
indeed, changing 'num' to size_t also makes this warning go away.

This might explain why you didn't see a warning on 32 bit arm (if that
is what you running while looking at my patch).

2) I hope to send in a second path shortly, changing 'num' to size_t. My
main doubt is whether its problematic that the loop index in
regmap_volatile_range() uses unsigned int too. If 'num' would exceed
UINT_MAX, that loop would never finish, wouldn't it? But is that
actually possible? Are there machines with that many registers?


Paul Bolle

0) https://bugzilla.redhat.com/show_bug.cgi?id=862620

Re: [PATCH] regmap: silence GCC warning

[Mark Brown]

On Sat, Oct 06, 2012 at 11:57:36AM +0200, Paul Bolle wrote:

> 2) I hope to send in a second path shortly, changing 'num' to size_t. My
> main doubt is whether its problematic that the loop index in
> regmap_volatile_range() uses unsigned int too. If 'num' would exceed
> UINT_MAX, that loop would never finish, wouldn't it? But is that
> actually possible? Are there machines with that many registers?

It's possible, of course - people can give whatever random numbers they
like to registers.  It's not particularly likely, though, so probably as
well not to worry about it until it's actually a problem.

[PATCH v2] regmap: silence GCC warning

[Paul Bolle]

Building regmap.o triggers this GCC warning:
    drivers/base/regmap/regmap.c: In function ‘regmap_raw_read’:
    drivers/base/regmap/regmap.c:1172:6: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]

Long story short: Jakub Jelinek pointed out that there is a type
mismatch between 'num' in regmap_volatile_range() and 'val_count' in
regmap_raw_read(). And indeed, converting 'num' to the type of
'val_count' (ie, size_t) makes this warning go away.

Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
---
 drivers/base/regmap/regmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index c241ae2..b99d5d3 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -82,7 +82,7 @@ bool regmap_precious(struct regmap *map, unsigned int reg)
 }
 
 static bool regmap_volatile_range(struct regmap *map, unsigned int reg,
-	unsigned int num)
+	size_t num)
 {
 	unsigned int i;
 
-- 
1.7.11.4

Re: [PATCH v2] regmap: silence GCC warning

[Mark Brown]

On Mon, Oct 08, 2012 at 10:06:30PM +0200, Paul Bolle wrote:

> Long story short: Jakub Jelinek pointed out that there is a type
> mismatch between 'num' in regmap_volatile_range() and 'val_count' in
> regmap_raw_read(). And indeed, converting 'num' to the type of
> 'val_count' (ie, size_t) makes this warning go away.

Applied, thanks.