Re: [PATCH 5/6] xen/x86: Avoid using local labels for UNLIKELY() regions

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

On 04/11/15 14:45, Jan Beulich wrote:
>>>> On 30.10.15 at 20:59, <andrew.cooper3@citrix.com> wrote:
>> Using local labels causes the stack trace to use the last non-local
>> label emitted by the compiler in the translation unit, which is almost
>> always unrelated.
>>
>> e.g. A (contrived debug) example switches from:
>>
>>   (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
>>   (XEN) CPU:    0
>>   (XEN) RIP:    e008:[<ffff82d0801961e2>] 
>> asm_domain_crash_synchronous+0x44/0x4c
>>   ...
>>   (XEN) Xen call trace:
>>   (XEN)    [<ffff82d0801961e2>] asm_domain_crash_synchronous+0x44/0x4c
>>   (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9
>>
>> to:
>>
>>   (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
>>   (XEN) CPU:    0
>>   (XEN) RIP:    e008:[<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
>>   ...
>>   (XEN) Xen call trace:
>>   (XEN)    [<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
>>   (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9
>>
>> which is far more relevant when identifying %eip.
> I disagree; I specifically used local labels here to avoid cluttering the
> symbol table.

Needless cluttering is indeed bad.  However, the effect here is a
misleading stack trace.

As the entire point of the symbol table is to generate usable
information in situations like this, I don't see this as an issue.

>  If anything I'd agree to adding a label to the start of
> subsection 1 (or .fixup),

How is this suggestion any different to what I have presented?  Each
unlikely section still gets a non-local label.

> but it's not clear how emitting such labels
> could be avoided when that section is empty (such labels would clash
> with whatever label is first in the next compilation unit).

All UNLIKELY() sections have content, as they are manually created.  Or
have I missed something?

> Maybe we
> could discard them in the symbols processing tool if their address
> matches another symbol.
>
>> Additionally, correct the inclusion of the tag parameter in the C UNLIKELY
>> blocks, to use the passed tag, rather than a literal ".tag".
> This part I agree with of course, and would therefore like to as for this
> to be split off.

I will see about this, after we have agreed on the other bits.

~Andrew