* [PATCH] KVM: selftests: Gracefully handle empty stack traces @ 2022-09-22 23:17 David Matlack 2022-09-26 20:41 ` Vipin Sharma 0 siblings, 1 reply; 5+ messages in thread From: David Matlack @ 2022-09-22 23:17 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, David Matlack, Vipin Sharma, Sean Christopherson 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. 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; + } for (i = 2; i < n; i++) c += sprintf(c, " %lx", ((unsigned long) stack[i]) - 1); + c += sprintf(c, "%s", pipeline); #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-result" base-commit: 372d07084593dc7a399bf9bee815711b1fb1bcf2 prerequisite-patch-id: 2e3661ba8856c29b769499bac525b6943d9284b8 prerequisite-patch-id: 1a148d98d96d73a520ed070260608ddf1bdd0f08 -- 2.37.3.998.g577e59143f-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Gracefully handle empty stack traces 2022-09-22 23:17 [PATCH] KVM: selftests: Gracefully handle empty stack traces David Matlack @ 2022-09-26 20:41 ` Vipin Sharma 2022-09-26 20:53 ` Sean Christopherson 0 siblings, 1 reply; 5+ messages in thread From: Vipin Sharma @ 2022-09-26 20:41 UTC (permalink / raw) To: David Matlack; +Cc: Paolo Bonzini, kvm, Sean Christopherson 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. > > 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) It is more natural to check the return value when an API has returned. Verified that this change does fix the issue. Thanks for the fix. > for (i = 2; i < n; i++) > c += sprintf(c, " %lx", ((unsigned long) stack[i]) - 1); > + > c += sprintf(c, "%s", pipeline); > #pragma GCC diagnostic push > #pragma GCC diagnostic ignored "-Wunused-result" > > base-commit: 372d07084593dc7a399bf9bee815711b1fb1bcf2 > prerequisite-patch-id: 2e3661ba8856c29b769499bac525b6943d9284b8 > prerequisite-patch-id: 1a148d98d96d73a520ed070260608ddf1bdd0f08 > -- > 2.37.3.998.g577e59143f-goog > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Gracefully handle empty stack traces 2022-09-26 20:41 ` Vipin Sharma @ 2022-09-26 20:53 ` Sean Christopherson 2022-09-26 21:47 ` David Matlack 0 siblings, 1 reply; 5+ messages in thread From: Sean Christopherson @ 2022-09-26 20:53 UTC (permalink / raw) To: Vipin Sharma; +Cc: David Matlack, Paolo Bonzini, kvm 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? > > 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Gracefully handle empty stack traces 2022-09-26 20:53 ` Sean Christopherson @ 2022-09-26 21:47 ` David Matlack 2022-09-27 13:51 ` Paolo Bonzini 0 siblings, 1 reply; 5+ messages in thread From: David Matlack @ 2022-09-26 21:47 UTC (permalink / raw) To: Sean Christopherson; +Cc: Vipin Sharma, Paolo Bonzini, kvm 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" ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: selftests: Gracefully handle empty stack traces 2022-09-26 21:47 ` David Matlack @ 2022-09-27 13:51 ` Paolo Bonzini 0 siblings, 0 replies; 5+ messages in thread From: Paolo Bonzini @ 2022-09-27 13:51 UTC (permalink / raw) To: David Matlack, Sean Christopherson; +Cc: Vipin Sharma, kvm On 9/26/22 23:47, David Matlack wrote: > 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" > I think your original patch is better, just with diff --git a/tools/testing/selftests/kvm/lib/assert.c b/tools/testing/selftests/kvm/lib/assert.c index c1ce54a41eca..be2cab00f541 100644 --- a/tools/testing/selftests/kvm/lib/assert.c +++ b/tools/testing/selftests/kvm/lib/assert.c @@ -36,11 +36,9 @@ static void test_dump_stack(void) n * (((sizeof(void *)) * 2) + 1) + /* Null terminator: */ 1]; - char *c; + char *c = cmd; n = backtrace(stack, n); - c = &cmd[0]; - c += sprintf(c, "%s", addr2line); /* * Skip the first 2 frames, which should be test_dump_stack() and * test_assert(); both of which are declared noinline. Bail if the @@ -51,6 +49,8 @@ static void test_dump_stack(void) fputs(" (stack trace empty)\n", stderr); return; } + + c += sprintf(c, "%s", addr2line); for (i = 2; i < n; i++) c += sprintf(c, " %lx", ((unsigned long) stack[i]) - 1); squashed in to keep the "if" and backtrace() call as close as possible. Paolo ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-27 13:51 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-22 23:17 [PATCH] KVM: selftests: Gracefully handle empty stack traces David Matlack 2022-09-26 20:41 ` Vipin Sharma 2022-09-26 20:53 ` Sean Christopherson 2022-09-26 21:47 ` David Matlack 2022-09-27 13:51 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox