[Xen-devel,07/12] xen/arm: cpuerrata: Match register size with value size in check_workaround_*

Message ID 20190327184531.30986-8-julien.grall@arm.com
State New
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 asm statement expects a 32-bit (resp. 64-bit) value on Arm32
(resp. Arm64) whereas the value is a boolean (Clang consider to be
32-bit).

It would be possible to impose 32-bit register for both architecture
but this require the code to use __OP32. However, it does not really
improve the assembly generated. Instead, replace switch the variable
to use register_t.

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

Comments

Stefano Stabellini April 17, 2019, 8:28 p.m. | #1
On Wed, 27 Mar 2019, 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 asm statement expects a 32-bit (resp. 64-bit) value on Arm32
> (resp. Arm64) whereas the value is a boolean (Clang consider to be
> 32-bit).
> 
> It would be possible to impose 32-bit register for both architecture
> but this require the code to use __OP32. However, it does not really
> improve the assembly generated. Instead, replace switch the variable
> to use register_t.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/include/asm-arm/cpuerrata.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
> index 55ddfda272..88ef3ca934 100644
> --- a/xen/include/asm-arm/cpuerrata.h
> +++ b/xen/include/asm-arm/cpuerrata.h
> @@ -14,7 +14,7 @@ static inline bool check_workaround_##erratum(void)             \
>          return false;                                           \
>      else                                                        \
>      {                                                           \
> -        bool ret;                                               \
> +        register_t ret;                                         \
>                                                                  \
>          asm volatile (ALTERNATIVE("mov %0, #0",                 \
>                                    "mov %0, #1",                 \

This is OK. Could you please also change the return statement below?
Maybe something like:

  return unlikely(!!ret);

Thank you!
Julien Grall April 17, 2019, 9:15 p.m. | #2
Hi,

On 4/17/19 9:28 PM, Stefano Stabellini wrote:
> On Wed, 27 Mar 2019, 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 asm statement expects a 32-bit (resp. 64-bit) value on Arm32
>> (resp. Arm64) whereas the value is a boolean (Clang consider to be
>> 32-bit).
>>
>> It would be possible to impose 32-bit register for both architecture
>> but this require the code to use __OP32. However, it does not really
>> improve the assembly generated. Instead, replace switch the variable
>> to use register_t.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/include/asm-arm/cpuerrata.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
>> index 55ddfda272..88ef3ca934 100644
>> --- a/xen/include/asm-arm/cpuerrata.h
>> +++ b/xen/include/asm-arm/cpuerrata.h
>> @@ -14,7 +14,7 @@ static inline bool check_workaround_##erratum(void)             \
>>           return false;                                           \
>>       else                                                        \
>>       {                                                           \
>> -        bool ret;                                               \
>> +        register_t ret;                                         \
>>                                                                   \
>>           asm volatile (ALTERNATIVE("mov %0, #0",                 \
>>                                     "mov %0, #1",                 \
> 
> This is OK. Could you please also change the return statement below?
> Maybe something like:
> 
>    return unlikely(!!ret);
Why? The compiler will implicitly convert the int to bool. 0 will turn 
to false, all the other will be true.

We actually been actively removing !! when the type is bool (see the 
example in get_paged_frame in common/grant_table.c).

Cheers,
Stefano Stabellini April 18, 2019, 6:23 p.m. | #3
On Wed, 17 Apr 2019, Julien Grall wrote:
> Hi,
> 
> On 4/17/19 9:28 PM, Stefano Stabellini wrote:
> > On Wed, 27 Mar 2019, 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 asm statement expects a 32-bit (resp. 64-bit) value on Arm32
> > > (resp. Arm64) whereas the value is a boolean (Clang consider to be
> > > 32-bit).
> > > 
> > > It would be possible to impose 32-bit register for both architecture
> > > but this require the code to use __OP32. However, it does not really
> > > improve the assembly generated. Instead, replace switch the variable
> > > to use register_t.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >   xen/include/asm-arm/cpuerrata.h | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/xen/include/asm-arm/cpuerrata.h
> > > b/xen/include/asm-arm/cpuerrata.h
> > > index 55ddfda272..88ef3ca934 100644
> > > --- a/xen/include/asm-arm/cpuerrata.h
> > > +++ b/xen/include/asm-arm/cpuerrata.h
> > > @@ -14,7 +14,7 @@ static inline bool check_workaround_##erratum(void)
> > > \
> > >           return false;                                           \
> > >       else                                                        \
> > >       {                                                           \
> > > -        bool ret;                                               \
> > > +        register_t ret;                                         \
> > >                                                                   \
> > >           asm volatile (ALTERNATIVE("mov %0, #0",                 \
> > >                                     "mov %0, #1",                 \
> > 
> > This is OK. Could you please also change the return statement below?
> > Maybe something like:
> > 
> >    return unlikely(!!ret);
> Why? The compiler will implicitly convert the int to bool. 0 will turn to
> false, all the other will be true.
> 
> We actually been actively removing !! when the type is bool (see the example
> in get_paged_frame in common/grant_table.c).

Really? Too bad, I loved the explicit conversions to bool. This is a
matter of code style, not correctness, so usually I wouldn't care much.
But I went to read MISRA-C to figure out if there are any differences
from that point of view. From Rule 10.3, it looks like it is not
compliant, because they say that:

  bool_t bla = 0;

is not MISRA-C compliant. While:

  int c = 1;
  bool_t bla = c == 0;

is compliant. So, if I read this right:

  return !!ret //compliant
  return ret;  //not compliant

I am not 100% sure though.
Julien Grall April 18, 2019, 6:47 p.m. | #4
Hi,

On 18/04/2019 19:23, Stefano Stabellini wrote:
> On Wed, 17 Apr 2019, Julien Grall wrote:
>> Hi,
>>
>> On 4/17/19 9:28 PM, Stefano Stabellini wrote:
>>> On Wed, 27 Mar 2019, 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 asm statement expects a 32-bit (resp. 64-bit) value on Arm32
>>>> (resp. Arm64) whereas the value is a boolean (Clang consider to be
>>>> 32-bit).
>>>>
>>>> It would be possible to impose 32-bit register for both architecture
>>>> but this require the code to use __OP32. However, it does not really
>>>> improve the assembly generated. Instead, replace switch the variable
>>>> to use register_t.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>    xen/include/asm-arm/cpuerrata.h | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/cpuerrata.h
>>>> b/xen/include/asm-arm/cpuerrata.h
>>>> index 55ddfda272..88ef3ca934 100644
>>>> --- a/xen/include/asm-arm/cpuerrata.h
>>>> +++ b/xen/include/asm-arm/cpuerrata.h
>>>> @@ -14,7 +14,7 @@ static inline bool check_workaround_##erratum(void)
>>>> \
>>>>            return false;                                           \
>>>>        else                                                        \
>>>>        {                                                           \
>>>> -        bool ret;                                               \
>>>> +        register_t ret;                                         \
>>>>                                                                    \
>>>>            asm volatile (ALTERNATIVE("mov %0, #0",                 \
>>>>                                      "mov %0, #1",                 \
>>>
>>> This is OK. Could you please also change the return statement below?
>>> Maybe something like:
>>>
>>>     return unlikely(!!ret);
>> Why? The compiler will implicitly convert the int to bool. 0 will turn to
>> false, all the other will be true.
>>
>> We actually been actively removing !! when the type is bool (see the example
>> in get_paged_frame in common/grant_table.c).
> 
> Really? Too bad, I loved the explicit conversions to bool. This is a
> matter of code style, not correctness, so usually I wouldn't care much.
> But I went to read MISRA-C to figure out if there are any differences
> from that point of view. From Rule 10.3, it looks like it is not
> compliant, because they say that:
> 
>    bool_t bla = 0;
> 
> is not MISRA-C compliant. While:
> 
>    int c = 1;
>    bool_t bla = c == 0;
> 
> is compliant. So, if I read this right:
> 
>    return !!ret //compliant
>    return ret;  //not compliant
> 
> I am not 100% sure though.

And if you read that rule the following would also be non-compliant

bool is_nonzero(int b)
{
   return b;
}

I know this example is pretty exaggerated but then does it mean the following 
code is also non-compliant?

bool is_nonzero(int b)
{
     if (b)
       return true;
     else
       return false;
}

If it is considered compliant, then it does not make sense.

Cheers,
Stefano Stabellini April 18, 2019, 6:52 p.m. | #5
On Thu, 18 Apr 2019, Julien Grall wrote:
> Hi,
> 
> On 18/04/2019 19:23, Stefano Stabellini wrote:
> > On Wed, 17 Apr 2019, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 4/17/19 9:28 PM, Stefano Stabellini wrote:
> > > > On Wed, 27 Mar 2019, 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 asm statement expects a 32-bit (resp. 64-bit) value on Arm32
> > > > > (resp. Arm64) whereas the value is a boolean (Clang consider to be
> > > > > 32-bit).
> > > > > 
> > > > > It would be possible to impose 32-bit register for both architecture
> > > > > but this require the code to use __OP32. However, it does not really
> > > > > improve the assembly generated. Instead, replace switch the variable
> > > > > to use register_t.
> > > > > 
> > > > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > > > ---
> > > > >    xen/include/asm-arm/cpuerrata.h | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/xen/include/asm-arm/cpuerrata.h
> > > > > b/xen/include/asm-arm/cpuerrata.h
> > > > > index 55ddfda272..88ef3ca934 100644
> > > > > --- a/xen/include/asm-arm/cpuerrata.h
> > > > > +++ b/xen/include/asm-arm/cpuerrata.h
> > > > > @@ -14,7 +14,7 @@ static inline bool check_workaround_##erratum(void)
> > > > > \
> > > > >            return false;                                           \
> > > > >        else                                                        \
> > > > >        {                                                           \
> > > > > -        bool ret;                                               \
> > > > > +        register_t ret;                                         \
> > > > >                                                                    \
> > > > >            asm volatile (ALTERNATIVE("mov %0, #0",                 \
> > > > >                                      "mov %0, #1",                 \
> > > > 
> > > > This is OK. Could you please also change the return statement below?
> > > > Maybe something like:
> > > > 
> > > >     return unlikely(!!ret);
> > > Why? The compiler will implicitly convert the int to bool. 0 will turn to
> > > false, all the other will be true.
> > > 
> > > We actually been actively removing !! when the type is bool (see the
> > > example
> > > in get_paged_frame in common/grant_table.c).
> > 
> > Really? Too bad, I loved the explicit conversions to bool. This is a
> > matter of code style, not correctness, so usually I wouldn't care much.
> > But I went to read MISRA-C to figure out if there are any differences
> > from that point of view. From Rule 10.3, it looks like it is not
> > compliant, because they say that:
> > 
> >    bool_t bla = 0;
> > 
> > is not MISRA-C compliant. While:
> > 
> >    int c = 1;
> >    bool_t bla = c == 0;
> > 
> > is compliant. So, if I read this right:
> > 
> >    return !!ret //compliant
> >    return ret;  //not compliant
> > 
> > I am not 100% sure though.
> 
> And if you read that rule the following would also be non-compliant
> 
> bool is_nonzero(int b)
> {
>   return b;
> }

Yes, I think you are right.


> I know this example is pretty exaggerated but then does it mean the following
> code is also non-compliant?
> 
> bool is_nonzero(int b)
> {
>     if (b)
>       return true;
>     else
>       return false;
> }
> 
> If it is considered compliant, then it does not make sense.

Yes, I think this is not compliant too. Also, from what I have been
told, this example is famous for being one of the most extreme examples
of MISRA-C non-compliance. I think the compliant version would be:

bool is_nonzero(int b)
{
    if (b != 0)
      return true;
    else
      return false;
}

This is also compliant:

bool is_nonzero(int b)
{
    return (b != 0);
}
Julien Grall April 18, 2019, 7:01 p.m. | #6
On 18/04/2019 19:52, Stefano Stabellini wrote:
> On Thu, 18 Apr 2019, Julien Grall wrote:
>> On 18/04/2019 19:23, Stefano Stabellini wrote:
>>> On Wed, 17 Apr 2019, Julien Grall wrote:
>>>> On 4/17/19 9:28 PM, Stefano Stabellini wrote:
>>>>> On Wed, 27 Mar 2019, Julien Grall wrote:
>> If it is considered compliant, then it does not make sense.
> 
> Yes, I think this is not compliant too. Also, from what I have been
> told, this example is famous for being one of the most extreme examples
> of MISRA-C non-compliance. I think the compliant version would be:
> 
> bool is_nonzero(int b)
> {
>      if (b != 0)
>        return true;
>      else
>        return false;
> }
> 
> This is also compliant:
> 
> bool is_nonzero(int b)
> {
>      return (b != 0);
> }
> 

I will use !!b here and in the next patch. But I still doubt you will be able to 
enforce it in Xen. if ( n ) is quite a common pattern.

Cheers,
Stefano Stabellini April 18, 2019, 7:04 p.m. | #7
On Thu, 18 Apr 2019, Julien Grall wrote:
> On 18/04/2019 19:52, Stefano Stabellini wrote:
> > On Thu, 18 Apr 2019, Julien Grall wrote:
> > > On 18/04/2019 19:23, Stefano Stabellini wrote:
> > > > On Wed, 17 Apr 2019, Julien Grall wrote:
> > > > > On 4/17/19 9:28 PM, Stefano Stabellini wrote:
> > > > > > On Wed, 27 Mar 2019, Julien Grall wrote:
> > > If it is considered compliant, then it does not make sense.
> > 
> > Yes, I think this is not compliant too. Also, from what I have been
> > told, this example is famous for being one of the most extreme examples
> > of MISRA-C non-compliance. I think the compliant version would be:
> > 
> > bool is_nonzero(int b)
> > {
> >      if (b != 0)
> >        return true;
> >      else
> >        return false;
> > }
> > 
> > This is also compliant:
> > 
> > bool is_nonzero(int b)
> > {
> >      return (b != 0);
> > }
> > 
> 
> I will use !!b here and in the next patch. But I still doubt you will be able
> to enforce it in Xen. if ( n ) is quite a common pattern.
 
Thanks. Baby steps :-)

Patch

diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
index 55ddfda272..88ef3ca934 100644
--- a/xen/include/asm-arm/cpuerrata.h
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -14,7 +14,7 @@  static inline bool check_workaround_##erratum(void)             \
         return false;                                           \
     else                                                        \
     {                                                           \
-        bool ret;                                               \
+        register_t ret;                                         \
                                                                 \
         asm volatile (ALTERNATIVE("mov %0, #0",                 \
                                   "mov %0, #1",                 \