Re: [Xen-devel] [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.02.19 at 01:05, <sstabellini@xxxxxxxxxx> wrote:
> On Wed, 27 Feb 2019, Jan Beulich wrote:
>> >>> On 26.02.19 at 19:43, <sstabellini@xxxxxxxxxx> wrote:
>> > On Tue, 26 Feb 2019, Ian Jackson wrote:
>> >> Jan Beulich writes ("Re: [PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
>> >> > On 26.02.19 at 17:46, <ian.jackson@xxxxxxxxxx> wrote:
>> >> > > I am not aware of a standard C type which could be used instead of
>> >> > > this struct.  But I think you can use the `packed' attribute to get
>> >> > > the right behaviour.  The GCC manual says:
>> >> > > 
>> >> > >  | Alignment can be decreased by specifying the 'packed' attribute.
>> >> > >  | See below.
>> >> ...
>> >> > Until I've looked at this (again) now, I wasn't even aware that
>> >> > one can combine packed and aligned attributes in a sensible
>> >> > way. May I suggest that, because of this being a theoretical
>> >> > issue only at this point, we limit ourselves to the build time
>> >> > assertion you suggest?
>> >> 
>> >> I am not suggesting combining `packed' and `aligned'.  I am suggesting
>> >> only `packed' (but based on text which is in the manual section for
>> >> `aligned').  But I am happy with a build-time assertion if you don't
>> >> want to add `packed'.  That is just as safe.
>> > 
>> > Could you please provide a rough example of the build-time assertion you
>> > are thinking about? I am happy to add it.
>> 
>> BUILD_BUG_ON(alignof(*s1) != alignof(*s2));
> 
> Thanks! I noticed that BUILD_BUG_ON requires xen/lib.h which cannot be
> included from xen/compiler.h. Actually, we were already erroneously
> using BUILD_BUG_ON_ZERO in xen/compiler.h without including xen/lib.h.
> 
> My suggestion would be to move the definitions of BUG_ON, WARN_ON,
> BUILD_BUG_ON and BUILD_BUG_ON_ZERO from xen/lib.h to compiler.h.
> Everything works fine if I do that, and it seems a better fit.
> 
> Are you OK with this?

Better fit or not, if it becomes a requirement, such movement ought to
be okay. However, why would you use these in compiler.h? The new
macro you define doesn't belong there (as it has nothing to do with
how we interface with the compiler) - did I overlook something there?
xen/lib.h would actually seem to be one sensible place to put it; there
may be other sensible candidates.

Jan



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