[Xen-devel,05/12] xen/arm64: bitops: Match the register size with the value size in flsl

Message ID 20190327184531.30986-6-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: Add support to build with clang
Related show

Commit Message

Julien Grall March 27, 2019, 6:45 p.m.
Clang is pickier than GCC for the register size in asm statement. It expects
the register size to match the value size.

The instruction clz is expecting the two operands to be the same size
(i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit
value, we need to make the destination variable 64-bit as well.

While at it, add a newline before the return statement.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/include/asm-arm/arm64/bitops.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Andrew Cooper March 27, 2019, 7:03 p.m. | #1
On 27/03/2019 18:45, Julien Grall wrote:
> Clang is pickier than GCC for the register size in asm statement. It expects
> the register size to match the value size.
>
> The instruction clz is expecting the two operands to be the same size
> (i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit
> value, we need to make the destination variable 64-bit as well.
>
> While at it, add a newline before the return statement.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/include/asm-arm/arm64/bitops.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/include/asm-arm/arm64/bitops.h b/xen/include/asm-arm/arm64/bitops.h
> index 6bf1922680..05045f1109 100644
> --- a/xen/include/asm-arm/arm64/bitops.h
> +++ b/xen/include/asm-arm/arm64/bitops.h
> @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long __ffs(unsigned long word)
>  
>  static inline int flsl(unsigned long x)
>  {
> -        int ret;
> +        uint64_t ret;
>  
>          if (__builtin_constant_p(x))
>                 return generic_flsl(x);
>  
>          asm("clz\t%0, %1" : "=r" (ret) : "r" (x));

As x is fixed by the ABI, ret should be unsigned long to match, surely?

This will compile as it is arm64 specific, but it looks just as wrong
(per the commit message) as using int in the first place.

~Andrew
Julien Grall March 27, 2019, 8:13 p.m. | #2
Hi,

On 27/03/2019 19:03, Andrew Cooper wrote:
> On 27/03/2019 18:45, Julien Grall wrote:

>> Clang is pickier than GCC for the register size in asm statement. It expects

>> the register size to match the value size.

>>

>> The instruction clz is expecting the two operands to be the same size

>> (i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit

>> value, we need to make the destination variable 64-bit as well.

>>

>> While at it, add a newline before the return statement.

>>

>> Signed-off-by: Julien Grall <julien.grall@arm.com>

>> ---

>>   xen/include/asm-arm/arm64/bitops.h | 3 ++-

>>   1 file changed, 2 insertions(+), 1 deletion(-)

>>

>> diff --git a/xen/include/asm-arm/arm64/bitops.h b/xen/include/asm-arm/arm64/bitops.h

>> index 6bf1922680..05045f1109 100644

>> --- a/xen/include/asm-arm/arm64/bitops.h

>> +++ b/xen/include/asm-arm/arm64/bitops.h

>> @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long __ffs(unsigned long word)

>>   

>>   static inline int flsl(unsigned long x)

>>   {

>> -        int ret;

>> +        uint64_t ret;

>>   

>>           if (__builtin_constant_p(x))

>>                  return generic_flsl(x);

>>   

>>           asm("clz\t%0, %1" : "=r" (ret) : "r" (x));

> 

> As x is fixed by the ABI, ret should be unsigned long to match, surely?


No need for it. The result of the instruction clz will always be smaller 
than 64.

I suspect they require a 64-bit register just for simplicity as they 
encode the size for the 2 registers in only a single bit (i.e sf).

> 

> This will compile as it is arm64 specific, but it looks just as wrong

> (per the commit message) as using int in the first place.

See above. I can clarify it in the commit message.

Cheers,

-- 
Julien Grall
Stefano Stabellini April 17, 2019, 8:24 p.m. | #3
On Wed, 27 Mar 2019, Julien Grall wrote:
> Hi,
> 
> On 27/03/2019 19:03, Andrew Cooper wrote:
> > On 27/03/2019 18:45, Julien Grall wrote:
> >> Clang is pickier than GCC for the register size in asm statement. It expects
> >> the register size to match the value size.
> >>
> >> The instruction clz is expecting the two operands to be the same size
> >> (i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit
> >> value, we need to make the destination variable 64-bit as well.
> >>
> >> While at it, add a newline before the return statement.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@arm.com>
> >> ---
> >>   xen/include/asm-arm/arm64/bitops.h | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/include/asm-arm/arm64/bitops.h b/xen/include/asm-arm/arm64/bitops.h
> >> index 6bf1922680..05045f1109 100644
> >> --- a/xen/include/asm-arm/arm64/bitops.h
> >> +++ b/xen/include/asm-arm/arm64/bitops.h
> >> @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long __ffs(unsigned long word)
> >>   
> >>   static inline int flsl(unsigned long x)
> >>   {
> >> -        int ret;
> >> +        uint64_t ret;
> >>   
> >>           if (__builtin_constant_p(x))
> >>                  return generic_flsl(x);
> >>   
> >>           asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
> > 
> > As x is fixed by the ABI, ret should be unsigned long to match, surely?
> 
> No need for it. The result of the instruction clz will always be smaller 
> than 64.
> 
> I suspect they require a 64-bit register just for simplicity as they 
> encode the size for the 2 registers in only a single bit (i.e sf).
> 
> > 
> > This will compile as it is arm64 specific, but it looks just as wrong
> > (per the commit message) as using int in the first place.
> See above. I can clarify it in the commit message.

I agree with Andrew: I can see that the code is correct, but it really
looks wrong at first sight. Why not use unsigned long? This is an arm64
header anyway.
Julien Grall April 17, 2019, 9:02 p.m. | #4
Hi,

On 4/17/19 9:24 PM, Stefano Stabellini wrote:
> On Wed, 27 Mar 2019, Julien Grall wrote:
>> Hi,
>>
>> On 27/03/2019 19:03, Andrew Cooper wrote:
>>> On 27/03/2019 18:45, Julien Grall wrote:
>>>> Clang is pickier than GCC for the register size in asm statement. It expects
>>>> the register size to match the value size.
>>>>
>>>> The instruction clz is expecting the two operands to be the same size
>>>> (i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit
>>>> value, we need to make the destination variable 64-bit as well.
>>>>
>>>> While at it, add a newline before the return statement.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>    xen/include/asm-arm/arm64/bitops.h | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/arm64/bitops.h b/xen/include/asm-arm/arm64/bitops.h
>>>> index 6bf1922680..05045f1109 100644
>>>> --- a/xen/include/asm-arm/arm64/bitops.h
>>>> +++ b/xen/include/asm-arm/arm64/bitops.h
>>>> @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long __ffs(unsigned long word)
>>>>    
>>>>    static inline int flsl(unsigned long x)
>>>>    {
>>>> -        int ret;
>>>> +        uint64_t ret;
>>>>    
>>>>            if (__builtin_constant_p(x))
>>>>                   return generic_flsl(x);
>>>>    
>>>>            asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
>>>
>>> As x is fixed by the ABI, ret should be unsigned long to match, surely?
>>
>> No need for it. The result of the instruction clz will always be smaller
>> than 64.
>>
>> I suspect they require a 64-bit register just for simplicity as they
>> encode the size for the 2 registers in only a single bit (i.e sf).
>>
>>>
>>> This will compile as it is arm64 specific, but it looks just as wrong
>>> (per the commit message) as using int in the first place.
>> See above. I can clarify it in the commit message.
> 
> I agree with Andrew: I can see that the code is correct, but it really
> looks wrong at first sight. Why not use unsigned long? This is an arm64
> header anyway.

While this is implemented in arm64, this is a function used in common 
code... So the prototype would have to be changed everywhere.

However, as I pointed out this is a design decision from Arm64 and I 
think it would be wrong to return unsigned long.

I offered the suggestion to update the commit message. I can also add a 
comment in the code.

Cheers,
Stefano Stabellini April 18, 2019, 6:40 p.m. | #5
On Wed, 17 Apr 2019, Julien Grall wrote:
> On 4/17/19 9:24 PM, Stefano Stabellini wrote:
> > On Wed, 27 Mar 2019, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 27/03/2019 19:03, Andrew Cooper wrote:
> > > > On 27/03/2019 18:45, Julien Grall wrote:
> > > > > Clang is pickier than GCC for the register size in asm statement. It
> > > > > expects
> > > > > the register size to match the value size.
> > > > > 
> > > > > The instruction clz is expecting the two operands to be the same size
> > > > > (i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit
> > > > > value, we need to make the destination variable 64-bit as well.
> > > > > 
> > > > > While at it, add a newline before the return statement.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > > ---
> > > > >    xen/include/asm-arm/arm64/bitops.h | 3 ++-
> > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/xen/include/asm-arm/arm64/bitops.h
> > > > > b/xen/include/asm-arm/arm64/bitops.h
> > > > > index 6bf1922680..05045f1109 100644
> > > > > --- a/xen/include/asm-arm/arm64/bitops.h
> > > > > +++ b/xen/include/asm-arm/arm64/bitops.h
> > > > > @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long
> > > > > __ffs(unsigned long word)
> > > > >       static inline int flsl(unsigned long x)
> > > > >    {
> > > > > -        int ret;
> > > > > +        uint64_t ret;
> > > > >               if (__builtin_constant_p(x))
> > > > >                   return generic_flsl(x);
> > > > >               asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
> > > > 
> > > > As x is fixed by the ABI, ret should be unsigned long to match, surely?
> > > 
> > > No need for it. The result of the instruction clz will always be smaller
> > > than 64.
> > > 
> > > I suspect they require a 64-bit register just for simplicity as they
> > > encode the size for the 2 registers in only a single bit (i.e sf).
> > > 
> > > > 
> > > > This will compile as it is arm64 specific, but it looks just as wrong
> > > > (per the commit message) as using int in the first place.
> > > See above. I can clarify it in the commit message.
> > 
> > I agree with Andrew: I can see that the code is correct, but it really
> > looks wrong at first sight. Why not use unsigned long? This is an arm64
> > header anyway.
> 
> While this is implemented in arm64, this is a function used in common code...
> So the prototype would have to be changed everywhere.
> 
> However, as I pointed out this is a design decision from Arm64 and I think it
> would be wrong to return unsigned long.
> 
> I offered the suggestion to update the commit message. I can also add a
> comment in the code.

Reading your reply, I am pretty sure that we are talking about different
things. I agree that we don't want to change the prototype everywhere,
arm32, x86. It would be good to add a comment to the code or the commit
message, I leave it to you.

My suggestion was basically "code style". I only meant the below. To me
it looks nicer because at least the ret var is the same type of the
argument, and it is more immediately obvious that the clz operation is
correct. Not a big deal.

diff --git a/xen/include/asm-arm/arm64/bitops.h b/xen/include/asm-arm/arm64/bitops.h
index 6bf1922..44cceb3 100644
--- a/xen/include/asm-arm/arm64/bitops.h
+++ b/xen/include/asm-arm/arm64/bitops.h
@@ -34,7 +34,7 @@ static /*__*/always_inline unsigned long __ffs(unsigned long word)
 
 static inline int flsl(unsigned long x)
 {
-        int ret;
+        unsigned long ret;
 
         if (__builtin_constant_p(x))
                return generic_flsl(x);
Julien Grall April 18, 2019, 6:57 p.m. | #6
Hi,

On 18/04/2019 19:40, Stefano Stabellini wrote:
> On Wed, 17 Apr 2019, Julien Grall wrote:
>> On 4/17/19 9:24 PM, Stefano Stabellini wrote:
>>> On Wed, 27 Mar 2019, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 27/03/2019 19:03, Andrew Cooper wrote:
>>>>> On 27/03/2019 18:45, Julien Grall wrote:
>>>>>> Clang is pickier than GCC for the register size in asm statement. It
>>>>>> expects
>>>>>> the register size to match the value size.
>>>>>>
>>>>>> The instruction clz is expecting the two operands to be the same size
>>>>>> (i.e 32-bit or 64-bit). As the flsl function is dealing with 64-bit
>>>>>> value, we need to make the destination variable 64-bit as well.
>>>>>>
>>>>>> While at it, add a newline before the return statement.
>>>>>>
>>>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>>> ---
>>>>>>     xen/include/asm-arm/arm64/bitops.h | 3 ++-
>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/xen/include/asm-arm/arm64/bitops.h
>>>>>> b/xen/include/asm-arm/arm64/bitops.h
>>>>>> index 6bf1922680..05045f1109 100644
>>>>>> --- a/xen/include/asm-arm/arm64/bitops.h
>>>>>> +++ b/xen/include/asm-arm/arm64/bitops.h
>>>>>> @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long
>>>>>> __ffs(unsigned long word)
>>>>>>        static inline int flsl(unsigned long x)
>>>>>>     {
>>>>>> -        int ret;
>>>>>> +        uint64_t ret;
>>>>>>                if (__builtin_constant_p(x))
>>>>>>                    return generic_flsl(x);
>>>>>>                asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
>>>>>
>>>>> As x is fixed by the ABI, ret should be unsigned long to match, surely?
>>>>
>>>> No need for it. The result of the instruction clz will always be smaller
>>>> than 64.
>>>>
>>>> I suspect they require a 64-bit register just for simplicity as they
>>>> encode the size for the 2 registers in only a single bit (i.e sf).
>>>>
>>>>>
>>>>> This will compile as it is arm64 specific, but it looks just as wrong
>>>>> (per the commit message) as using int in the first place.
>>>> See above. I can clarify it in the commit message.
>>>
>>> I agree with Andrew: I can see that the code is correct, but it really
>>> looks wrong at first sight. Why not use unsigned long? This is an arm64
>>> header anyway.
>>
>> While this is implemented in arm64, this is a function used in common code...
>> So the prototype would have to be changed everywhere.
>>
>> However, as I pointed out this is a design decision from Arm64 and I think it
>> would be wrong to return unsigned long.
>>
>> I offered the suggestion to update the commit message. I can also add a
>> comment in the code.
> 
> Reading your reply, I am pretty sure that we are talking about different
> things. I agree that we don't want to change the prototype everywhere,
> arm32, x86. It would be good to add a comment to the code or the commit
> message, I leave it to you.
> 
> My suggestion was basically "code style". I only meant the below. To me
> it looks nicer because at least the ret var is the same type of the
> argument, and it is more immediately obvious that the clz operation is
> correct. Not a big deal.

I find quite amusing that you suggest this when in the patch #7 you mention the 
MISRA rule 10.1 which forbid assigning a value to a narrower type (unsigned long 
-> int).

Regardless that, clz operation clearly ask for a 64-bit value. So uint64_t makes 
much more sense than "unsigned long". In general, I am a strong advocate to use 
uintX_t when the size is known. "unsigned long" should only be used in place you 
need compat between 32-bit and 64-bit code.

Cheers,
Stefano Stabellini April 18, 2019, 7 p.m. | #7
On Thu, 18 Apr 2019, Julien Grall wrote:
> Hi,
> 
> On 18/04/2019 19:40, Stefano Stabellini wrote:
> > On Wed, 17 Apr 2019, Julien Grall wrote:
> > > On 4/17/19 9:24 PM, Stefano Stabellini wrote:
> > > > On Wed, 27 Mar 2019, Julien Grall wrote:
> > > > > Hi,
> > > > > 
> > > > > On 27/03/2019 19:03, Andrew Cooper wrote:
> > > > > > On 27/03/2019 18:45, Julien Grall wrote:
> > > > > > > Clang is pickier than GCC for the register size in asm statement.
> > > > > > > It
> > > > > > > expects
> > > > > > > the register size to match the value size.
> > > > > > > 
> > > > > > > The instruction clz is expecting the two operands to be the same
> > > > > > > size
> > > > > > > (i.e 32-bit or 64-bit). As the flsl function is dealing with
> > > > > > > 64-bit
> > > > > > > value, we need to make the destination variable 64-bit as well.
> > > > > > > 
> > > > > > > While at it, add a newline before the return statement.
> > > > > > > 
> > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > > > > ---
> > > > > > >     xen/include/asm-arm/arm64/bitops.h | 3 ++-
> > > > > > >     1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/xen/include/asm-arm/arm64/bitops.h
> > > > > > > b/xen/include/asm-arm/arm64/bitops.h
> > > > > > > index 6bf1922680..05045f1109 100644
> > > > > > > --- a/xen/include/asm-arm/arm64/bitops.h
> > > > > > > +++ b/xen/include/asm-arm/arm64/bitops.h
> > > > > > > @@ -34,12 +34,13 @@ static /*__*/always_inline unsigned long
> > > > > > > __ffs(unsigned long word)
> > > > > > >        static inline int flsl(unsigned long x)
> > > > > > >     {
> > > > > > > -        int ret;
> > > > > > > +        uint64_t ret;
> > > > > > >                if (__builtin_constant_p(x))
> > > > > > >                    return generic_flsl(x);
> > > > > > >                asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
> > > > > > 
> > > > > > As x is fixed by the ABI, ret should be unsigned long to match,
> > > > > > surely?
> > > > > 
> > > > > No need for it. The result of the instruction clz will always be
> > > > > smaller
> > > > > than 64.
> > > > > 
> > > > > I suspect they require a 64-bit register just for simplicity as they
> > > > > encode the size for the 2 registers in only a single bit (i.e sf).
> > > > > 
> > > > > > 
> > > > > > This will compile as it is arm64 specific, but it looks just as
> > > > > > wrong
> > > > > > (per the commit message) as using int in the first place.
> > > > > See above. I can clarify it in the commit message.
> > > > 
> > > > I agree with Andrew: I can see that the code is correct, but it really
> > > > looks wrong at first sight. Why not use unsigned long? This is an arm64
> > > > header anyway.
> > > 
> > > While this is implemented in arm64, this is a function used in common
> > > code...
> > > So the prototype would have to be changed everywhere.
> > > 
> > > However, as I pointed out this is a design decision from Arm64 and I think
> > > it
> > > would be wrong to return unsigned long.
> > > 
> > > I offered the suggestion to update the commit message. I can also add a
> > > comment in the code.
> > 
> > Reading your reply, I am pretty sure that we are talking about different
> > things. I agree that we don't want to change the prototype everywhere,
> > arm32, x86. It would be good to add a comment to the code or the commit
> > message, I leave it to you.
> > 
> > My suggestion was basically "code style". I only meant the below. To me
> > it looks nicer because at least the ret var is the same type of the
> > argument, and it is more immediately obvious that the clz operation is
> > correct. Not a big deal.
> 
> I find quite amusing that you suggest this when in the patch #7 you mention
> the MISRA rule 10.1 which forbid assigning a value to a narrower type
> (unsigned long -> int).
> 
> Regardless that, clz operation clearly ask for a 64-bit value. So uint64_t
> makes much more sense than "unsigned long". In general, I am a strong advocate
> to use uintX_t when the size is known. "unsigned long" should only be used in
> place you need compat between 32-bit and 64-bit code.

all right, OK by me

Patch

diff --git a/xen/include/asm-arm/arm64/bitops.h b/xen/include/asm-arm/arm64/bitops.h
index 6bf1922680..05045f1109 100644
--- a/xen/include/asm-arm/arm64/bitops.h
+++ b/xen/include/asm-arm/arm64/bitops.h
@@ -34,12 +34,13 @@  static /*__*/always_inline unsigned long __ffs(unsigned long word)
 
 static inline int flsl(unsigned long x)
 {
-        int ret;
+        uint64_t ret;
 
         if (__builtin_constant_p(x))
                return generic_flsl(x);
 
         asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
+
         return BITS_PER_LONG - ret;
 }