Re: [PATCH 03/10 v3] ext4: add physical block and status member into extent status tree

Re: [PATCH 03/10 v3] ext4: add physical block and status member into extent status tree

[Theodore Ts'o]

On Wed, Jan 30, 2013 at 10:43:33AM +0800, Zheng Liu wrote:
> 
> Clang is first coming in my mind.  I know that some one try to use it
> to build a linux kernel and get a lot of problems that are about gcc
> extension.  But for us it seems that things are not too bad. ;)

Clang accepts bitfields with "unsigned long long", but I've discovered
something which does _not_ support unsigned long long --- the "sparse"
tool.  :-(

I discovered this when running "make C=1", i.e.:

  rm -f fs/ext4/extents_status.o
  make C=1 fs/ext4/extents_status.o

Here's a simple test case which demo's that sparse doesn't deal well
with unsigned long long.  If we change the last two fields in struct
extents_status to:

	unsigned long es_pblk : 30;	/* first physical block */
	unsigned long es_status : 2;	/* record the status of extent */

sparse doesn't complain.  But as shown below, sparse complains bitterly:

/tmp/foo.c:22:24: warning: invalid access past the end of 'es' (24 28)

I'm not sure Chris will consider this a bug, since bitfields
with "unsigned long long" isn't standards complaint, even if gcc and
clang supports it.   Chris, what do you think?

              	       		      	       - Ted
   	

#!/bin/sh
cat > /tmp/foo.c << EOF
#include <unistd.h>
#include <stdio.h>

struct rb_node {
	unsigned long  __rb_parent_color;
	struct rb_node *rb_right;
	struct rb_node *rb_left;
} __attribute__((aligned(sizeof(long))));

struct extent_status {
	struct rb_node rb_node;
	unsigned long es_lblk;		/* first logical block extent covers */
	unsigned long es_len;		/* length of extent in block */
	unsigned long long es_pblk : 62;	/* first physical block */
	unsigned long long es_status : 2;	/* record the status of extent */
};

int main(int argc, char **argv)
{
	struct extent_status es;

	es.es_status = 3;

	printf("%d\n", es.es_status);
	printf("size %u\n", sizeof(es));
}
EOF
sparse /tmp/foo.c

Re: [PATCH 03/10 v3] ext4: add physical block and status member into extent status tree

[Zheng Liu]

On Sat, Feb 02, 2013 at 10:03:43PM -0500, Theodore Ts'o wrote:
> On Wed, Jan 30, 2013 at 10:43:33AM +0800, Zheng Liu wrote:
> > 
> > Clang is first coming in my mind.  I know that some one try to use it
> > to build a linux kernel and get a lot of problems that are about gcc
> > extension.  But for us it seems that things are not too bad. ;)
> 
> Clang accepts bitfields with "unsigned long long", but I've discovered
> something which does _not_ support unsigned long long --- the "sparse"
> tool.  :-(

Yes, this problem has been reported by Fengguang.  So I am plan to use
another method to define extent_status structure as last time we
discuessed.  What do you think?

Thanks,
                                                - Zheng

> 
> I discovered this when running "make C=1", i.e.:
> 
>   rm -f fs/ext4/extents_status.o
>   make C=1 fs/ext4/extents_status.o
> 
> Here's a simple test case which demo's that sparse doesn't deal well
> with unsigned long long.  If we change the last two fields in struct
> extents_status to:
> 
> 	unsigned long es_pblk : 30;	/* first physical block */
> 	unsigned long es_status : 2;	/* record the status of extent */
> 
> sparse doesn't complain.  But as shown below, sparse complains bitterly:
> 
> /tmp/foo.c:22:24: warning: invalid access past the end of 'es' (24 28)
> 
> I'm not sure Chris will consider this a bug, since bitfields
> with "unsigned long long" isn't standards complaint, even if gcc and
> clang supports it.   Chris, what do you think?
> 
>               	       		      	       - Ted
>    	
> 
> #!/bin/sh
> cat > /tmp/foo.c << EOF
> #include <unistd.h>
> #include <stdio.h>
> 
> struct rb_node {
> 	unsigned long  __rb_parent_color;
> 	struct rb_node *rb_right;
> 	struct rb_node *rb_left;
> } __attribute__((aligned(sizeof(long))));
> 
> struct extent_status {
> 	struct rb_node rb_node;
> 	unsigned long es_lblk;		/* first logical block extent covers */
> 	unsigned long es_len;		/* length of extent in block */
> 	unsigned long long es_pblk : 62;	/* first physical block */
> 	unsigned long long es_status : 2;	/* record the status of extent */
> };
> 
> int main(int argc, char **argv)
> {
> 	struct extent_status es;
> 
> 	es.es_status = 3;
> 
> 	printf("%d\n", es.es_status);
> 	printf("size %u\n", sizeof(es));
> }
> EOF
> sparse /tmp/foo.c
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/10 v3] ext4: add physical block and status member into extent status tree

