Re: [Xen-devel] [PATCH 3/4] xen: introduce __symbol

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 16 Oct 2018, Andrew Cooper wrote:
> On 16/10/18 02:34, Stefano Stabellini wrote:
> > On Mon, 15 Oct 2018, Julien Grall wrote:
> >> Hi,
> >>
> >> Please use scripts/get_maintainers.pl to CC relevant maintainers.
> >>
> >> On 15/10/2018 10:56, Stefano Stabellini wrote:
> >>> Introduce a macro, __symbol, which is a simple wrapper around RELOC_HIDE
> >>> to be used everywhere symbols such as _stext and _etext are used in the
> >>> code.
> >>>
> >>> RELOC_HIDE is needed when accessing symbols such as _stext and _etext
> >>> because the C standard forbids comparisons between pointers pointing to
> >>> different objects. _stext, _etext, etc. are all pointers to different
> >>> objects from ANCI C point of view.
> >>>
> >>> To work around potential C compiler issues (which have actually
> >>> been found, see the comment on top of RELOC_HIDE in Linux), and to help
> >>> with certifications, let's introduce some syntactic sugar to be used in
> >>> following patches.
> >>>
> >>> Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> >>> ---
> >>>   xen/include/asm-arm/mm.h   | 6 ++++++
> >>>   xen/include/asm-x86/page.h | 6 ++++++
> >>>   2 files changed, 12 insertions(+)
> >>>
> >>> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> >>> index 940b74b..02ce05a 100644
> >>> --- a/xen/include/asm-arm/mm.h
> >>> +++ b/xen/include/asm-arm/mm.h
> >>> @@ -284,6 +284,12 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va,
> >>> paddr_t *pa,
> >>>   #define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
> >>>     /*
> >>> + * Use RELOC_HIDE with symbols such as _stext and _etext to avoid errors
> >>> + * on comparing pointers to different objects
> >>> + */
> >>> +#define __symbol(x)         ((char *)RELOC_HIDE((unsigned long)(x), 0))
> >> There are no different between arm and x86. A better place would be
> >> xen/compiler.h (or something common).
> > OK
> >
> >
> >> But, after this patch, there are even more chance the compiler will 
> >> consider
> >> that 2 _symbol(...) will come from different objects. So how is this meant 
> >> to
> >> help here?
> > I think the mistake was to cast the return to char*. If I remove the
> > cast, as Andrew suggested, then any comparison would be a comparison
> > between unsigned long, that should be accepted and safe.
> >
> > However, the parameter to RELOC_HIDE has to be casted to unsigned long,
> > because most often we pass char*:
> >
> > /local/repos/xen-upstream/xen/include/xen/compiler.h:100:5: error: cast 
> > specifies array type
> >      (typeof(ptr)) (__ptr + (off)); })
> >
> > So I think the __symbol macro should be:
> >
> > +#define __symbol(x)         (RELOC_HIDE((unsigned long)(x), 0))
> >
> 
> I don't see why the unsigned long cast is needed.  What is wrong by
> having "char *ptr + 0" which is legal pointer arithmetic, and has well
> defined behaviour?

The issue is that _stext is defined as char _stext[], so
__symbol(_stext) wouldn't work because _stext is an array type. As I
pasted above the error is "cast specifies array type". I guess it would
have to be __symbol(&_stext[0]) everywhere.

Also, I don't think it is a good idea for __symbol to return a pointer,
because then we are still left with the original issue of comparing
pointers of different sizes. We just moved the issue from the original
pointers to the ones returned by RELOC_HIDE.


> If it is necessary, then you need a (typeof(x)) cast again on the
> outside, because the unsigned long cast causes RELOC_HIDE's typeof to be
> wrong overall.

I am not sure I follow you here, do you mean:

  ((typeof(x)) RELOC_HIDE((unsigned long)(x), 0)

If so, I think it is best to have RELOC_HIDE return unsigned long (no
outside cast), hence removing all issues of comparing pointers to
different objects. Then, I'll adjust the few call sites that need
changes because of the char* -> unsigned long conversion. I have
already modified patch #4 following this strategy and the modifications
required were very few.

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