Re: [Xen-devel] [PATCH v6 1/4] xen: introduce SYMBOL

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.01.19 at 00:42, <sstabellini@xxxxxxxxxx> wrote:
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -99,6 +99,16 @@
>      __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
>      (typeof(ptr)) (__ptr + (off)); })
>  
> +/*
> + * Similar to RELOC_HIDE, but written to be used with symbols such as
> + * _stext and _etext to avoid undefined behavior comparing pointers to
> + * different objects. It can handle array types.
> + */
> +#define SYMBOL(ptr)                               \
> +  ({ unsigned long __ptr;                       \
> +    __asm__ ("" : "=r"(__ptr) : "0"(ptr));      \
> +    (typeof(*(ptr)) *) (__ptr); })

I'm sorry for thinking of this only now, but SYMBOL() is (no longer)
appropriate as a name here. I'd like to suggest SYMBOL_HIDE(), in
line with the other macro's name. With that and with
- the stray blank after the cast dropped
- the asm() formatting corrected; there are a number of blanks
  missing
- the name of the local variable corrected as per my original
  suggestion
- indentation corrected
- the use of underscores on "asm" and "typeof" brought in line
you may (re-)add
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Furthermore I wonder why the cast is needed in the first place.
Doesn't

#define SYMBOL_HIDE(ptr) ({                   \
    __typeof__(*(ptr)) *ptr_;                 \
    __asm__ ( "" : "=r" (ptr_) : "0" (ptr) ); \
    ptr_; \
})

do the job as well?

Jan



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