Cleanup possibility in asm-i386/string.h

Cleanup possibility in asm-i386/string.h

[Andi Kleen]

Adrian,

If you feel the need to remove some more code: Now that gcc 2.95 isn't supported
anymore there isn't really a need to keep the handwritten inline string functions
in asm-i386/string.h around. Just declaring them as normal externs will cause
gcc to use its builtin expansions, which are typically better than these old inline
functions with inline assembly.

For out of line the C versions in lib/string.c can be used (by not setting __ARCH_*) 
x86-64 did it like this forever and I guess it would be valuable cleanup for i386 too.

-Andi

Re: Cleanup possibility in asm-i386/string.h

[Roman Zippel]

Hi,

On Tue, 7 Feb 2006, Andi Kleen wrote:

> If you feel the need to remove some more code: Now that gcc 2.95 isn't supported
> anymore there isn't really a need to keep the handwritten inline string functions
> in asm-i386/string.h around. Just declaring them as normal externs will cause
> gcc to use its builtin expansions, which are typically better than these old inline
> functions with inline assembly.
> 
> For out of line the C versions in lib/string.c can be used (by not setting __ARCH_*) 
> x86-64 did it like this forever and I guess it would be valuable cleanup for i386 too.

The only problem is that we compile with -ffreestanding which implies 
-fno-builtin, so just declaring them as normal externs is not enough and 
you have to something like this:

#define __HAVE_ARCH_MEMSET
extern void *memset(void *, int, __kernel_size_t);
#define memset(d, c, n) __builtin_memset(d, c, n)

(BTW you do this already in x86-64.)

Another problem here is because of -fno-builtin it's not easy to use the 
generic functions as fallback. x86-64 basically does this: 

#define strlen __builtin_strlen
size_t strlen(const char * s);

#ifndef __HAVE_ARCH_STRLEN
extern __kernel_size_t strlen(const char *);
#endif

This means you define a prototype for the builtin function and not for the 
normal function. I'm not sure this is really intended.

bye, Roman

Re: Cleanup possibility in asm-i386/string.h

[Andi Kleen]

On Tuesday 07 February 2006 13:00, Roman Zippel wrote:
> Hi,
> 
> On Tue, 7 Feb 2006, Andi Kleen wrote:
> 
> > If you feel the need to remove some more code: Now that gcc 2.95 isn't supported
> > anymore there isn't really a need to keep the handwritten inline string functions
> > in asm-i386/string.h around. Just declaring them as normal externs will cause
> > gcc to use its builtin expansions, which are typically better than these old inline
> > functions with inline assembly.
> > 
> > For out of line the C versions in lib/string.c can be used (by not setting __ARCH_*) 
> > x86-64 did it like this forever and I guess it would be valuable cleanup for i386 too.
> 
> The only problem is that we compile with -ffreestanding which implies 
> -fno-builtin, so just declaring them as normal externs is not enough and 
> you have to something like this:
> 
> #define __HAVE_ARCH_MEMSET
> extern void *memset(void *, int, __kernel_size_t);
> #define memset(d, c, n) __builtin_memset(d, c, n)
> 
> (BTW you do this already in x86-64.)

Yes thinking about it the x86-64 string.h could just be copied over to i386.
It should already do everything correctly and I don't think there is 
anything really 64bit specific in there.

> Another problem here is because of -fno-builtin it's not easy to use the 
> generic functions as fallback. x86-64 basically does this: 
> 
> #define strlen __builtin_strlen
> size_t strlen(const char * s);
> 
> #ifndef __HAVE_ARCH_STRLEN
> extern __kernel_size_t strlen(const char *);
> #endif
> 
> This means you define a prototype for the builtin function and not for the 
> normal function. I'm not sure this is really intended.

What good would be a prototype for a symbol that is defined to a different symbol?

-Andi

Re: Cleanup possibility in asm-i386/string.h

[Roman Zippel]

Hi,

On Tue, 7 Feb 2006, Andi Kleen wrote:

> > This means you define a prototype for the builtin function and not for the 
> > normal function. I'm not sure this is really intended.
> 
> What good would be a prototype for a symbol that is defined to a different symbol?

