On Mon, Sep 26, 2022 at 08:53:35PM +0000, Sean Christopherson wrote:
> On Mon, Sep 26, 2022, Vipin Sharma wrote:
> > On Thu, Sep 22, 2022 at 4:17 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> > >
> > > Bail out of test_dump_stack() if the stack trace is empty rather than
> > > invoking addr2line with zero addresses. The problem with the latter is
> > > that addr2line will block waiting for addresses to be passed in via
> > > stdin, e.g. if running a selftest from an interactive terminal.
>
> How does this bug occur? Does backtrace() get inlined?
backtrace() is returning 0. I haven't debugged it further than that yet.
I figured gracefully handling an empty stack trace would be useful to
have independent of this specific issue (which I assume has something to
do with our Google-specific build process).
backtrace() is not getting inlined.
>
> > > Opportunistically fix up the comment that mentions skipping 3 frames
> > > since only 2 are skipped in the code.
> > >
> > > Cc: Vipin Sharma <vipinsh@xxxxxxxxxx>
> > > Cc: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>
> > > ---
> > > tools/testing/selftests/kvm/lib/assert.c | 12 +++++++++---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/lib/assert.c b/tools/testing/selftests/kvm/lib/assert.c
> > > index 71ade6100fd3..c1ce54a41eca 100644
> > > --- a/tools/testing/selftests/kvm/lib/assert.c
> > > +++ b/tools/testing/selftests/kvm/lib/assert.c
> > > @@ -42,12 +42,18 @@ static void test_dump_stack(void)
> > > c = &cmd[0];
> > > c += sprintf(c, "%s", addr2line);
> > > /*
> > > - * Skip the first 3 frames: backtrace, test_dump_stack, and
> > > - * test_assert. We hope that backtrace isn't inlined and the other two
> > > - * we've declared noinline.
> > > + * Skip the first 2 frames, which should be test_dump_stack() and
> > > + * test_assert(); both of which are declared noinline. Bail if the
> > > + * resulting stack trace would be empty. Otherwise, addr2line will block
> > > + * waiting for addresses to be passed in via stdin.
> > > */
> > > + if (n <= 2) {
> > > + fputs(" (stack trace empty)\n", stderr);
> > > + return;
> > > + }
> >
> > Shouldn't this condition be put immediately after
> > n = backtrace(stack,n)
>
> Agreed, that would be more intuitive.
I had that at one point, but then it became confusing that the check is for
(n <= 2) and not (!n).
How about this?
diff --git a/tools/testing/selftests/kvm/lib/assert.c b/tools/testing/selftests/kvm/lib/assert.c
index 71ade6100fd3..2b56bbff970c 100644
--- a/tools/testing/selftests/kvm/lib/assert.c
+++ b/tools/testing/selftests/kvm/lib/assert.c
@@ -38,16 +38,28 @@ static void test_dump_stack(void)
1];
char *c;
- n = backtrace(stack, n);
c = &cmd[0];
c += sprintf(c, "%s", addr2line);
- /*
- * Skip the first 3 frames: backtrace, test_dump_stack, and
- * test_assert. We hope that backtrace isn't inlined and the other two
- * we've declared noinline.
- */
- for (i = 2; i < n; i++)
- c += sprintf(c, " %lx", ((unsigned long) stack[i]) - 1);
+
+ n = backtrace(stack, n);
+ if (n > 2) {
+ /*
+ * Skip the first 2 frames, which should be test_dump_stack()
+ * and test_assert(); both of which are declared noinline.
+ */
+ for (i = 2; i < n; i++)
+ c += sprintf(c, " %lx", ((unsigned long) stack[i]) - 1);
+ } else {
+ /*
+ * Bail if the resulting stack trace would be empty. Otherwise,
+ * addr2line will block waiting for addresses to be passed in
+ * via stdin.
+ */
+ fputs(" (stack trace missing)\n", stderr);
+ return;
+ }
+
c += sprintf(c, "%s", pipeline);
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-result"
