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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2019 14:10, Andrew Cooper wrote:
> 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.

While above you say to ignore the specifics of this patch, I'm
afraid I still take "crap" as a qualification of the submitted
patch (and possibly other parts of my work). Perhaps I should
go look for another job if it's like this.

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

Once again - no. If there is a problem with this patch, then
please point it out by review comments. If there is a problem
with the live patching tools, it is there where things need
fixing. Just to remind you of what I'm hearing/seeing you and
others say/do: Quite often issues with the tools arise when
dealing with particular XSAs. That's still no reason to
delay/reject the fixes for those XSAs.

And once again - please accept that views if different people
may vary.

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

Which I'd have been fine with if issues _the patch_ causes had
been pointed out in the form of review comments. But as in so
many other cases, the patch was just left hanging in the air.
Despite Sergey's reply I'm none the wiser what may be wrong, or
where. But I'll reply to that effect there, to keep technical
things separated from this unfortunate discussion.

Jan

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