Re: [Xen-devel] [PATCH v11 3/9] xen: introduce DECLARE_BOUNDS

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.19 at 23:38, <sstabellini@xxxxxxxxxx> wrote:
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -173,4 +173,92 @@ void init_constructors(void);
>  void *bsearch(const void *key, const void *base, size_t num, size_t size,
>                int (*cmp)(const void *key, const void *elt));
>  
> + /*
> +  * Declare start and end array variables in C corresponding to existing
> +  * linker symbols.
> +  *
> +  * These macros, or an alternative technique, MUST be used any time
> +  * linker symbols are imported into C via the `extern []' idiom.
> +  *
> +  *    __DECLARE_BOUNDS(TYPE, START, START, END)

START used twice here makes it ambiguous which one is which in the
subsequent text.

> +  *  introduces the following two constant expressions
> +  *
> +  *    const TYPE *START;
> +  *    const struct abstract_NAME *END;

For one these are declarations, not (constant) expressions. And
then the declarations produce array types, not pointer types.
Please let's not have a comment which is out of sync with what
it describes from the very beginning.

> +  *  whose values are the linker symbols START and END; these
> +  *  should be the start and end of a memory region.
> +  *
> +  *  You may then use these two inline functions:
> +  *
> +  *    bool NAME_lt(const TYPE *s1, const struct abstract_NAME *s2);
> +  *    ptrdiff_t NAME_diff(const TYPE *s1, const struct abstract_NAME *s2);
> +  *
> +  *  lt returns true iff s1 < s2.
> +  *  diff returns the s2-s1 in units of TYPE.
> +  *
> +  *

Stray double blank comment lines and no mention of _bytediff.

> +static inline ptrdiff_t name ## _diff(const type s1[],                       
>  \
> +                                      const struct abstract_ ## name s2[])   
>  \
> +{                                                                            
>  \
> +    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                              
>  \
> +    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1) /                      
>  \
> +           (ptrdiff_t)sizeof(*s1);                                           
>   \
> +}                                                                           

I had specifically asked for this to simply call _bytediff, to limit
redundancy and in particular the total number of casts.

> +static inline ptrdiff_t name ## _bytediff(const type s1[],                   
>  \
> +                                          const struct abstract_ ## name 
> s2[])\
> +{                                                                            
>  \
> +    BUILD_BUG_ON(alignof(*s1) != alignof(*s2));                              
>  \
> +    return (ptrdiff_t)((uintptr_t)s2 - (uintptr_t)s1);                       
>  \
> +}

What's the value of the intermediate casting to uintptr_t? Why not
cast to ptrdiff_t right away?

I also don't think the BUILD_BUG_ON() is helpful in this latter case.

Jan



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