The point is you define a prototype for a builtin function, I'm not sure 
that's a good thing to do.
Actually I'd prefer to remove -ffreestanding again, especially because it
disables builtin functions, which we have to painfully enable all again 
one by one, instead of leaving it just to gcc.

bye, Roman

Re: Cleanup possibility in asm-i386/string.h

[Adrian Bunk]

On Tue, Feb 07, 2006 at 01:39:50PM +0100, Roman Zippel wrote:
> Hi,
> 
> On Tue, 7 Feb 2006, Andi Kleen wrote:
> 
> > > This means you define a prototype for the builtin function and not for the 
> > > normal function. I'm not sure this is really intended.
> > 
> > What good would be a prototype for a symbol that is defined to a different symbol?
> 
> The point is you define a prototype for a builtin function, I'm not sure 
> that's a good thing to do.
> Actually I'd prefer to remove -ffreestanding again, especially because it
> disables builtin functions, which we have to painfully enable all again 
> one by one, instead of leaving it just to gcc.

I remember playing with using more gcc builtins in the kernel some time 
ago, and some gcc builtin used a different library function, which was a 
function the kernel did not supply.

I don't remember the exact details, but this was the reason why I 
preferred using builtins only when explicitely enabled.

> bye, Roman

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: Cleanup possibility in asm-i386/string.h

[Andi Kleen]

On Friday 10 February 2006 01:05, Adrian Bunk wrote:
> On Tue, Feb 07, 2006 at 01:39:50PM +0100, Roman Zippel wrote:
> > Hi,
> > 
> > On Tue, 7 Feb 2006, Andi Kleen wrote:
> > 
> > > > This means you define a prototype for the builtin function and not for the 
> > > > normal function. I'm not sure this is really intended.
> > > 
> > > What good would be a prototype for a symbol that is defined to a different symbol?
> > 
> > The point is you define a prototype for a builtin function, I'm not sure 
> > that's a good thing to do.
> > Actually I'd prefer to remove -ffreestanding again, especially because it
> > disables builtin functions, which we have to painfully enable all again 
> > one by one, instead of leaving it just to gcc.
> 
> I remember playing with using more gcc builtins in the kernel some time 
> ago, and some gcc builtin used a different library function, which was a 
> function the kernel did not supply.

It works fine on x86-64. If something is missing it can be also supplied.

-Andi

Re: Cleanup possibility in asm-i386/string.h

[Roman Zippel]

Hi,

On Fri, 10 Feb 2006, Adrian Bunk wrote:

> I remember playing with using more gcc builtins in the kernel some time 
> ago, and some gcc builtin used a different library function, which was a 
> function the kernel did not supply.
> 
> I don't remember the exact details, but this was the reason why I 
> preferred using builtins only when explicitely enabled.

gcc can turn a sprintf() into strcpy(), which is a valid thing to do even 
in a kernel environment. It can be turned off selectively, if it should 
be a problem, so using -free-standing is IMO complete overkill.

bye, Roman

Re: Cleanup possibility in asm-i386/string.h

[Kyle Moffett]

On Feb 09, 2006, at 19:23, Andi Kleen wrote:
> On Friday 10 February 2006 01:05, Adrian Bunk wrote:
>> On Tue, Feb 07, 2006 at 01:39:50PM +0100, Roman Zippel wrote:
>>> Hi,
>>>
>>> On Tue, 7 Feb 2006, Andi Kleen wrote:
>>>
>>>>> This means you define a prototype for the builtin function and  
>>>>> not for the normal function. I'm not sure this is really intended.
>>>>
>>>> What good would be a prototype for a symbol that is defined to a  
>>>> different symbol?
>>>
>>> The point is you define a prototype for a builtin function, I'm  
>>> not sure that's a good thing to do. Actually I'd prefer to remove  
>>> -ffreestanding again, especially because it disables builtin  
>>> functions, which we have to painfully enable all again  one by  
>>> one, instead of leaving it just to gcc.
>>
>> I remember playing with using more gcc builtins in the kernel some  
>> time ago, and some gcc builtin used a different library function,  
>> which was a function the kernel did not supply.
>
> It works fine on x86-64. If something is missing it can be also  
> supplied.

