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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2019 10:55, Jan Beulich wrote:
> 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.

And indeed - on my box with gcc 4.3, a full fresh build fails with
two duplicate symbols. Roger's workaround therefore is not enough
in any event for 4.13, unless we want to - in the last minute -
raise the bar of what gcc versions we claim compatibility with.

Jan

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