| Message ID | 20190327184531.30986-3-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Add support to build with clang | expand |
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
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,
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.
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,
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.
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>
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,
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,
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)
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(-)