[Dan Carpenter]

On Sun, Feb 03, 2013 at 01:21:13PM +0800, Zheng Liu wrote:
> On Sat, Feb 02, 2013 at 10:03:43PM -0500, Theodore Ts'o wrote:
> > On Wed, Jan 30, 2013 at 10:43:33AM +0800, Zheng Liu wrote:
> > > 
> > > Clang is first coming in my mind.  I know that some one try to use it
> > > to build a linux kernel and get a lot of problems that are about gcc
> > > extension.  But for us it seems that things are not too bad. ;)
> > 
> > Clang accepts bitfields with "unsigned long long", but I've discovered
> > something which does _not_ support unsigned long long --- the "sparse"
> > tool.  :-(
> 
> Yes, this problem has been reported by Fengguang.  So I am plan to use
> another method to define extent_status structure as last time we
> discuessed.  What do you think?
> 

I don't get this warning on my version of Sparse.

Sparse used to assume -m32 all the time but now that's been changed.
Are you using the most recent version of Sparse?  Try passing -m64.

regards,
dan carpenter

Re: [PATCH 03/10 v3] ext4: add physical block and status member into extent status tree

[Theodore Ts'o]

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

On Sun, Feb 03, 2013 at 11:19:43AM +0300, Dan Carpenter wrote:
> 
> I don't get this warning on my version of Sparse.
> 
> Sparse used to assume -m32 all the time but now that's been changed.
> Are you using the most recent version of Sparse?  Try passing -m64.

Hmm, I got my version of sparse from

git://git.kernel.org/pub/scm/devel/sparse/sparse.git

... where the latest version is 0.4.4, dated November 21, 2011.  Is
there a different dit repository I should be using?

I see that it doesn't complain with -m64, but the test program should
be valid for x86 with 32 bits just as much as 64 bits.  Am I missing
something?

						- Ted


[-- Attachment #2: testcase --]
[-- Type: text/plain, Size: 881 bytes --]

#!/bin/sh
cat > /tmp/testcase.c << EOF
#include <unistd.h>
#include <stdio.h>

struct rb_node {
	unsigned long  __rb_parent_color;
	struct rb_node *rb_right;
	struct rb_node *rb_left;
} __attribute__((aligned(sizeof(long))));

struct extent_status {
	struct rb_node rb_node;
	unsigned long es_lblk;		/* first logical block extent covers */
	unsigned long es_len;		/* length of extent in block */
	unsigned long long es_pblk : 62;	/* first physical block */
	unsigned long long es_status : 2;	/* record the status of extent */
};

int main(int argc, char **argv)
{
	struct extent_status es;

	es.es_status = 3;

	printf("%d\n", es.es_status);
	printf("size %u\n", sizeof(es));
}
EOF
echo "sparse /tmp/testcase.c"
sparse /tmp/testcase.c
echo " "
echo "sparse -m32 /tmp/testcase.c"
sparse -m32 /tmp/testcase.c
echo " "
echo "sparse -m64 /tmp/testcase.c"
sparse -m64 /tmp/testcase.c

Re: [PATCH 03/10 v3] ext4: add physical block and status member into extent status tree

[Sam Ravnborg]

On Sat, Feb 02, 2013 at 10:03:43PM -0500, Theodore Ts'o wrote:
> On Wed, Jan 30, 2013 at 10:43:33AM +0800, Zheng Liu wrote:
> > 
> > Clang is first coming in my mind.  I know that some one try to use it
> > to build a linux kernel and get a lot of problems that are about gcc
> > extension.  But for us it seems that things are not too bad. ;)
> 
> Clang accepts bitfields with "unsigned long long", but I've discovered
> something which does _not_ support unsigned long long --- the "sparse"
> tool.  :-(
> 
> I discovered this when running "make C=1", i.e.:
> 
>   rm -f fs/ext4/extents_status.o
>   make C=1 fs/ext4/extents_status.o

Small hint...
If you use:

    make C=2 fs/ext4/extents_status.o

Then kbuild will run sparse on all targets you specify,
even if they do not need to be rebuild.
In other words - you then do not need to delete the .o file first.

This works for all the usual ways you can specify a target so to
check all of ext4 you just issue:

    make C=2 fs/ext4/

	Sam