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

[Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>]

On 27/11/2019 18: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.
> 
> 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.
> 
> Luckily for you, Ross and Sergey have agreed to smoke test this with
> some livepatches.  They will report on this thread with their findings.

Applying the patch didn't end up well for my test LP (from another thread):

Perform full initial build with 8 CPU(s)...
Reading special section data
Apply patch and build with 8 CPU(s)...
Unapply patch and build with 8 CPU(s)...
Extracting new and modified ELF sections...
Processing xen/arch/x86/mm/shadow/guest_2.o
Processing xen/arch/x86/mm/shadow/guest_4.o
Processing xen/arch/x86/mm/shadow/guest_3.o
Processing xen/arch/x86/mm/guest_walk_3.o
Processing xen/arch/x86/mm/hap/guest_walk_3level.o
Processing xen/arch/x86/mm/hap/guest_walk_4level.o
Processing xen/arch/x86/mm/hap/guest_walk_2level.o
Processing xen/arch/x86/mm/guest_walk_2.o
Processing xen/arch/x86/mm/guest_walk_4.o
Processing xen/arch/x86/efi/efi/check.o
Processing xen/arch/x86/pv/gpr_switch.o
Processing xen/arch/x86/indirect-thunk.o
Processing xen/arch/x86/boot/head.o
Processing xen/arch/x86/x86_64/kexec_reloc.o
Processing xen/arch/x86/x86_64/compat/entry.o
Processing xen/arch/x86/x86_64/entry.o
Processing xen/arch/x86/hvm/vmx/entry.o
Processing xen/arch/x86/hvm/svm/entry.o
Processing 
xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0s.o
Processing 
xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0r.o
Processing 
xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1s.o
Processing 
xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1r.o
ERROR: no functional changes found.

So this looks like a regression.

--
Thanks,
Sergey


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