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

[George Dunlap <dunlapg@xxxxxxxxx>]

On Mon, Feb 4, 2019 at 9:37 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> On 01.02.19 at 19:52, <dunlapg@xxxxxxxxx> wrote:
>
> I'm not going to reply in detail to all of what you wrote about fanatics,
> but I would like to say that I think compiler people less of that than
> you appear to imply, at least the ones I know. In particular, they can
> be convinced of there being bugs by pointing out what aspect of the
> standard their implementation violates. (Of course there are also
> going to be areas where interpretations of the standard vary too
> much to come to an agreement.)

Right, so I did realize after sending the mail that it was pretty
harsh, and that a compiler person who read it might be angered or hurt
by it.  I'm not sure I would have changed the bulk of it, but I may
have added some caveats to help reframe it.

I spent a chunk of the day Friday reading this thread and all the
references, and was frankly outraged at the kinds of arguments made in
those threads in defense of gcc's behavior.  I'm sure that most
compiler people are nice and friendly in person, and also that the
majority of them are sensible and open to reason.  But that doesn't
change the fact that every couple of years, the OS community has
exactly this sort of interaction -- where the OS is trying to do
something that it absolutely must do, and the compiler people are
telling them that the C spec doesn't allow it, and there's a big long
discussion back and forth, where the conclusion ends up being that the
OS people have to make some crazy ugly work-around.

I spoke hyperbolically to try to make a point, but I stand by the
principles I was advocating: With regard to undefined behavior, we
cannot assume that we'll be safe by following the rules.  Our goal
should be to minimize the risk of tripping over UD behavior at all.

[snip]
> 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.

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

[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?

 -George

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