Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Jan Beulich <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>, WeiLiu <wl@xen.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack
Date: Wed, 3 Jul 2019 10:34:43 +0000
Message-ID: <2e2a1d5d-f04e-5400-6817-86a7985b1c9b@suse.com> (raw)
In-Reply-To: <eb44801a-1081-858d-4853-6dc574e0c6d6@citrix.com>

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.

>> (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?

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

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-31  8:59 [PATCH 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich
2019-05-31  8:59 ` [Xen-devel] " Jan Beulich
2019-05-31  9:17 ` [PATCH 1/2] x86/traps: guard top-of-stack reads Jan Beulich
2019-05-31  9:17   ` [Xen-devel] " Jan Beulich
2019-06-07 17:51   ` Andrew Cooper
2019-06-11  9:57     ` Jan Beulich
2019-05-31  9:22 ` [PATCH 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich
2019-05-31  9:22   ` [Xen-devel] " Jan Beulich
2019-06-07 18:01   ` Andrew Cooper
2019-06-11  9:46     ` Jan Beulich
2019-06-17  8:10 ` [Xen-devel] [PATCH v2 0/2]: x86/traps: improve show_trace()'s top-of-stack handling Jan Beulich
2019-06-17  8:12   ` [Xen-devel] [PATCH v2 1/2] x86/traps: guard top-of-stack reads Jan Beulich
2019-07-02 17:47     ` Andrew Cooper
2019-07-03  7:10       ` Jan Beulich
2019-06-17  8:13   ` [Xen-devel] [PATCH v2 2/2] x86/traps: widen condition for logging top-of-stack Jan Beulich
2019-07-03 10:21     ` Andrew Cooper
2019-07-03 10:34       ` Jan Beulich [this message]
2019-07-03 19:47         ` Andrew Cooper
2019-07-04  9:09           ` Jan Beulich

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2e2a1d5d-f04e-5400-6817-86a7985b1c9b@suse.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=roger.pau@citrix.com \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox