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

[Ian Jackson <ian.jackson@citrix.com>]

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.)

> +/*
> + * 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.  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);
  })

  /*
   * 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.

> +/*
> + * 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.

> and:
> 
> +    for ( alt = region->begin;
> +          SYMBOLS_COMPARE(alt, region->end) < 0;
> +          alt++ )

region->begin and ->end aren't linker symbols, are they ?  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.

Ian.

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