Re: Bogus sparse warning?

[Anton Altaparmakov <aia21@cam.ac.uk>]

On Mon, 12 Feb 2007, Linus Torvalds wrote:
> On Mon, 12 Feb 2007, Anton Altaparmakov wrote:
> > Using latest code from
> > git://git.kernel.org/pub/scm/linux/kernel/git/josh/sparse.git
> > 
> > When I run:  make CHECKFLAGS="-D__CHECK_ENDIAN__" C=2 modules
> > 
> > I get this warning:
> > 
> >  CHECK   fs/ntfs/file.c
> > fs/ntfs/file.c:2241:5: warning: incorrect type in argument 8 (different
> > signedness)
> > fs/ntfs/file.c:2241:5:    expected int [signed] ( [signed] [usertype] get_block )( ... )
> > fs/ntfs/file.c:2241:5:    got int [signed] ( static [toplevel] *<noident> )( ... )
> 
> Ok, that does seem like a sparse bug. I _think_ that the trigger here is 
> the fact that we make "get_block_t" be the *function* type, rather than a 
> "pointer to function" type, and then we screw up somewhere when we do the 
> don't do the implicit pointer conversion, and we also thus don't move the 
> type attributes around properly (notice how "get_block" is marked as being 
> a "signed usertype", _not_ a pointer).
> 
> So we should really have converted the function type in the declaration to 
> a "pointer to function", but since this is such an unusual thing to have, 
> nobody noticed.
> 
> Does the warning go away if you make "get_block_t" explicitly a pointer to 
> a function, ie you add the "*" to the typedef:
> 
> > From <include/linux/fs.h>:
> > 
> > typedef int (get_block_t)(struct inode *inode, sector_t iblock,
> >                        struct buffer_head *bh_result, int create);
> 
> so that it looks like
> 
> 	typedef int (*get_block_t)(...)
> 
> instead?

I have Christopher Li's patch applied and with that and changing to the 
(*get_block_t) in linux/fs.h it now produces no warnings:

  CHECK   fs/ntfs/file.c
  CC [M]  fs/ntfs/file.o

> (It is perfectly proper to have a typedef that is actually of a function 
> type, so this looks like a sparse bug regardless, it looks just as if we 
> don't turn a function type into a function pointer type when we see it as 
> an argument in the declaration).
> 
> Has this been there for a long time, or was it something recent in sparse 
> that seemed to trigger it (like the recent ctype conversion changes due to 
> attribute parsing?)

I am afraid I have no idea.  Until yesterday I used to run:

make CHECKFLAGS="-Wbitwise" C=2 modules

And also I kept pulling from your sparse repository and wondering why 
there have not been any changes in so long!  Only yesterday did I spot an 
actual endianness bug in NTFS which caused me to investigate why sparse 
was not picking it up and in the process I discovered that the sparse 
repository had moved and that "-Wbitwise" no longer was the correct 
option...  I then pulled the new sparse repository and changed from 
-Wbitwise to -D__CHECK_ENDIAN__ and then got tons of warnings that did not 
use to be there before.  I managed to fix all except the two I reported on 
the mailing list yesterday (and one more that I have not looked at yet - 
in fs/ntfs/runlist.c I get 18 warnings like this:
 
fs/ntfs/runlist.c:1549:18: warning: potentially expensive pointer subtraction

I have not had a chance to investigate what those mean yet)...

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/