Re: [PATCH v2] x86/current: Provide additional information to optimise get_cpu_info()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/09/2014 17:10, Marcin Cieslak wrote:
>>> Andrew Cooper<andrew.cooper3@citrix.com>  wrote:
>> Exactly as with c/s d55c5eefe "x86: use compiler visible "add" instead of
>> inline assembly "or" in get_cpu_info()", this is achieved by providing more
>> information to the compiler.
>>
>> This causes a net drop of almost 4K of .text
>>
>> Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com>
>> CC: Jan Beulich<JBeulich@suse.com>
>>
>> ---
>> v2: Less speculation about generated code in the comment
>> ---
>>   xen/include/asm-x86/current.h |    6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
>> index 2081015..b95fd79 100644
>> --- a/xen/include/asm-x86/current.h
>> +++ b/xen/include/asm-x86/current.h
>> @@ -25,9 +25,9 @@ struct cpu_info {
>>   
>>   static inline struct cpu_info *get_cpu_info(void)
>>   {
>> -    unsigned long tos;
>> -    __asm__ ( "and %%rsp,%0" : "=r" (tos) : "0" (~(STACK_SIZE-1)) );
>> -    return (struct cpu_info *)(tos + STACK_SIZE) - 1;
>> +    register unsigned long sp asm("rsp");
>> +
>> +    return (struct cpu_info *)((sp & ~(STACK_SIZE-1)) + STACK_SIZE) - 1;
>>   }
> Hello, it seems to me that the above code fails on me with clang 3.4
> on FreeBSD-CURRENT:
>
> xen/include/asm/current.h:30:33:
> 	error: variable 'sp' is uninitialized when used here [-Werror,-Wuninitialized]

That is rather unfortunate for clang.  The stack pointer has an 
initialised and perfectly good value anywhere this function can be used.

> Reverting df0ae94fd56d5f9c64089364efecb1793442360b helps.
>
> There is a workaround suggested inhttp://llvm.org/devmtg/2014-02/slides/moller-llvmlinux.pdf:
>
> diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
> index b95fd79..e133d9d 100644
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -26,6 +26,7 @@ struct cpu_info {
>   static inline struct cpu_info *get_cpu_info(void)
>   {
>       register unsigned long sp asm("rsp");
> +    asm("" : "=r" (sp));
>   
>       return (struct cpu_info *)((sp & ~(STACK_SIZE-1)) + STACK_SIZE) - 1;
>   }
>
> That silences the warning (not sure it it works).

It functions under GCC as well, but undoes some (but not all of) the 
improvements introduced as a result of df0ae94f.

It would probably be acceptable in a suitable #ifdef, along with comment 
why this seemingly redundant statement is present.

~Andrew