Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 28/10/15 13:25, Jan Beulich wrote:
>>>> On 28.10.15 at 13:55, <andrew.cooper3@citrix.com> wrote:
>> On 26/10/15 11:49, Jan Beulich wrote:
>>> This requires adjustments to the tool generating the symbol table and
>>> its as well as nm's invocation.
>>>
>>> Note: Not warning about duplicate symbols in the EFI case for now, as
>>> a binutils bug causes misnamed file name entries to appear in EFI
>>> binaries' symbol tables when the file name is longer than 18 chars.
>>> (Not doing so also avoids other duplicates getting printed twice.)
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Should warn_dups become fatal once the patches to fix these...
>>
>> Duplicate symbol 'memory.c#get_reserved_device_memory' (ffff82d080140183
>> != ffff82d080118b5b)
>> Duplicate symbol 'platform_hypercall.c#__maddr_to_virt'
>> (ffff82d08023a3a2 != ffff82d080167e66)
>> Duplicate symbol 'platform_hypercall.c#__virt_to_maddr'
>> (ffff82d08023a401 != ffff82d080167ec5)
>> Duplicate symbol 'platform_hypercall.c#cpumask_check' (ffff82d08023a489
>> != ffff82d080167f4d)
>>
>> have been committed, to avoid accidental reintroduction?
> They all went in already. Or are you saying you saw these on top
> of what is in staging right now?

Current staging right now

andrewcoop@andrewcoop:/local/xen.git/xen$ git log --oneline staging^..
69bdd7f symbols: prefix static symbols with their source file names
bf0d492 x86/mm: don't call HVM-only function for PV guests

However, those duplicates are from the compat code, which I didn't
specifically take your patch 2 for.

>
> However - no to the question here. For one, there's nothing fatal
> about it the absence of xSplice. And even with xSplice I'm not sure
> this really should be fatal at the build stage.

For the non-xSplice case, the worst which can happen is indeed just a
harder-to-read stack trace.

However, for the xSplice case, we really should take reasonable steps to
make patching easier, and that includes avoiding duplicate symbols.

As a future user of xSplice, I would definitely like an option to fail
the hypervisor build if it will result in a hard-to-patch binary.

>  What should force
> people to at least look closely would be a runtime patch using such
> a symbol. And second, ...
>
>> I note that even sysv format doesn't appear to provide directory
>> information, so we still cant distinguish duplicate static symbols in
>> identically named source files in difference directories, but hopefully
>> that should be very rare.
> ... this. I actually see one with some gcc versions (an inline function
> not expanded inline in two different cpufreq.c).

An inline function with an ASSERT/BUG or alternative by any chance?  GCC
appears to aggressively out-of-line these, which is why had to sprinkle
always_inline to helpers such as stac()/clac()

~Andrew