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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 13 Feb 2019, Jan Beulich wrote:
> >>> On 13.02.19 at 02:17, <sstabellini@xxxxxxxxxx> wrote:
> > 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[]"
> 
> These are declared types. Effective type when used as rvalue
> (i.e. also when passed as arguments to functions) is struct
> wombat * in both cases.

I see..


> The important aspect (and new idea) Ian has been introducing
> really is that the "end" symbols intentionally no longer be of
> the same type as the start ones (but some type derived
> thereof).

I missed that part of his proposal. Reading back your suggested static
inline functions and WHATEVER macro I can see that it works. But I don't
understand why is it desirable to have the "end" symbols intentionally
no longer be of the same type as the start ones.

Also, I would ask to consinder the impact of WHATEVER on the code versus
the var.S approach, which at least has the benefit of being simpler.


> > 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!
> 
> I've indeed been considering this already, as I expected the point
> would come up sooner or later. Thing is though that, in particular
> with Adrian not having replied at all so far, I'm still unconvinced that
> we need to make this many changes (i.e. other than to work around
> compiler deficiencies, which would boil down to using SYMBOL_HIDE()
> from v7, but only in places where it is known certain compiler versions
> might mis-optimize it, and with a clear reference to the involved
> compiler bug/versions) at all. It's just that what we're now discussing
> is the approach I have the least problem following _if_ such a global
> "marking" of linker symbol uses ends up being necessary.

Thank you for taking this into consideration, I really appreciate it!
When/If we decide that we need a global "marking", and we also want
"fancy" type-checking, I would really appreciate your help.


> >> 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.
> 
> Except that, as I think Ian has also suggested, DIFFERENCE() (or
> SYMBOL_DIFFERENCE()) might be better, as it (hopefully)
> reduces the connections to the - operator, and hence the risk
> of possibly getting the argument order wrong. Otoh with the
> type safety added wrong argument order would cause a
> compile time error.

I don't mind either way. The good thing about the way is done in this
series is that all the comparison signs (<, >, <=, >=, etc) didn't have
to be changed. It makes it much easier to review and check it's correct.

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