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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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.

> > 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.

> 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...

Ian.

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