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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.19 at 17:46, <ian.jackson@xxxxxxxxxx> wrote:
> Stefano Stabellini writes ("[PATCH v10 2/6] xen: introduce DEFINE_SYMBOL"):
>> +/*
>> + * Declare start and end array variables in C corresponding to existing
>> + * linker symbols.
>> + *
>> + * Two static inline functions are declared to do comparisons and
>> + * subtractions between these variables.
>> + *
>> + * The end variable is declared with a different type to make sure that
>> + * the static inline functions cannot be misused.
>> + */
>> +#define DEFINE_SYMBOL(type, name, start_name, end_name)                   \
>> +                                                                          \
>> +struct abstract_ ## name {                                                \
>> +    type _;                                                               \
>> +};                                                                        \
>> +                                                                          \
>> +extern const type start_name[];                                           \
>> +extern const struct abstract_ ## name end_name[];                         \
> 
> I have thought of a problem with this approach.
> 
> This goes wrong unless `type' is a struct type.  Because the compiler
> is allowed to assume that end_name has the correct alignment for its
> type.  And in some ABIs, the alignment of a struct containing (say) a
> char is bigger than that of a char.  AIUI in some of the actual use
> cases the linker-generated symbols may not be struct aligned.
> 
> 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.
>    
> Bizarrely, this seems only to be stated, slightly elliptically like
> this, in the section on the `aligned' attribute; it's not mentioned in
> `packed'.  I suggest we couple this with a compile-time assertion that
> alignof is the struct is the same as alignof the type.

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?

>> +static inline bool name ## _lt(const type s1[],                           \
>> +                               const struct abstract_ ## name  s2[])       
> \
>> +{                                                                         \
>> +    return (uintptr_t)s1 < (uintptr_t)s2;                                 \
>> +}                                                                         \
> 
> This seems right to me.
> 
>> +static inline ptrdiff_t name ## _diff(const type s1[],                    \
>> +                                      const struct abstract_ ## name s2[])\
>> +{                                                                         \
>> +    return ((uintptr_t)s2 - (uintptr_t)s1) / sizeof(*s1);                 \
> 
> 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.

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.

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.

Jan


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