Re: [PATCH] regmap: silence GCC warning
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
On Sat, 2012-10-06 at 09:53 +0100, Mark Brown wrote: > On Fri, Oct 05, 2012 at 06:20:44PM -0400, valdis.kletni...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
On Fri, Oct 05, 2012 at 06:20:44PM -0400, valdis.kletni...@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... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
On Fri, Oct 05, 2012 at 06:20:44PM -0400, valdis.kletni...@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... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
On Sat, 2012-10-06 at 09:53 +0100, Mark Brown wrote: On Fri, Oct 05, 2012 at 06:20:44PM -0400, valdis.kletni...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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. pgpfYqH7jx650.pgp Description: PGP signature
Re: [PATCH] regmap: silence GCC warning
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. pgpfYqH7jx650.pgp Description: PGP signature
Re: [PATCH] regmap: silence GCC warning
On Wed, Oct 03, 2012 at 09:23:36AM +0200, Paul Bolle wrote:
> On Tue, 2012-10-02 at 20:11 -0400, valdis.kletni...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
On Tue, 2012-10-02 at 20:11 -0400, valdis.kletni...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
On Tue, 2012-10-02 at 20:11 -0400, valdis.kletni...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
On Wed, Oct 03, 2012 at 09:23:36AM +0200, Paul Bolle wrote:
On Tue, 2012-10-02 at 20:11 -0400, valdis.kletni...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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...
pgpxWnEN9X2Ta.pgp
Description: PGP signature
Re: [PATCH] regmap: silence GCC warning
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...
pgpxWnEN9X2Ta.pgp
Description: PGP signature
Re: [PATCH] regmap: silence GCC warning
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regmap: silence GCC warning
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regmap: silence GCC warning
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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regmap: silence GCC warning
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 pebo...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
