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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.19 at 15:56, <dunlapg@xxxxxxxxx> wrote:
> On Mon, Feb 4, 2019 at 9:37 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >>> On 01.02.19 at 19:52, <dunlapg@xxxxxxxxx> wrote:
>> What I'm not sure I see is what you mean to
>> express with all you wrote in terms of finding a way out of the
>> current situation (besides requesting a vote)
> 
> If you're just tired of this discussion and want it to be done, then
> of course we can just take a vote.
> 
> But ideally I think votes are best when everyone sees the landscape of
> the decision clearly, and agrees on exactly what it is they disagree
> about. Furthermore, it seems to me from reading this discussion that
> it's more than just a few specific examples that I disagree with you
> about, but about a number of principles; as such, investing time
> trying to come to a common understanding should pay dividends in the
> form of reduced friction in the future.

Which I appreciate and agree with. I've been mentioning the option
of a vote solely to show a way to make forward progress without
us reaching agreement.

> Before I expressed an opinion, I wanted to make sure that I hadn't
> misunderstood you or missed a big aspect of the discussion.
> 
>> > Improvements this series seeks to make, as I understand it, include
>> > (in order of urgency):
>> >
>> > 1. Fixing one concrete instance of "UB hazard"
>>
>> Right, and we want to work around compiler bugs here.
>>
>> > 2. Minimize risk of further "UB hazard" in this bit of functionality
>> > 3. Retain the effort Stefano has put in identifying all the places
>> > where such UB hazards need to be addressed.
>> > 4. Move towards MISRA-C compliance.
>> >
>> > As far as I can tell, primary objections you've leveled at the options
>> > which try to address 2-4 are:
>> >
>> > a. "UB hazard" still not zero
>> > b. MISRA compliancy no 100%
>> > c. Ugly
>> > d. Inefficient
>> >
>> > (Obviously some proposals have had more technical discussion.)
>> >
>> > Anything I missed?
>>
>> I don't think so, especially since various aspects can fall under "ugly"
>> and/or "inefficient".
> 
> Well for instance, other objections that you might have made that I
> don't include in those include:
> 
> * Incorrect (i.e., known ways in which the patch will break functionality)
> * Misleading / confusing (i.e., someone modifying it is likely to
> introduce regressions)
> * Fragile (i.e., likely to break due to small or unrelated changes).

There are none of the first category that I'm aware of, but to me
the latter two categories at least overlap with "inefficient", and
to me especially the var.S approach falls in the fragile and maybe
also in the misleading/confusing category.

> [snip]
>>: Improving on a. and
>> b. is not a good excuse to extend c., at least not unequivocally.
>> Whether d. actually matters is a separate aspect, partly because it
>> may mean different things (it could e.g. be taken as another
>> wording for a. and b.).
> 
> I take it you mean 2 and 4 (reduced UB hazard and increased chance of
> MISRA-C compliance) are not a good excuse for c (ugly).
> 
> The "ugliness" here involves, variously:
> * Passing a variable through an asm "barrier"
> * Casing pointers to other types, sometimes multiple times at once
> 
> Most of them are a handful of lines hidden behind a macro in a header file.
> 
> To me, on a scale of 1 to 10, I'd give them an ugliness factor 2 or 3.
> 
> On the other hand, I find 2-4 compelling:
> 
> * I consider your suggested approach, of using simple
> pointer-to-pointer casting,  or casting to a pointer after asm and
> comparing the resulting pointers, to have a reasonably high chance of
> tripping over UB behavior at some point in the future.  Regardless of
> the outcome of that -- whether we change our work-around again or
> whether the compiler authors change the compilers -- both we and our
> users and customers will have had a lot of hassle to deal with.
> Avoiding that hassle is worth the slight ugliness introduced by the
> other solutions.
> 
> * Stefano has done a fair amount of work identifying the places that
> need to be changed.  We know that we're likely to need to make *some
> sort* of change like this for MISRA compliance at some point.
> Throwing away work that then will need to be duplicated is both a
> waste of time, and of developer motivation.  Even if we didn't think
> it would impove UB behavior *or* get us closer to MISRA C compliance,
> retaining the work he's done would be worth accepting a patch creating
> such a macro.
> 
> * The patch takes the code base one step closer to being MISRA C
> compliant, by setting up infrastructure likely needed by whatever it
> needs.  Even before we had the recommendation from MISRA C, I would
> consider preparing for that eventuality to be worth the minor ugliness
> introduced.
> 
> And so, to me, the unitptr_t casting proposal seems like an obvious 
> "accept".
> 
> Do you disagree with any of my assessments above?  Did I miss anything
> that should be factored in?

Well, I think the picture you've given isn't complete. For one, I'm
certainly willing to accept a certain level of ugliness (or even
fragility, which I consider even more problematic) if the proposed
adjustments indeed _guarantee_ an improvement. But so far I've
not seen any proof of this (and your explanations of why you
find "2-4 compelling" also doesn't seem to add any). Of course one
might make changes just in the hope of an improvement, but then
my personal tolerance to it (potentially) having undesirable side
effects goes down.

Furthermore, as with any other change, I think it is a fair
expectation that it be made clear what improvements will result.
As by this point I remain unconvinced that any change is needed
at all (other than to work around compiler bugs), it is - I think -
clear that I'm not seeing the supposed improvements as actual
ones.

So to evaluate the proposals in these terms:

- the var.S approach simply is ugliest among all of them (which
  I admit is a subjective assessment of mine), but gives the
   highest level of "hope" of being compliant
- the cast to integer as well as the var.S approach are more
  fragile than the one retaining (or to be precise, re-establishing)
  original types
- the cast-retaining-types approach is the least fragile one,
  but also the one delivering the least level of "hope"

On the balance, i.e. weighing upsides and downsides, I would
probably come to a zero for all of them, which is the same as
simply not changing anything. Which is why I continue to think
that, again besides dealing with known (but not "predicted" or
whatever you might call it) compiler bugs, leaving the code as
is will be the best choice at this point in time.

Jan


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