Re: [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack

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

On 03/07/2019 11:34, Jan Beulich wrote:
> On 03.07.2019 12:21, Andrew Cooper wrote:
>> On 17/06/2019 09:13, Jan Beulich wrote:
>>> Despite -fno-omit-frame-pointer the compiler may omit the frame pointer,
>>> often for relatively simple leaf functions.
>> Actually, the problem is more widespread than this.  For every function,
>> there is a non-zero quantity of time between the function starting and
>> the frame pointer being set up.
>>
>> However, half of this time is spent with the old %rbp on the top of the
>> stack, so won't benefit from these changes.
> I think the compiler typically pairs push %rbp and mov %rsp, %rbp,
> but this pair may not sit at the beginning of the function. And it's
> that other code that's prone to crash. The push %rbp may also fault
> (most notably due to stack overrun), but that would then still have
> the top of stack covered by the change here. The mov %rsp, %rbp,
> otoh, won't plausibly fault. IOW I think it's far more than "half of
> the time" that this change helps.

My statement wasn't meant as a criticism, but more of an observation.

>
>>> (To give a specific example,
>>> the case I've run into this with is _pci_hide_device() and gcc 8.
>>> Interestingly the even more simple neighboring iommu_has_feature() does
>>> get a frame pointer set up, around just a single instruction. But this
>>> may be a result of the size-of-asm() effects discussed elsewhere.)
>>>
>>> Log the top-of-stack value if it looks valid _or_ if RIP looks invalid.
>> This far, I'm happy with.
>>
>>> Also annotate non-frame-pointer-based stack trace entries with a
>>> question mark, to signal clearly that any one of them may not actually
>>> be part of the call stack.
>> I'm still opposed to this.  The introduction of ? does more harm than
>> good IMO, because it simply can't be trusted.
>>
>> Stack traces are not guaranteed-accurate, even with frame pointers
>> enabled.  The only thing we can say for certain in any trace is where
>> %rip points.
> Yes, I realize you still don't like this. But similarly to the
> other patch set - on the v1 discussion here I was lacking
> feedback, and hence I eventually timed out and sent v2. The
> question is - what is your alternative proposal to distinguish
> the truly guessed entry logged here from the more reliable
> ones? And then similarly how to distinguish the less reliable
> ones produced by the !CONFIG_FRAME_POINTER variant of
> _show_trace() from their more reliable counterparts?

A crazy idea I've just had.  Annotate all printed lines with a character
identifying which source of information we used?

We could have [r] for register state, [f] for "from frame pointer", and
[s] for "from stack rubble".

~Andrew

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