[PATCH] btrfs: fix __ucmpdi2 compile bug on 32 bit builds

[PATCH] btrfs: fix __ucmpdi2 compile bug on 32 bit builds

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

We get this on 32 builds:

fs/built-in.o: In function `extent_fiemap':
(.text+0x1019f2): undefined reference to `__ucmpdi2'

Happens because of a switch statement with a 64 bit argument.
Convert this to an if statement to fix this.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 fs/btrfs/extent_io.c |   16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Index: linux-2.6/fs/btrfs/extent_io.c
===================================================================
--- linux-2.6.orig/fs/btrfs/extent_io.c
+++ linux-2.6/fs/btrfs/extent_io.c
@@ -2884,25 +2884,19 @@ int extent_fiemap(struct inode *inode, s
 		disko = 0;
 		flags = 0;
 
-		switch (em->block_start) {
-		case EXTENT_MAP_LAST_BYTE:
+		if (em->block_start == EXTENT_MAP_LAST_BYTE) {
 			end = 1;
 			flags |= FIEMAP_EXTENT_LAST;
-			break;
-		case EXTENT_MAP_HOLE:
+		} else if (em->block_start == EXTENT_MAP_HOLE) {
 			flags |= FIEMAP_EXTENT_UNWRITTEN;
-			break;
-		case EXTENT_MAP_INLINE:
+		} else if (em->block_start == EXTENT_MAP_INLINE) {
 			flags |= (FIEMAP_EXTENT_DATA_INLINE |
 				  FIEMAP_EXTENT_NOT_ALIGNED);
-			break;
-		case EXTENT_MAP_DELALLOC:
+		} else if (em->block_start == EXTENT_MAP_DELALLOC) {
 			flags |= (FIEMAP_EXTENT_DELALLOC |
 				  FIEMAP_EXTENT_UNKNOWN);
-			break;
-		default:
+		} else {
 			disko = em->block_start;
-			break;
 		}
 		if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags))
 			flags |= FIEMAP_EXTENT_ENCODED;

Re: [PATCH] btrfs: fix __ucmpdi2 compile bug on 32 bit builds

[Stephen Rothwell]

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

Hi Heiko,

On Fri, 27 Mar 2009 13:42:52 +0100 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
>
\> @@ -2884,25 +2884,19 @@ int extent_fiemap(struct inode *inode, s
>  		disko = 0;
>  		flags = 0;
>  
> -		switch (em->block_start) {
> -		case EXTENT_MAP_LAST_BYTE:
> +		if (em->block_start == EXTENT_MAP_LAST_BYTE) {

I might be a good idea to put a comment there about why this isn't a
switch statement so that someone doesn't "clean up" this code in the
future.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

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

Re: [PATCH] btrfs: fix __ucmpdi2 compile bug on 32 bit builds

[Kyle McMartin]

On Sat, Mar 28, 2009 at 01:16:26AM +1100, Stephen Rothwell wrote:
> > -		switch (em->block_start) {
> > -		case EXTENT_MAP_LAST_BYTE:
> > +		if (em->block_start == EXTENT_MAP_LAST_BYTE) {
> 
> I might be a good idea to put a comment there about why this isn't a
> switch statement so that someone doesn't "clean up" this code in the
> future.
> 

nrrrgh. Remind me why we can't just add the libgcc helpers?

Unless we plan on growing some more of these, surely we can just reduce
it to a u32 for the switch...

regards, Kyle

Re: [PATCH] btrfs: fix __ucmpdi2 compile bug on 32 bit builds

[Chris Mason]

On Fri, 2009-03-27 at 10:24 -0400, Kyle McMartin wrote:
> On Sat, Mar 28, 2009 at 01:16:26AM +1100, Stephen Rothwell wrote:
> > > -		switch (em->block_start) {
> > > -		case EXTENT_MAP_LAST_BYTE:
> > > +		if (em->block_start == EXTENT_MAP_LAST_BYTE) {
> > 
> > I might be a good idea to put a comment there about why this isn't a
> > switch statement so that someone doesn't "clean up" this code in the
> > future.
> > 
> 
> nrrrgh. Remind me why we can't just add the libgcc helpers?
>
> Unless we plan on growing some more of these, surely we can just reduce
> it to a u32 for the switch...

EXTENT_MAP_LAST_BYTE is something like (u64)-5

This if/else setup is fine, I'll take the patch.

-chris

Re: [PATCH] btrfs: fix __ucmpdi2 compile bug on 32 bit builds

[Andi Kleen]

Heiko Carstens <heiko.carstens@de.ibm.com> writes:

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>
> We get this on 32 builds:
>
> fs/built-in.o: In function `extent_fiemap':
> (.text+0x1019f2): undefined reference to `__ucmpdi2'

>
> Happens because of a switch statement with a 64 bit argument.
> Convert this to an if statement to fix this.


To be honest that sounds more like a bug in your architecture.

I don't think it's the right solution to make a new rule
"you shall not do 64bit switch()", because that's a reasonable
thing to do and will be hard to enforce over millions of lines
of random Linux code.

There was a explicit decision to not support implicit 64bit 
divides on 32bit because they're very costly, but that doesn't
really apply to 64bit switch(). At least they shouldn't be very costly
in theory. It seems indeed weird to call a function to compare
a 64bit value. I bet the call sequence is larger than just 
doing two cmps. Perhaps your gcc should be fixed? Or alternatively
at least that function be added to the kernel runtime library.

-Andi


ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] btrfs: fix __ucmpdi2 compile bug on 32 bit builds

[Kyle McMartin]

On Sat, Mar 28, 2009 at 10:38:50AM +0100, Andi Kleen wrote:
> To be honest that sounds more like a bug in your architecture.
> 
> I don't think it's the right solution to make a new rule
> "you shall not do 64bit switch()", because that's a reasonable
> thing to do and will be hard to enforce over millions of lines
> of random Linux code.
> 
> There was a explicit decision to not support implicit 64bit 
> divides on 32bit because they're very costly, but that doesn't
> really apply to 64bit switch(). At least they shouldn't be very costly
> in theory. It seems indeed weird to call a function to compare
> a 64bit value. I bet the call sequence is larger than just 
> doing two cmps. Perhaps your gcc should be fixed? Or alternatively
> at least that function be added to the kernel runtime library.
> 

i386 will do this kind of stupidity too if you let it. I had to fix
this in nouveau a few weeks back because they were doing a u64 modulus
(a power of 2 too, no idea why gcc was so clueless as to not properly
 reduce it...)

GCC is getting much worse in this regard...

regards, Kyle