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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.11.2019 19:17, Andrew Cooper wrote:
> On 21/11/2019 08:34, Jan Beulich wrote:
>> On 20.11.2019 18:13, Andrew Cooper wrote:
>>> On 20/11/2019 16:40, Jürgen Groß wrote:
>>>> On 20.11.19 17:30, Jan Beulich wrote:
>>>>> On 08.11.2019 12:18, Jan Beulich wrote:
>>>>>> The .file assembler directives generated by the compiler do not include
>>>>>> any path components (gcc) or just the ones specified on the command
>>>>>> line
>>>>>> (clang, at least version 5), and hence multiple identically named
>>>>>> source
>>>>>> files (in different directories) may produce identically named static
>>>>>> symbols (in their kallsyms representation). The binary diffing
>>>>>> algorithm
>>>>>> used by xen-livepatch, however, depends on having unique symbols.
>>>>>>
>>>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>>>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>>>>> that this build option is made no longer depend on LIVEPATCH, but
>>>>>> merely
>>>>>> defaults to its setting now.
>>>>>>
>>>>>> Conditionalize explicit .file directive insertion in C files where it
>>>>>> exists just to disambiguate names in a less generic manner; note that
>>>>>> at the same time the redundant emission of STT_FILE symbols gets
>>>>>> suppressed for clang. Assembler files as well as multiply compiled C
>>>>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>>>>
>>>>>> Since we now expect there not to be any duplicates anymore, also don't
>>>>>> force the selection of the option to 'n' anymore in allrandom.config.
>>>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>>>>> enforcement is in effect, which in turn allows
>>>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>>>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>> I've got acks from Konrad and Wei, but still need an x86 and a release
>>>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>>>>> this go in anymore?
>>>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
>>>> Release-ack, but I'm hesitant to take it later.
>>> 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.

> This is a change to fix a concrete livepatch issue with Clang.  Sure -
> it resolves the symbol uniqueness failures for the in-tree build, but
> considering the risks to the area you are modifying, the fact you
> haven't even done a dev test of a livepatch build on GCC means that the
> patch as a whole has not had what I would consider a reasonable amount
> of testing.

While Clang is the primary area where we need this change, it is
in no way limited to that environment. Gcc can, at any time,
start triggering the issue again as well - both because of changes
to the compiler itself, or because of (perhaps seemingly innocent)
changes we do to Xen. I can't imagine the workaround for this
would be to make it impossible altogether to select LIVEPATCH=y.
(In fact some of the recent always_inline may want dropping again
as well with this change in place, when they were added for
symbol collision reasons rather than code generation ones.)

> Luckily for you, Ross and Sergey have agreed to smoke test this with
> some livepatches.  They will report on this thread with their findings.

I appreciate this. And I do not consider it my responsibility to
do regression tests of the live patching tools. 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.

Jan

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