| Message ID | 1239173283.10104.38.camel@localhost (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, 8 Apr 2009, Michael Ellerman wrote: > On Wed, 2009-04-08 at 15:51 +1000, Tony Breeds wrote: > > On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote: > > > > > The getter routines in here could really multiplex their return values > > > with a negative error code, which I generally prefer, but this works I > > > guess. > > > > I was hoping someone would notice and suggest it. tag you're it! > > I meant we /could/ change them, but we could also leave them, it's a bit > of a coin-flip which is better. Nathan might have an opinion? > > Something like this: > > diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c > index bb37b1d..9f3a155 100644 > --- a/arch/powerpc/kernel/cacheinfo.c > +++ b/arch/powerpc/kernel/cacheinfo.c > -static int cache_size_kb(const struct cache *cache, unsigned int *ret) > +static int cache_size_kb(const struct cache *cache) > { > unsigned int size; ^^^^^^^^^^^^ `int size', or Roel will come to get you ;-) > > - if (cache_size(cache, &size)) > - return -ENODEV; > + size = cache_size(cache); > + if (size < 0) ^^^^^^^^ > + return size; I can't check the others, due to lacking context... With kind regards, Geert Uytterhoeven Software Architect Sony Techsoft Centre Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone: +32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: Geert.Uytterhoeven@sonycom.com Internet: http://www.sony-europe.com/ A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010
Michael Ellerman <michael@ellerman.id.au> wrote: > On Wed, 2009-04-08 at 15:51 +1000, Tony Breeds wrote: > > On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote: > > > > > The getter routines in here could really multiplex their return values > > > with a negative error code, which I generally prefer, but this works I > > > guess. > > > > I was hoping someone would notice and suggest it. tag you're it! > > I meant we /could/ change them, but we could also leave them, it's a bit > of a coin-flip which is better. Nathan might have an opinion? I think I had some reason for doing it this way, but I'm drawing a blank right now. In the meantime, can someone post the warnings that gcc emits for cacheinfo.c, as well as the gcc version? I can't reproduce them with 4.3.2 on Fedora.
On Wed, Apr 08, 2009 at 01:47:36PM -0500, Nathan Lynch wrote: > I think I had some reason for doing it this way, but I'm drawing a > blank right now. > > In the meantime, can someone post the warnings that gcc emits for > cacheinfo.c, as well as the gcc version? I can't reproduce them with > 4.3.2 on Fedora. --- [tony@thor ~]$ egrep cacheinfo tmp/build.log /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'associativity_show': /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562: warning: 'associativity' may be used uninitialized in this function /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'size_show': /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513: warning: 'size_kb' may be used uninitialized in this function tony@Sprygo:~/scratch$ /opt/crosstool/gcc-4.4.0-20090216-nolibc/powerpc-linux/bin/powerpc-linux-gcc -v Using built-in specs. Target: powerpc-linux Configured with: /home/tony/buildall/src/gcc/configure --target=powerpc-linux --enable-targets=all --prefix=/opt/crosstool/gcc-4.4.0-20090216-nolibc/powerpc-linux --program-prefix=powerpc-linux- --disable-bootstrap --with-mpfr=/usr --enable-languages=c --without-headers --enable-sjlj-exceptions --with-system-libunwind --disable-nls --disable-threads --disable-shared --disable-libmudflap --disable-libssp --disable-libgomp --disable-decimal-float --enable-checking=release Thread model: single gcc version 4.4.0 20090216 (experimental) (GCC) tony@Sprygo:~/scratch$ --- That's the compiler in rawhide Yours Tony
Tony Breeds <tony@bakeyournoodle.com> wrote: > On Wed, Apr 08, 2009 at 01:47:36PM -0500, Nathan Lynch wrote: > > > I think I had some reason for doing it this way, but I'm drawing a > > blank right now. > > > > In the meantime, can someone post the warnings that gcc emits for > > cacheinfo.c, as well as the gcc version? I can't reproduce them with > > 4.3.2 on Fedora. > > --- > [tony@thor ~]$ egrep cacheinfo tmp/build.log > /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'associativity_show': > /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562: warning: 'associativity' may be used uninitialized in this function > /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'size_show': > /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513: warning: 'size_kb' may be used uninitialized in this function Thanks. So I think I've convinced myself that the warnings are incorrect and that uninitialized use is not possible. But I find it odd that gcc gives warnings for these sites but not others in the file that use the same idiom (e.g. line_size_show, nr_sets_show). I'd guess that inlining is implicated somehow. Would I be justified in worrying that this version of gcc is generating incorrect code? If not, then I'm fine with the uninitialized_var() changes, but do please include the warnings and the compiler version in the changelog.
>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function >> 'associativity_show': >> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562: >> warning: 'associativity' may be used uninitialized in this function >> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function >> 'size_show': >> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513: >> warning: 'size_kb' may be used uninitialized in this function > > Thanks. > > So I think I've convinced myself that the warnings are incorrect and > that uninitialized use is not possible. Strictly speaking the warnings aren't incorrect: the variables "may be used uninitialized", they just never are. I agree this isn't very helpful. > But I find it odd that gcc gives warnings for these sites but not > others > in the file that use the same idiom (e.g. line_size_show, > nr_sets_show). Yeah. > I'd guess that inlining is implicated somehow. Would I > be justified in worrying that this version of gcc is generating > incorrect code? Not really. Sub-optimal code perhaps, but not incorrect. > If not, then I'm fine with the uninitialized_var() changes, but do > please include the warnings and the compiler version in the changelog. If this happens with a non-ancient GCC version, can we have a bugreport please? Segher
diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c index bb37b1d..9f3a155 100644 --- a/arch/powerpc/kernel/cacheinfo.c +++ b/arch/powerpc/kernel/cacheinfo.c @@ -191,7 +191,7 @@ static void cache_cpu_set(struct cache *cache, int cpu) } } -static int cache_size(const struct cache *cache, unsigned int *ret) +static int cache_size(const struct cache *cache) { const char *propname; const u32 *cache_size; @@ -202,19 +202,18 @@ static int cache_size(const struct cache *cache, unsigned if (!cache_size) return -ENODEV; - *ret = *cache_size; - return 0; + return cache_size; } -static int cache_size_kb(const struct cache *cache, unsigned int *ret) +static int cache_size_kb(const struct cache *cache) { unsigned int size; - if (cache_size(cache, &size)) - return -ENODEV; + size = cache_size(cache); + if (size < 0) + return size; - *ret = size / 1024; - return 0; + return size / 1024; } /* not cache_line_size() because that's a macro in include/linux/cache.h */ @@ -515,8 +514,9 @@ static ssize_t size_show(struct kobject *k, struct kobj_attr cache = index_kobj_to_cache(k); - if (cache_size_kb(cache, &size_kb)) - return -ENODEV; + size_kb = cache_size_kb(cache); + if (size_kb < 0) + return size_kb; return sprintf(buf, "%uK\n", size_kb); }