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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxx>
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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel