Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/11/2019 10:17, George Dunlap wrote:
>> On Nov 28, 2019, at 9:55 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> Has anyone actually tried building a livepatch with this change in place?
>>>> Actually - what is your concern here? The exact spelling of symbols
>>>> names should be of no interest to the tool. After all the compiler is
>>>> free to invent all sorts of names for its local symbols, including
>>>> the ones we would produce with this change in place. All the tool
>>>> cares about is that the names be unambiguous. Hence any (theoretical)
>>>> regression here would be a bug in the tools, which imo is no reason
>>>> to delay this change any further. (Granted I should have got to it
>>>> earlier, but it had been continuing to get deferred.)
>>> This might all be true (theoretically).
>>>
>>> The livepatch build tools are fragile and very sensitive to how the
>>> object files are laid out.  There is a very real risk that this change
>>> accidentally breaks livepatching totally, even on GCC builds.
>>>
>>> Were this to happen, it would be yet another 4.13 regression.
>> It's perhaps a matter of perception, but I'd still call this a
>> live patching tools bug then, not a 4.13 regression.
> After the discussion yesterday, I was thinking a bit more about this, and I’m 
> not happy with the principle Andy seems to be operating on,

I'm sorry that you feel that way.

> that it’s reasonable to completely block a bug-fixing patch to Xen, forcing a 
> work-around to be used in a release, because it has unknown effects on 
> livepatching.

This is not a fair characterisation of what is going on here.  Ignore
the specifics of this patch - they are not relevant to my objection.

As a maintainer, it is my responsibility to ensure that crap doesn't get
committed.

As a consequence, it is up to me to judge whether I believe that the
submitter of a patch has provided adequate thought/testing to what they
are changing.  Mostly this is judged on comments provided (or usually,
their absence), weighed up against the risk of what it might be likely
to break.

In the case that I don't believe adequate through/testing/etc has been
done, I'm not going to ack the patch.  I'd be failing as a maintainer if
I did.

Ergo, I am not inclined to change my position.


In this case, all I asked was "has anyone done a livepatch build?"

I'd be entirely happy with a reply of "yes [I or someone else did] and
it seems ok".

I'd even be happy with "There does seem to be an issue with
livepatching, but I've engaged the relevant people in this other thread".

What I'm not happy with is "I haven't even done a single build to see
whether it might have problems", and what is definitely not acceptable
is, and I quote:

> And I do not consider it my responsibility to
> do regression tests of the live patching tools.

Yes.  Yes it really is, when you're making a material change
specifically to this area, with a high chance of adverse impact.

I don't expect you necessarily to fix the issue, but I do expect you to
have some idea of whether you're trading one 4.13 blocker for a
different one.

>>  If they're so
>> extremely fragile, then I think this needs urgently taking care of
>> by their maintainers. As mentioned before - how exactly static
>> symbols get represented is up to the compiler, i.e. may change at
>> any time. As a result, any change whatsoever would need such
>> regression testing, no matter that I agree that a larger scope
>> change like this one has slightly higher potential of triggering
>> some issue.
> This is another argument I would agree with.
>
> Given the closeness to the release, I’d favor checking in the patch today or 
> tomorrow, regardless of testing status, so that it can get more testing in 
> our automated systems; it can always be reverted if it is shown to break 
> livepatching on gcc.

Oh, and shocker in what is apparently a surprise to everyone but me...

Sergey went and did the work Jan believed to be "not his
responsibility", and yes - this really does break livepatching.

Do I expect Jan to fix it?  No, but I do expect a discussion and an
understanding of the issue before this patch gets anywhere near staging.

~Andrew, quite irritated at the total lack of due diligence being
demonstrated here.

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