Received-SPF: pass (google.com: domain of linux-kernel+bounces-157715-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 24 Apr 2024 18:02:24 -0300
From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Namhyung Kim <namhyung@kernel.org>,
	Kan Liang <kan.liang@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	linux-perf-users@vger.kernel.org
Subject: Re: [PATCH] perf test: Add a new test for perf annotate
Message-ID: <ZilzYDlzj7c1mQq4@x1>
References: <20240424001231.849972-1-namhyung@kernel.org>
 <CAP-5=fWNWiuiLOKyNHYcuT9xmJkciUBodjCX1E_TRrTMMsM4uw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAP-5=fWNWiuiLOKyNHYcuT9xmJkciUBodjCX1E_TRrTMMsM4uw@mail.gmail.com>

On Wed, Apr 24, 2024 at 11:09:48AM -0700, Ian Rogers wrote:
> On Tue, Apr 23, 2024 at 5:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Add a basic perf annotate test
> >
> >   $ ./perf test annotate -vv
> >    76: perf annotate basic tests:
> >   --- start ---
> >   test child forked, pid 846989
> >    fbcd0-fbd55 l noploop
> >   perf does have symbol 'noploop'
> >   Basic perf annotate test
> >            : 0     0xfbcd0 <noploop>:
> >       0.00 :   fbcd0:       pushq   %rbp
> >       0.00 :   fbcd1:       movq    %rsp, %rbp
> >       0.00 :   fbcd4:       pushq   %r12
> >       0.00 :   fbcd6:       pushq   %rbx
> >       0.00 :   fbcd7:       movl    $1, %ebx
> >       0.00 :   fbcdc:       subq    $0x10, %rsp
> >       0.00 :   fbce0:       movq    %fs:0x28, %rax
> >       0.00 :   fbce9:       movq    %rax, -0x18(%rbp)
> >       0.00 :   fbced:       xorl    %eax, %eax
> >       0.00 :   fbcef:       testl   %edi, %edi
> >       0.00 :   fbcf1:       jle     0xfbd04
> >       0.00 :   fbcf3:       movq    (%rsi), %rdi
> >       0.00 :   fbcf6:       movl    $0xa, %edx
> >       0.00 :   fbcfb:       xorl    %esi, %esi
> >       0.00 :   fbcfd:       callq   0x41920
> >       0.00 :   fbd02:       movl    %eax, %ebx
> >       0.00 :   fbd04:       leaq    -0x7b(%rip), %r12   # fbc90 <sighandler>
> >       0.00 :   fbd0b:       movl    $2, %edi
> >       0.00 :   fbd10:       movq    %r12, %rsi
> >       0.00 :   fbd13:       callq   0x40a00
> >       0.00 :   fbd18:       movl    $0xe, %edi
> >       0.00 :   fbd1d:       movq    %r12, %rsi
> >       0.00 :   fbd20:       callq   0x40a00
> >       0.00 :   fbd25:       movl    %ebx, %edi
> >       0.00 :   fbd27:       callq   0x407c0
> >       0.10 :   fbd2c:       movl    0x89785e(%rip), %eax        # 993590 <done>
> >       0.00 :   fbd32:       testl   %eax, %eax
> >      99.90 :   fbd34:       je      0xfbd2c
> >       0.00 :   fbd36:       movq    -0x18(%rbp), %rax
> >       0.00 :   fbd3a:       subq    %fs:0x28, %rax
> >       0.00 :   fbd43:       jne     0xfbd50
> >       0.00 :   fbd45:       addq    $0x10, %rsp
> >       0.00 :   fbd49:       xorl    %eax, %eax
> >       0.00 :   fbd4b:       popq    %rbx
> >       0.00 :   fbd4c:       popq    %r12
> >       0.00 :   fbd4e:       popq    %rbp
> >       0.00 :   fbd4f:       retq
> >       0.00 :   fbd50:       callq   0x407e0
> >       0.00 :   fbcd0:       pushq   %rbp
> >       0.00 :   fbcd1:       movq    %rsp, %rbp
> >       0.00 :   fbcd4:       pushq   %r12
> >       0.00 :   fbcd0:  push   %rbp
> >       0.00 :   fbcd1:  mov    %rsp,%rbp
> >       0.00 :   fbcd4:  push   %r12
> >   Basic annotate test [Success]
> >   ---- end(0) ----
> >    76: perf annotate basic tests                                       : Ok
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> 
> Looks good, thanks for this!
> 
> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks! Applied, with the following changes to improve the error
reporting, please holler if you disagree:

diff --git a/tools/perf/tests/shell/annotate.sh b/tools/perf/tests/shell/annotate.sh
index 7820d13eebaef535..1db1e8113d9943a6 100755
--- a/tools/perf/tests/shell/annotate.sh
+++ b/tools/perf/tests/shell/annotate.sh
@@ -36,7 +36,7 @@ test_basic() {
   echo "Basic perf annotate test"
   if ! perf record -o "${perfdata}" ${testprog} 2> /dev/null
   then
-    echo "Basic annotate [Failed record]"
+    echo "Basic annotate [Failed: perf record]"
     err=1
     return
   fi
@@ -44,7 +44,7 @@ test_basic() {
   # check if it has the target symbol
   if ! perf annotate -i "${perfdata}" 2> /dev/null | grep "${testsym}"
   then
-    echo "Basic annotate [Failed missing symbol]"
+    echo "Basic annotate [Failed: missing target symbol]"
     err=1
     return
   fi
@@ -52,7 +52,7 @@ test_basic() {
   # check if it has the disassembly lines
   if ! perf annotate -i "${perfdata}" 2> /dev/null | grep "${disasm_regex}"
   then
-    echo "Basic annotate [Failed missing disasm output]"
+    echo "Basic annotate [Failed: missing disasm output from default disassembler]"
     err=1
     return
   fi
@@ -61,7 +61,7 @@ test_basic() {
   if ! perf annotate -i "${perfdata}" "${testsym}" 2> /dev/null | \
 	  grep -m 3 "${disasm_regex}"
   then
-    echo "Basic annotate [Failed missing disasm output]"
+    echo "Basic annotate [Failed: missing disasm output when specifying the target symbol]"
     err=1
     return
   fi
@@ -70,7 +70,7 @@ test_basic() {
   if ! perf annotate -i "${perfdata}" --objdump=objdump 2> /dev/null | \
 	  grep -m 3 "${disasm_regex}"
   then
-    echo "Basic annotate [Failed missing disasm output from objdump]"
+    echo "Basic annotate [Failed: missing disasm output from non default disassembler (using --objdump)]"
     err=1
     return
   fi