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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 7 Feb 2019, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v6 1/4] xen: introduce 
> SYMBOL"):
> > I am OK with this approach. Maybe not the best IMO, but good enough. It
> > should also satisfy the MISRAC guys, as they wrote "ideally cast to
> > uintptr_t only once": here we wouldn't be casting only once, but at
> > least we would do it inside a single well-defined macro.
> 
> Right.  I think it meets the goals of MISRA-C, probably better than
> most other approaches.
> 
> FAOD, I think you should expect people to declare the linker symbols
> either as I suggested:
> 
>      extern const struct wombat _wombats_start[];
>      extern const struct abstract_symbol _wombats_end[];
> 
> (or along the lines of Jan's suggestion, but frankly I think that is
> going to be too hard to sort out now.)

Yes, they are already declared this way, I would prefer to avoid
changing the declaration as part of this series.


> > +/*
> > + * Performs x - y, returns the original pointer type. To be used when
> > + * either x or y or both are linker symbols.
> > + */
> > +#define SYMBOLS_SUBTRACT(x, y) ({                                          
> >    \
> > +    __typeof__(*(y)) *ptr_;                                                
> >    \
> > +    ptr_ = (typeof(ptr_)) (((uintptr_t)(x) - (uintptr_t)(y)) / 
> > sizeof(*(y))); \
> > +    ptr_;                                                                  
> >    \
> > +})
> 
> This is type-incoherent.  The difference between two pointers is a
> scalar, not another pointer.

I am glad you highlighted this. The vast majority of changes in this
series are subtractions or comparisons.  So, if subtractions (and also
comparisons as you wrote below) need to return a scalar, then we might
as well return uintptr_t or ptrdiff_t from the two macros. It makes a
lot of sense to me.


> Also "the original pointer type" is
> ambiguous.  It should refer explicitly to y.  IMO this function should
> contain a typecheck which assures that x is of the right type.
> 
> How about something like this:
> 
>   /*
>    * Calculate (end - start), where start and end are linker symbols,
>    * giving a ptrdiff_t.  The size is in units of start's referent.
>    * end must be a `struct abstract_symbol*'.
>    */
>   #define SYMBOLS_ARRAY_LEN(start,end) ({
>      ((end) == (struct abstract_symbol*)0);                               
>      (ptrdiff_t)((uintptr_t)(end) - (uintptr_t)(start)) / sizeof(*start);
>   })

Sounds good, but the issue is that we might have to use this macro with:

- start is a linker symbol and end as a normal pointer
- start is a normal pointer and end as a linker symbol
- both are linker symbols

If so, do we need three slightly different variations of this macro?


>   /*
>    * Given two pointers A,B of arbitrary types, gives the difference
>    * B-A in bytes.  Can be used for comparisons:
>    *   If A<B, gives a negative number
>    *   if A==B, gives zero
>    *   If A>B, gives a positive number
>    * Legal even if the pointers are to different objects.
>    */
>   #define POINTER_CMP(a,b) ({
>      ((a) == (void*)0);
>      ((b) == (void*)0);
>      (ptrdiff_t)((uintptr_t)(end) - (uintptr_t)(start));
>   })
> 
> The application of these two your two examples is complex because your
> examples seem wrong to me.

Yeah, I realize it wasn't really possible to understand my examples
unless one was very familiar with past versions of the series. I'll add
more context below.


> > +/*
> > + * Performs x - y, returns uintptr_t. To be used when either x or y or
> 
> This is wrong.  Comparisons should give a signed output.
> 
> > + * both are linker symbols.
> 
> In neither of your example below are the things in question linker
> symbols so your examples violate your own preconditions...
> 
> 
> > Examples:
> > 
> > +    new_ptr = SYMBOLS_SUBTRACT(func->old_addr, _start) + vmap_of_xen_text;
> 
> This is punning wildly between pointers and integers.  I infer that
> old_addr is a pointer of some kind and vmap_of_xen_text is an integer.
> I also infer that sizeof(*old_addr) is 1 because otherwise you
> multiply vmap_of_xen_text by the size which is clearly entirely wrong.
> Ie this code is just entirely wrong.
> 
> This is presumably some kind of relocation.  I don't think it makes
> much sense to macro this.  Instead, it is better to make
> vmap_of_xen_text a pointer and do this:
> 
>   +    /* Relocation.  We need to calculate the offset of the address
>   +     * from _start, and apply that to our own map, to find where we
>   +     * have this mapped.  Doing these kind of games directly with
>   +     * pointers is contrary to the C rules for what pointers may be
>   +     * compared and computed.  So we do the offset calculation with
>   +     * integers, which is always legal.  The subsequent addition of
>   +     * the offset to the vmap_of_xen_text pointer is legal because
>   +     * the computed pointer is indeed a valid part of the object
>   +     * referred to by vmap_of_xen_text - namely, the byte array
>   +     * of our mapping of the Xen text. */
>   +    new_ptr = ((uintptr_t)func->old_addr - (uintptr_t)_start) + 
> vmap_of_xen_text;
> 
> Note that, unfortunately, any deviation from completely normal pointer
> handling *must* be accompanied by this kind of a proof, to explain why
> it is OK.

OK. Most of the call sites only do things like (_end - _start) or (p >
_end). I wanted to bring up cases that are not trivial.

We have a couple of cases where we are "punning wildly between pointers
and integers", for instance:

xen/arch/arm/arm64/livepatch.c:arch_livepatch_apply
xen/arch/arm/setup.c:start_xen  line 772
xen/arch/x86/setup.c:__start_xen  line 1382

I think it is OK to manually cast to (uintptr_t) in those cases as you
suggest.


> > and:
> > 
> > +    for ( alt = region->begin;
> > +          SYMBOLS_COMPARE(alt, region->end) < 0;
> > +          alt++ )
> 
> region->begin and ->end aren't linker symbols, are they ?

I made this example because this is a common pattern that we have in the
hypervisor. A better example using your suggested macro would be:

+    for ( call = __initcall_start;
+          POINTER_CMP(call, __presmp_initcall_end) < 0;
+          call++ )
 
Where __initcall_start and __presmp_initcall_end are linker symbols.
(Above region->begin and region->end are initialized to two linker
symbols.)


> So the
> wrong assumption by the compiler (which is at the root of this thread)
> that different linker symbols are necessarily different objects
> (resulting from the need to declare them in C as if they were) does
> not arise.  I think you mean maybe something like _region_start and
> _region_end.  So with my proposed macro:
> 
> > We could also define a third macro such as:
> >   #define SYMBOLS_SUBTRACT_INT(x, y)  SYMBOLS_COMPARE((x), (y))
> > because we have many places where we need the result of SYMBOLS_SUBTRACT
> > converted to an integer type. For instance:
> >   paddr_t xen_size = (uintptr_t)SYMBOLS_SUBTRAC(_end, _start);
> 
> This need arises because the difference between two pointers is indeed
> an integer and not another pointer.

Yes, I get it.

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