Re: [PATCH] KVM: selftests: Gracefully handle empty stack traces

[David Matlack <dmatlack@google.com>]

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@google.com> 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@google.com>
> > > Cc: Sean Christopherson <seanjc@google.com>
> > > Signed-off-by: David Matlack <dmatlack@google.com>
> > > ---
> > >  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"