Re: [PATCH 2/2] Xen/x86: Improve information from domain_crash_synchronous

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

On 05/09/13 11:17, Jan Beulich wrote:
>>> That shouldn't be happening - whether a symbol is local or global
>>> should not matter to symbol table generation and consumption.
>>> The matter would be different is the label started with .L...
>> Hmm - this was caught by my testing.  I had initially assumed that
>> print_symbol() would DTRT, but it didn't.  Perhaps it is been fed off
>> the global symbol table rather than the debug symbol table.
> The debug symbol table never gets used, but local symbols
> should always end up in the normal ELF symbol table. I just
> checked - they do here.

So they are.  I have just double checked my debugging, and I was getting
mixed up with the issue from below, which would leave these .globl as
failed debugging.  I shall remove them.

>
>>>>>> @@ -329,7 +330,12 @@ UNLIKELY_END(compat_bounce_failsafe)
>>>>>>          movzwl TRAPBOUNCE_cs(%rdx),%eax
>>>>>>          /* Null selectors (0-3) are not allowed. */
>>>>>>          testl $~3,%eax
>>>>>> -        jz    domain_crash_synchronous
>>>>>> +.Lcompat_bounce_null_selector:
>>>>>> +UNLIKELY_START(z, compat_bounce_null_selector)
>>>>>> +        lea   .Lcompat_bounce_null_selector(%rip), %rdi
>>>>>> +        jmp   asm_domain_crash_synchronous
>>>>>> +        ud2a
>>>>>> +UNLIKELY_END(compat_bounce_null_selector)
>>>>> Here and further down you don't really need the label at the
>>>>> start of the unlikely section - the place can as well be identified
>>>>> by using
>>>>>
>>>>>         lea   (%rip), %rdi
>>>>>
>>>>> inside that section (the place is still unique, just outside the
>>>>> original code stream, i.e. just slightly more difficult to
>>>>> re-associate).
>>>> But in an unlikely section, %rip is shifted quite a lot from %rip of the
>>>> code immediately before.  This is also for the benefit of print_symbol()
>>>> which will pick up the {compat_,}create_bounce_frame rather than the
>>>> global symbol surrounding the unlikely section.
>>> I understand that, but stray labels are at clear risk of getting
>>> deleted by a subsequent cleanup patch anyway. Hence either
>>> we need a solution without stray labels, or live with the need
>>> to re-associate the address pointed to be the crash log
>>> messages to the original function.
>> That was eluded to in my patch 0 (perhaps not well enough), where I
>> intend to augment UNLIKELY_START() to automatically generate this
>> symbol, and provide an __UNLIKLEY_ENTRY_SYM() accessor.  The code for
>> that was rather tangled with your UNLIKELY_DONE() patch, which is why I
>> left it and was going to fix up after your series is committed.
> I did realize those intentions, but whether an orphan label gets
> added here or in the macro doesn't matter - the label remains
> orphaned, and hence would be a likely subject to janitorial work.
>
> Jan
>

But any janitorial work which removes the proposed new label from
UNLIKELY_START() will cause a subsequent assemble error when
__UNLIKELY_ENTRY_SYM() references a non-existant label.

~Andrew