I don't remember exactly, but I think the problem was something like  
this (even if not this exact case, it was similarly obscure):  If - 
ffreestanding was not specified, then the following code would  
generate an implicit call to memcpy() or some other library function.

struct a {
	[... large struct with lots of fields ...]
}

struct a first = { ..... };
[... more code ...];
struct a second = first; /* <== This line would generate implicit  
memcpy */

Cheers,
Kyle Moffett

--
Diplomacy involves walking softly and _carrying_ a big stick.   
Actually using the big stick means the diplomacy part failed.
   -- Rob Landley

Re: Cleanup possibility in asm-i386/string.h

[Roman Zippel]

Hi,

On Fri, 10 Feb 2006, Andi Kleen wrote:

> > I remember playing with using more gcc builtins in the kernel some time 
> > ago, and some gcc builtin used a different library function, which was a 
> > function the kernel did not supply.
> 
> It works fine on x86-64. If something is missing it can be also supplied.

I think I now see what the real problem was, x86-64 does:

#define strcpy __builtin_strcpy

which also renames the version in lib/string.c, so x86-64 never had a 
fallback copy for __builtin_sprintf.
Can we please get rid of -freestanding and fix x86-64 instead?

bye, Roman

Re: Cleanup possibility in asm-i386/string.h

[Andi Kleen]

On Friday 10 February 2006 14:02, Roman Zippel wrote:
> Hi,
> 
> On Fri, 10 Feb 2006, Andi Kleen wrote:
> 
> > > I remember playing with using more gcc builtins in the kernel some time 
> > > ago, and some gcc builtin used a different library function, which was a 
> > > function the kernel did not supply.
> > 
> > It works fine on x86-64. If something is missing it can be also supplied.
> 
> I think I now see what the real problem was, x86-64 does:
> 
> #define strcpy __builtin_strcpy
> 
> which also renames the version in lib/string.c, so x86-64 never had a 
> fallback copy for __builtin_sprintf.
> Can we please get rid of -freestanding and fix x86-64 instead?

Ok I can fix that. Just removing the defines should be ok i guess 
(afaik gcc detects them automatically as the builtin) 

I don't know if the freestanding in the main Makefile isn't needed 
for other architectures so I won't touch it right now.

-Andi

Re: Cleanup possibility in asm-i386/string.h

[Roman Zippel]

Hi,

On Fri, 10 Feb 2006, Andi Kleen wrote:

> > #define strcpy __builtin_strcpy
> > 
> > which also renames the version in lib/string.c, so x86-64 never had a 
> > fallback copy for __builtin_sprintf.
> > Can we please get rid of -freestanding and fix x86-64 instead?
> 
> Ok I can fix that. Just removing the defines should be ok i guess 
> (afaik gcc detects them automatically as the builtin) 

Not with -freestanding.

> I don't know if the freestanding in the main Makefile isn't needed 
> for other architectures so I won't touch it right now.

Well, it was added for x86-64...
It shouldn't break anything, that isn't already broken. 

bye, Roman

Re: Cleanup possibility in asm-i386/string.h

[Andi Kleen]

On Friday 10 February 2006 15:46, Roman Zippel wrote:
> Hi,
> 
> On Fri, 10 Feb 2006, Andi Kleen wrote:
> 
> > > #define strcpy __builtin_strcpy
> > > 
> > > which also renames the version in lib/string.c, so x86-64 never had a 
> > > fallback copy for __builtin_sprintf.
> > > Can we please get rid of -freestanding and fix x86-64 instead?
> > 
> > Ok I can fix that. Just removing the defines should be ok i guess 
> > (afaik gcc detects them automatically as the builtin) 
> 
> Not with -freestanding.

True.

> 
> > I don't know if the freestanding in the main Makefile isn't needed 
> > for other architectures so I won't touch it right now.
> 
> Well, it was added for x86-64...
> It shouldn't break anything, that isn't already broken. 

Ok i will remove it then.


Thanks,
-Andi