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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

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.

~Andrew

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