From: Heiko Carstens <[email protected]>
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 <[email protected]>
---
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;
Hi Heiko,
On Fri, 27 Mar 2009 13:42:52 +0100 Heiko Carstens <[email protected]> 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 [email protected]
http://www.canb.auug.org.au/~sfr/
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
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
Heiko Carstens <[email protected]> writes:
> From: Heiko Carstens <[email protected]>
>
> 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
[email protected] -- Speaking for myself only.
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