Re: [Xen-devel] [PATCH v9 2/5] xen: introduce SYMBOLS_SUBTRACT and SYMBOLS_COMPARE

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Jan Beulich writes ("Re: [Xen-devel] [PATCH v9 2/5] xen: introduce 
SYMBOLS_SUBTRACT and SYMBOLS_COMPARE"):
> On 12.02.19 at 13:01, <ian.jackson@xxxxxxxxxx> wrote:
> > I would particularly welcome the opinion of hypervisor maintainers on
> > my type safety point, below.
> 
> I agree with the requirements you put forward; I think I'd
> prefer the inline function versions I had suggested (or
> something similar) over macros though, not the least because
> they come with "built-in" type safety, rather than grafted one
> (by adding "pseudo" comparisons).

I didn't see your proposed inline function, but don't think it can
work correctly because it won't be type-generic.  Ie, the requirement
is to use the same `SYMBOLS_DIFFERENCE(p, _foo_end)' for various
different `struct foo *p'.  p is perhaps of different types in the
different invocations and the return value needs to have been divided
by sizeof(*p).


I hate to suggest this now, but as an alternative it would be possible
to do this:

 /*
  *    T *ALTER_PROVENANCE(T *value, T *provenance);
  *
  *    Returns a pointer with the value of `value' but the provenance
  *    of `provenance'; that is, which is considered by the compiler
  *    to be part of same object as `provenance' and not (necessarily)
  *    part of the same object as `value'.
  *
  *    `value' MUST be within whatever the compiler thinks the size
  *    of `provenance' is (if the compiler has any way to know the
  *    size of `provenance').  That is, `value' must be one that can
  *    legally be constructed by indexing within `provenance'.
  *
  *    `value' and `provenance' are evaluated only once each.
  */
 #define ALTER_PROVENANCE(value,provenance) ({
      (void)( typeof((value))0      == (void*)0           );
      (void)( typeof((provenance))0 == (void*)0           );
      (void)( typeof((provenance))0 == typeof((value))0   );
      typeof((provenance)) const alter_provenance__copy = (provenance);
      (typeof((provenance)))(
         (char*)(alter_provenance__copy) +
         ( (uintptr_t)(char*)(value) -
           (uintptr_t)(char*)(alter_provenance__copy) )
      )
 })

(\ omitted for clarity).

But you would still want _foo_end to have a different type to
_foo_start so you would have to wrap ALTER_PROVENANCE in another
macro.


> > Secondly, I find the argument ordering extremely confusing.  With your
> > macros DIFFERENCE(start,end) is negative but COMPARE(start,end) is
> > positive.  I suggest that you call the macro DIFFERENCE and have
> > DIFFERENCE(start,end) be positive.
> 
> Indeed having to put end before start in either macro invocation
> is prone to be got wrong. In the common case this will be noticed
> quickly, but even then it's likely one extra compile and test run to
> notice that there's something wrong.
> 
> However, I realize this is to keep use sites look more like
> "end - start", which has its merits as well.

Yes.  The problem is that with the `-' disappearing, the need to swap
the arguments (as is needed with `-') is much less evident.


> Furthermore - do we really need both a subtract and a compare
> construct? The result subtract produces can be used for
> comparison purposes as well, after all (just like all CPUs I know
> details of implement [integer] compare as a subtract discarding
> its numeric result, instead [or only] updating certain status flags).

I think we only need one macro construct to deal with comparisons with
_foo_end.

The other cases seem very few and don't fall into a convenient pattern
anyway.

Ian.

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