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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.


> > > This is wrong.  The conversion to ptrdiff_t (currently done implicitly
> > > by return) must come before the division.  Otherwise it will give the
> > > wrong answer when s1 > s2.
> > > 
> > > Suppose 32-bit, s1=0x00000040 s2=0x00000020 sizeof=0x10, Then
> > > s2-s1=0xffffffe0, and unsigned division gives
> > > (s2-s1)/sizeof=0x0ffffffe.  Converstion to ptrdiff_t does not change
> > > the bit pattern.  But we wanted 0xffffffe.
> > > 
> > > Signed integer division by a positive divisor is always defined (and
> > > always fits) so doing the conversion first is fine.
> > 
> > Well, this would come as a side effect if there first was a function
> > producing the byte delta, and then the function here would call
> > that other function, doing the division on the result.
> 
> I don't mind if someone wants to provide a byte delta function.  It
> ought to have a different name to `blah_diff' though.  `blah_bytediff'
> maybe.
> 
> > There's another caveat here though: Even by doing the cast first,
> > the division will still be unsigned as long as the sizeof() doesn't also
> > get cast to ptrdiff_t.
> 
> Yes, the sizeof would have to be cast to ptrdiff_t too.

OK, it makes sense.

> > One question though is whether we actually care about the case
> > when s1 > s2 in the first place. But perhaps it's better to consider
> > it right away than getting bitten later on.
> 
> Having a thing which silently goes wrong giving bizarre and large
> answers is clearly not acceptable...
 
Right

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