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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 12 Feb 2019, Jan Beulich wrote:
> >>> 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 don't mind the type checks in principle, I didn't add them to this
version because, as I wrote in a previous email, with have occurrences
of all three this possible calls:

  SYMBOL_COMPARE/SUBTRACT( symbol, symbol )
  SYMBOL_COMPARE/SUBTRACT( symbol, non-symbol )
  SYMBOL_COMPARE/SUBTRACT( non-symbol, symbol )

If you look through the patches you should be able to spot all three.
As you know "non-symbol" and "symbol" are of different type:

  non-symbol would be like a "struct wombat*"
  symbol would be like a "struct wombat[]"

However, it is not possible to have symbol as struct wombar[] and
non-symbol as something entirely different like char*.

So, my question is: do we need three different variations of these
macros for the types checks?


I don't understand from IanJ email whether the suggestion is to change
the type of all the linker symbols. If so, why are we doing this instead
of the var.S approach? If we go and change the type of all the linker
symbols in C-land, this series will become much bigger and at least as
invasive as the var.S approach, but with added weird macros. It is kind
of a lose - lose situation.

Similarly I would prefer to avoid Jan's proposed inline function approach
because we have a few different array types for the linker symbols
(vpci_register_init_t*, struct scheduler *, etc.), it looks far more
work, and I am already waaaay over-budget for this series (as in 700%
over budget). I would be very happy to "gift it" to somebody else
willing to take it over :-)

Seriously, now that all the calls sites are marked appropriately and we
all agree on the compare/subtract macro approach, it wouldn't be hard
for somebody else to jump in and write the macros in their favorite way.
Let me know if you would like to volunteer!




> > Stefano Stabellini writes ("[Xen-devel] [PATCH v9 2/5] xen: introduce 
> > SYMBOLS_SUBTRACT and SYMBOLS_COMPARE"):
> >> +/*
> >> + * Calculate (end - start), where start and/or end are linker symbols,
> >> + * returning a ptrdiff_t. The size is in units of start's referent.
> >> + */
> >> +#define SYMBOLS_SUBTRACT(end, start) ({                                   
> >>     \
> >> +    (ptrdiff_t)((uintptr_t)(end) - (uintptr_t)(start)) / sizeof(*start);  
> >>     \
> >> +})
> > 
> > I'm afraid I have several problems with the details of these macros.
> > 
> > Firstly, I really don't like the lack a type check, which was present
> > in my proposal.  For example, given:
> >      extern struct wombat _foos_start[];
> >      extern char _end[];
> > and your macro definition, the two expressions
> >       SYMBOLS_SUBTRACT(_foos_start, _end)
> >      -SYMBOLS_SUBTRACT(_end, _foos_start)
> > produce different values because one is divided by sizeof(struct
> > wombat) and the other by sizeof(char).  This is hardly desirable.
> > 
> > 
> > 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, I attempted to keep the same ordering as before. I can change it
without too much troubles if that is what we want.


> > Thirdly, in an earlier exchange:
> > [...]
> 
> And fourth, looking at just what's left in context, I see that the
> macro argument uses are incompletely parenthesized.
> 
> 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).

No, we don't. In my first attempt I only had one macro. I am happy to
follow your suggestion and keep only SUBTRACT.

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