[Xen-devel,02/12] xen/arm: fix get_cpu_info() when built with clang

Message ID 20190327184531.30986-3-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 understands the GCCism in use here, but still complains that sp is
unitialised. In such cases, resort to the older versions of this code,
which directly read sp into the temporary variable.

Note that we still keep the GCCism in the default case, as it causes GCC
to create rather better assembly.

This is based on the x86 counterpart.

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

Comments

Stefano Stabellini April 17, 2019, 8:45 p.m. | #1
On Wed, 27 Mar 2019, Julien Grall wrote:
> Clang understands the GCCism in use here, but still complains that sp is
> unitialised. In such cases, resort to the older versions of this code,
> which directly read sp into the temporary variable.
> 
> Note that we still keep the GCCism in the default case, as it causes GCC
> to create rather better assembly.
> 
> This is based on the x86 counterpart.

I understand this is based on an existing approach but what about other
compilers? I have a suggestion below.


> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/include/asm-arm/current.h | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
> index c4af66fbb9..6b7c1df64d 100644
> --- a/xen/include/asm-arm/current.h
> +++ b/xen/include/asm-arm/current.h
> @@ -28,8 +28,16 @@ struct cpu_info {
>  
>  static inline struct cpu_info *get_cpu_info(void)
>  {
> +#ifdef __clang__
> +    unsigned long sp;
> +
> +    asm ("mov %0, sp" : "=r" (sp));
> +#else
>      register unsigned long sp asm ("sp");
> -    return (struct cpu_info *)((sp & ~(STACK_SIZE - 1)) + STACK_SIZE - sizeof(struct cpu_info));
> +#endif
> +
> +    return (struct cpu_info *)((sp & ~(STACK_SIZE - 1)) +
> +                               STACK_SIZE - sizeof(struct cpu_info));
>  }

I think it makes sense to switch the #ifdef around:

#ifdef __GNUC__
    /* gcc specific optimization */
#else
   /* regular implementation */
#endif
Julien Grall April 17, 2019, 9 p.m. | #2
Hi,

On 4/17/19 9:45 PM, Stefano Stabellini wrote:
> On Wed, 27 Mar 2019, Julien Grall wrote:
>> Clang understands the GCCism in use here, but still complains that sp is
>> unitialised. In such cases, resort to the older versions of this code,
>> which directly read sp into the temporary variable.
>>
>> Note that we still keep the GCCism in the default case, as it causes GCC
>> to create rather better assembly.
>>
>> This is based on the x86 counterpart.
> 
> I understand this is based on an existing approach but what about other
> compilers? I have a suggestion below.

What if the compiler actually support named registers? Why would we make 
the code less efficient?

Cheers,
Stefano Stabellini April 18, 2019, 6:03 p.m. | #3
On Wed, 17 Apr 2019, Julien Grall wrote:
> Hi,
> 
> On 4/17/19 9:45 PM, Stefano Stabellini wrote:
> > On Wed, 27 Mar 2019, Julien Grall wrote:
> > > Clang understands the GCCism in use here, but still complains that sp is
> > > unitialised. In such cases, resort to the older versions of this code,
> > > which directly read sp into the temporary variable.
> > > 
> > > Note that we still keep the GCCism in the default case, as it causes GCC
> > > to create rather better assembly.
> > > 
> > > This is based on the x86 counterpart.
> > 
> > I understand this is based on an existing approach but what about other
> > compilers? I have a suggestion below.
> 
> What if the compiler actually support named registers? Why would we make the
> code less efficient?

It is not my intention to make the code less efficient for other
compilers. However, reading the commit message and the patch I have the
impression that the clang version is more likely to be applicable to
other compilers, compared to the gcc version. More "standard". The
reason is that the clang version only requires asm inline, while the gcc
version requires both asm inline and named registers. For the sake of
getting Xen to compile out of the box with any C compiler, I think it is
best if we default to the less demanding version of the implementation
for unknown compilers.
Julien Grall Sept. 29, 2019, 2:26 p.m. | #4
Hi,

Sorry, I am picking up this series again.

On 4/18/19 7:03 PM, Stefano Stabellini wrote:
> On Wed, 17 Apr 2019, Julien Grall wrote:
>> Hi,
>>
>> On 4/17/19 9:45 PM, Stefano Stabellini wrote:
>>> On Wed, 27 Mar 2019, Julien Grall wrote:
>>>> Clang understands the GCCism in use here, but still complains that sp is
>>>> unitialised. In such cases, resort to the older versions of this code,
>>>> which directly read sp into the temporary variable.
>>>>
>>>> Note that we still keep the GCCism in the default case, as it causes GCC
>>>> to create rather better assembly.
>>>>
>>>> This is based on the x86 counterpart.
>>>
>>> I understand this is based on an existing approach but what about other
>>> compilers? I have a suggestion below.
>>
>> What if the compiler actually support named registers? Why would we make the
>> code less efficient?
> 
> It is not my intention to make the code less efficient for other
> compilers. However, reading the commit message and the patch I have the
> impression that the clang version is more likely to be applicable to
> other compilers, compared to the gcc version. More "standard". The
> reason is that the clang version only requires asm inline, while the gcc
> version requires both asm inline and named registers. For the sake of
> getting Xen to compile out of the box with any C compiler, I think it is
> best if we default to the less demanding version of the implementation
> for unknown compilers.
While building Xen out of box is nice goal to have, this is likely be 
very hard to reach out because Xen is using a lot of GCCism. It mostly 
work with Clang because they have adopted some of them.

I would be happy to revert the condition, but then AFAICT there are no 
pretty way to now that we are using GCC. While the define __GNUC__ is 
meant to tell you this is compiled with GCC, clang is also defining it.

So the condition would have to be

#if !defined(__clang__) && defined(__GNUC__)

But then if clang is already defining __GNUC__, what actually prevents 
any other to do it?

I have yet to see anyone wanted to build Xen with another compiler other 
than clang and GCC. So I will leave this patch as is. Feel free to 
suggest a different approach if you are not happy with this.

Cheers,
Stefano Stabellini Oct. 1, 2019, 1:15 a.m. | #5
On Sun, 29 Sep 2019, Julien Grall wrote:
> Hi,
> 
> Sorry, I am picking up this series again.
> 
> On 4/18/19 7:03 PM, Stefano Stabellini wrote:
> > On Wed, 17 Apr 2019, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 4/17/19 9:45 PM, Stefano Stabellini wrote:
> > > > On Wed, 27 Mar 2019, Julien Grall wrote:
> > > > > Clang understands the GCCism in use here, but still complains that sp
> > > > > is
> > > > > unitialised. In such cases, resort to the older versions of this code,
> > > > > which directly read sp into the temporary variable.
> > > > > 
> > > > > Note that we still keep the GCCism in the default case, as it causes
> > > > > GCC
> > > > > to create rather better assembly.
> > > > > 
> > > > > This is based on the x86 counterpart.
> > > > 
> > > > I understand this is based on an existing approach but what about other
> > > > compilers? I have a suggestion below.
> > > 
> > > What if the compiler actually support named registers? Why would we make
> > > the
> > > code less efficient?
> > 
> > It is not my intention to make the code less efficient for other
> > compilers. However, reading the commit message and the patch I have the
> > impression that the clang version is more likely to be applicable to
> > other compilers, compared to the gcc version. More "standard". The
> > reason is that the clang version only requires asm inline, while the gcc
> > version requires both asm inline and named registers. For the sake of
> > getting Xen to compile out of the box with any C compiler, I think it is
> > best if we default to the less demanding version of the implementation
> > for unknown compilers.
> While building Xen out of box is nice goal to have, this is likely be very
> hard to reach out because Xen is using a lot of GCCism. It mostly work with
> Clang because they have adopted some of them.
> 
> I would be happy to revert the condition, but then AFAICT there are no pretty
> way to now that we are using GCC. While the define __GNUC__ is meant to tell
> you this is compiled with GCC, clang is also defining it.

That's horrible, I didn't know about that!


> So the condition would have to be
> 
> #if !defined(__clang__) && defined(__GNUC__)

:-(


> But then if clang is already defining __GNUC__, what actually prevents any
> other to do it?
> 
> I have yet to see anyone wanted to build Xen with another compiler other than
> clang and GCC. So I will leave this patch as is. Feel free to suggest a
> different approach if you are not happy with this.

Is there a __REALLY_REALLY_GUNC__ variable? I guess not, so I don't have
a better suggestion. This problem is quite annoying (not your fault of
course) I wonder how other projects deal with it. There must be a
"clean" way to distinguish gcc from others?

For now, I am OK with this patch as is because I wouldn't know what else
to suggest, and I agree that !defined(__clang__) && defined(__GNUC__) is
bad.
Stefano Stabellini Oct. 1, 2019, 1:22 a.m. | #6
On Mon, 30 Sep 2019, Stefano Stabellini wrote:
> On Sun, 29 Sep 2019, Julien Grall wrote:
> > Hi,
> > 
> > Sorry, I am picking up this series again.
> > 
> > On 4/18/19 7:03 PM, Stefano Stabellini wrote:
> > > On Wed, 17 Apr 2019, Julien Grall wrote:
> > > > Hi,
> > > > 
> > > > On 4/17/19 9:45 PM, Stefano Stabellini wrote:
> > > > > On Wed, 27 Mar 2019, Julien Grall wrote:
> > > > > > Clang understands the GCCism in use here, but still complains that sp
> > > > > > is
> > > > > > unitialised. In such cases, resort to the older versions of this code,
> > > > > > which directly read sp into the temporary variable.
> > > > > > 
> > > > > > Note that we still keep the GCCism in the default case, as it causes
> > > > > > GCC
> > > > > > to create rather better assembly.
> > > > > > 
> > > > > > This is based on the x86 counterpart.
> > > > > 
> > > > > I understand this is based on an existing approach but what about other
> > > > > compilers? I have a suggestion below.
> > > > 
> > > > What if the compiler actually support named registers? Why would we make
> > > > the
> > > > code less efficient?
> > > 
> > > It is not my intention to make the code less efficient for other
> > > compilers. However, reading the commit message and the patch I have the
> > > impression that the clang version is more likely to be applicable to
> > > other compilers, compared to the gcc version. More "standard". The
> > > reason is that the clang version only requires asm inline, while the gcc
> > > version requires both asm inline and named registers. For the sake of
> > > getting Xen to compile out of the box with any C compiler, I think it is
> > > best if we default to the less demanding version of the implementation
> > > for unknown compilers.
> > While building Xen out of box is nice goal to have, this is likely be very
> > hard to reach out because Xen is using a lot of GCCism. It mostly work with
> > Clang because they have adopted some of them.
> > 
> > I would be happy to revert the condition, but then AFAICT there are no pretty
> > way to now that we are using GCC. While the define __GNUC__ is meant to tell
> > you this is compiled with GCC, clang is also defining it.
> 
> That's horrible, I didn't know about that!
> 
> 
> > So the condition would have to be
> > 
> > #if !defined(__clang__) && defined(__GNUC__)
> 
> :-(
> 
> 
> > But then if clang is already defining __GNUC__, what actually prevents any
> > other to do it?
> > 
> > I have yet to see anyone wanted to build Xen with another compiler other than
> > clang and GCC. So I will leave this patch as is. Feel free to suggest a
> > different approach if you are not happy with this.
> 
> Is there a __REALLY_REALLY_GUNC__ variable? I guess not, so I don't have
> a better suggestion. This problem is quite annoying (not your fault of
> course) I wonder how other projects deal with it. There must be a
> "clean" way to distinguish gcc from others?
> 
> For now, I am OK with this patch as is because I wouldn't know what else
> to suggest, and I agree that !defined(__clang__) && defined(__GNUC__) is
> bad.

and you can add:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Julien Grall Oct. 1, 2019, 9:47 a.m. | #7
Hi,

On 01/10/2019 02:15, Stefano Stabellini wrote:
> On Sun, 29 Sep 2019, Julien Grall wrote:
>> On 4/18/19 7:03 PM, Stefano Stabellini wrote:
>>> On Wed, 17 Apr 2019, Julien Grall wrote:
>>>> On 4/17/19 9:45 PM, Stefano Stabellini wrote:
>>>>> On Wed, 27 Mar 2019, Julien Grall wrote:
>> But then if clang is already defining __GNUC__, what actually prevents any
>> other to do it?
>>
>> I have yet to see anyone wanted to build Xen with another compiler other than
>> clang and GCC. So I will leave this patch as is. Feel free to suggest a
>> different approach if you are not happy with this.
> 
> Is there a __REALLY_REALLY_GUNC__ variable? I guess not, so I don't have
> a better suggestion. This problem is quite annoying (not your fault of
> course) I wonder how other projects deal with it. There must be a
> "clean" way to distinguish gcc from others?

Linux only supports clang and GCC. AFAICT they are using !__clang__ to detect if 
GCC is used.

Cheers,
Julien Grall Oct. 1, 2019, 9:49 a.m. | #8
On 01/10/2019 02:22, Stefano Stabellini wrote:
> On Mon, 30 Sep 2019, Stefano Stabellini wrote:
>> On Sun, 29 Sep 2019, Julien Grall wrote:
>>> Hi,
>>>
>>> Sorry, I am picking up this series again.
>>>
>>> On 4/18/19 7:03 PM, Stefano Stabellini wrote:
>>>> On Wed, 17 Apr 2019, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 4/17/19 9:45 PM, Stefano Stabellini wrote:
>>>>>> On Wed, 27 Mar 2019, Julien Grall wrote:
>>>>>>> Clang understands the GCCism in use here, but still complains that sp
>>>>>>> is
>>>>>>> unitialised. In such cases, resort to the older versions of this code,
>>>>>>> which directly read sp into the temporary variable.
>>>>>>>
>>>>>>> Note that we still keep the GCCism in the default case, as it causes
>>>>>>> GCC
>>>>>>> to create rather better assembly.
>>>>>>>
>>>>>>> This is based on the x86 counterpart.
>>>>>>
>>>>>> I understand this is based on an existing approach but what about other
>>>>>> compilers? I have a suggestion below.
>>>>>
>>>>> What if the compiler actually support named registers? Why would we make
>>>>> the
>>>>> code less efficient?
>>>>
>>>> It is not my intention to make the code less efficient for other
>>>> compilers. However, reading the commit message and the patch I have the
>>>> impression that the clang version is more likely to be applicable to
>>>> other compilers, compared to the gcc version. More "standard". The
>>>> reason is that the clang version only requires asm inline, while the gcc
>>>> version requires both asm inline and named registers. For the sake of
>>>> getting Xen to compile out of the box with any C compiler, I think it is
>>>> best if we default to the less demanding version of the implementation
>>>> for unknown compilers.
>>> While building Xen out of box is nice goal to have, this is likely be very
>>> hard to reach out because Xen is using a lot of GCCism. It mostly work with
>>> Clang because they have adopted some of them.
>>>
>>> I would be happy to revert the condition, but then AFAICT there are no pretty
>>> way to now that we are using GCC. While the define __GNUC__ is meant to tell
>>> you this is compiled with GCC, clang is also defining it.
>>
>> That's horrible, I didn't know about that!
>>
>>
>>> So the condition would have to be
>>>
>>> #if !defined(__clang__) && defined(__GNUC__)
>>
>> :-(
>>
>>
>>> But then if clang is already defining __GNUC__, what actually prevents any
>>> other to do it?
>>>
>>> I have yet to see anyone wanted to build Xen with another compiler other than
>>> clang and GCC. So I will leave this patch as is. Feel free to suggest a
>>> different approach if you are not happy with this.
>>
>> Is there a __REALLY_REALLY_GUNC__ variable? I guess not, so I don't have
>> a better suggestion. This problem is quite annoying (not your fault of
>> course) I wonder how other projects deal with it. There must be a
>> "clean" way to distinguish gcc from others?
>>
>> For now, I am OK with this patch as is because I wouldn't know what else
>> to suggest, and I agree that !defined(__clang__) && defined(__GNUC__) is
>> bad.
> 
> and you can add:
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>

Thank you. I have also updated the commit message to reflect the discussion:


     xen/arm: fix get_cpu_info() when built with clang

     Clang understands the GCCism in use here, but still complains that sp is
     unitialised. In such cases, resort to the older versions of this code,
     which directly read sp into the temporary variable.

     Note that GCCism is still kept in default because other compilers (e.g.
     clang) may also define __GNUC__, so AFAIK there are no proper way to
     detect properly GCC.

     This means that in the event Xen is ported to a new compiler, the code
     will need to be updated. But that likely not going to be the only place
     where Xen will need to be adapted...

     This is based on the x86 counterpart.

Cheers,

Patch

diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
index c4af66fbb9..6b7c1df64d 100644
--- a/xen/include/asm-arm/current.h
+++ b/xen/include/asm-arm/current.h
@@ -28,8 +28,16 @@  struct cpu_info {
 
 static inline struct cpu_info *get_cpu_info(void)
 {
+#ifdef __clang__
+    unsigned long sp;
+
+    asm ("mov %0, sp" : "=r" (sp));
+#else
     register unsigned long sp asm ("sp");
-    return (struct cpu_info *)((sp & ~(STACK_SIZE - 1)) + STACK_SIZE - sizeof(struct cpu_info));
+#endif
+
+    return (struct cpu_info *)((sp & ~(STACK_SIZE - 1)) +
+                               STACK_SIZE - sizeof(struct cpu_info));
 }
 
 #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)