[PATCH v2 0/2] perf test: Add option to change objdump binary

[PATCH v2 0/2] perf test: Add option to change objdump binary

[James Clark]

Changes since V1:

 * Add perf config option 

James Clark (2):
  perf test: Add option to change objdump binary
  perf test: Add support for setting objdump binary via perf config

 tools/perf/Documentation/perf-config.txt |  4 ++++
 tools/perf/tests/builtin-test.c          | 15 +++++++++++++++
 tools/perf/tests/code-reading.c          |  2 +-
 tools/perf/tests/tests.h                 |  1 +
 4 files changed, 21 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v2 1/2] perf test: Add option to change objdump binary

[James Clark]

All of the other Perf subcommands that use objdump have an option to
specify the binary, so add the same option to perf test.

This is useful if you have built the kernel with a different toolchain
to the system one, where the system objdump may fail to disassemble
vmlinux.

Now this can be fixed with something like this:

  $ perf test --objdump llvm-objdump "object code reading"

Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/tests/builtin-test.c | 3 +++
 tools/perf/tests/code-reading.c | 2 +-
 tools/perf/tests/tests.h        | 1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index cb6f1dd00dc4..a8d17dd50588 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -32,6 +32,7 @@
 
 static bool dont_fork;
 const char *dso_to_test;
+const char *test_objdump_path = "objdump";
 
 /*
  * List of architecture specific tests. Not a weak symbol as the array length is
@@ -529,6 +530,8 @@ int cmd_test(int argc, const char **argv)
 		    "Do not fork for testcase"),
 	OPT_STRING('w', "workload", &workload, "work", "workload to run for testing"),
 	OPT_STRING(0, "dso", &dso_to_test, "dso", "dso to test"),
+	OPT_STRING(0, "objdump", &test_objdump_path, "path",
+		   "objdump binary to use for disassembly and annotations"),
 	OPT_END()
 	};
 	const char * const test_subcommands[] = { "list", NULL };
diff --git a/tools/perf/tests/code-reading.c b/tools/perf/tests/code-reading.c
index 3af81012014e..b353358acc3a 100644
--- a/tools/perf/tests/code-reading.c
+++ b/tools/perf/tests/code-reading.c
@@ -185,7 +185,7 @@ static int read_via_objdump(const char *filename, u64 addr, void *buf,
 	int ret;
 
 	fmt = "%s -z -d --start-address=0x%"PRIx64" --stop-address=0x%"PRIx64" %s";
-	ret = snprintf(cmd, sizeof(cmd), fmt, "objdump", addr, addr + len,
+	ret = snprintf(cmd, sizeof(cmd), fmt, test_objdump_path, addr, addr + len,
 		       filename);
 	if (ret <= 0 || (size_t)ret >= sizeof(cmd))
 		return -1;
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index b394f3ac2d66..dad3d7414142 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -207,5 +207,6 @@ DECLARE_WORKLOAD(brstack);
 DECLARE_WORKLOAD(datasym);
 
 extern const char *dso_to_test;
+extern const char *test_objdump_path;
 
 #endif /* TESTS_H */
-- 
2.34.1

[PATCH v2 2/2] perf test: Add support for setting objdump binary via perf config

[James Clark]

Add a perf config variable that does the same thing as "perf test
--objdump <x>".

Also update the man page.

Signed-off-by: James Clark <james.clark@arm.com>
---
 tools/perf/Documentation/perf-config.txt |  4 ++++
 tools/perf/tests/builtin-test.c          | 12 ++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
index 0b4e79dbd3f6..16398babd1ef 100644
--- a/tools/perf/Documentation/perf-config.txt
+++ b/tools/perf/Documentation/perf-config.txt
@@ -722,6 +722,10 @@ session-<NAME>.*::
 		Defines new record session for daemon. The value is record's
 		command line without the 'record' keyword.
 
+test.*::
+
+	test.objdump::
+		objdump binary to use for disassembly and annotations.
 
 SEE ALSO
 --------
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index a8d17dd50588..113e92119e1d 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -14,6 +14,7 @@
 #include <sys/wait.h>
 #include <sys/stat.h>
 #include "builtin.h"
+#include "config.h"
 #include "hist.h"
 #include "intlist.h"
 #include "tests.h"
@@ -514,6 +515,15 @@ static int run_workload(const char *work, int argc, const char **argv)
 	return -1;
 }
 
+static int perf_test__config(const char *var, const char *value,
+			     void *data __maybe_unused)
+{
+	if (!strcmp(var, "test.objdump"))
+		test_objdump_path = value;
+
+	return 0;
+}
+
 int cmd_test(int argc, const char **argv)
 {
 	const char *test_usage[] = {
@@ -541,6 +551,8 @@ int cmd_test(int argc, const char **argv)
         if (ret < 0)
                 return ret;
 
+	perf_config(perf_test__config, NULL);
+
 	/* Unbuffered output */
 	setvbuf(stdout, NULL, _IONBF, 0);
 
-- 
2.34.1

Re: [PATCH v2 2/2] perf test: Add support for setting objdump binary via perf config

[Arnaldo Carvalho de Melo]

Em Mon, Nov 06, 2023 at 03:10:49PM +0000, James Clark escreveu:
> Add a perf config variable that does the same thing as "perf test
> --objdump <x>".
> 
> Also update the man page.

That is ok, if one wants to change objdump just for testing, as a
followup improvement it may be interesting to allow that for the other
tools that have --objdump as well as to add this as a global option,
that affects all tools, no?

Anyway, applied both patches.

- Arnaldo
 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  tools/perf/Documentation/perf-config.txt |  4 ++++
>  tools/perf/tests/builtin-test.c          | 12 ++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> index 0b4e79dbd3f6..16398babd1ef 100644
> --- a/tools/perf/Documentation/perf-config.txt
> +++ b/tools/perf/Documentation/perf-config.txt
> @@ -722,6 +722,10 @@ session-<NAME>.*::
>  		Defines new record session for daemon. The value is record's
>  		command line without the 'record' keyword.
>  
> +test.*::
> +
> +	test.objdump::
> +		objdump binary to use for disassembly and annotations.
>  
>  SEE ALSO
>  --------
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index a8d17dd50588..113e92119e1d 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -14,6 +14,7 @@
>  #include <sys/wait.h>
>  #include <sys/stat.h>
>  #include "builtin.h"
> +#include "config.h"
>  #include "hist.h"
>  #include "intlist.h"
>  #include "tests.h"
> @@ -514,6 +515,15 @@ static int run_workload(const char *work, int argc, const char **argv)
>  	return -1;
>  }
>  
> +static int perf_test__config(const char *var, const char *value,
> +			     void *data __maybe_unused)
> +{
> +	if (!strcmp(var, "test.objdump"))
> +		test_objdump_path = value;
> +
> +	return 0;
> +}
> +
>  int cmd_test(int argc, const char **argv)
>  {
>  	const char *test_usage[] = {
> @@ -541,6 +551,8 @@ int cmd_test(int argc, const char **argv)
>          if (ret < 0)
>                  return ret;
>  
> +	perf_config(perf_test__config, NULL);
> +
>  	/* Unbuffered output */
>  	setvbuf(stdout, NULL, _IONBF, 0);
>  
> -- 
> 2.34.1
> 

-- 

- Arnaldo

Re: [PATCH v2 2/2] perf test: Add support for setting objdump binary via perf config

[Arnaldo Carvalho de Melo]

Em Wed, Nov 08, 2023 at 05:53:34PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Nov 06, 2023 at 03:10:49PM +0000, James Clark escreveu:
> > Add a perf config variable that does the same thing as "perf test
> > --objdump <x>".
> > 
> > Also update the man page.
> 
> That is ok, if one wants to change objdump just for testing, as a
> followup improvement it may be interesting to allow that for the other
> tools that have --objdump as well as to add this as a global option,
> that affects all tools, no?
> 
> Anyway, applied both patches.

And added this to the last one:

Committer testing:

  # perf config test.objdump
  # perf test "object code reading"
   26: Object code reading                                             : Ok
  # perf config test.objdump=blah
  # perf config test.objdump
  test.objdump=blah
  # perf test "object code reading"
   26: Object code reading                                             : FAILED!
  # perf test -v "object code reading"
   26: Object code reading                                             :
  --- start ---
  test child forked, pid 600599
  Looking at the vmlinux_path (8 entries long)
  Using /proc/kcore for kernel data
  Using /proc/kallsyms for symbols
  Parsing event 'cycles'
  Using CPUID AuthenticAMD-25-21-0
  mmap size 528384B
  Reading object code for memory address: 0x4d9a02
  File is: /home/acme/bin/perf
  On file address is: 0xd9a02
  Objdump command is: blah -z -d --start-address=0x4d9a02 --stop-address=0x4d9a82 /home/acme/bin/perf
  objdump read too few bytes: 128
  Bytes read differ from those read by objdump
  buf1 (dso):
  0x48 0x85 0xff 0x74 0x29 0xe8 0x94 0xdf 0x07 0x00 0x8b 0x73 0x1c 0x48 0x8b 0x43 
  0x08 0xeb 0xa5 0x0f 0x1f 0x00 0x48 0x8b 0x45 0xe8 0x64 0x48 0x2b 0x04 0x25 0x28 
  0x00 0x00 0x00 0x75 0x0f 0x48 0x8b 0x5d 0xf8 0xc9 0xc3 0x0f 0x1f 0x00 0x48 0x8b 
  0x43 0x08 0xeb 0x84 0xe8 0xc5 0x3e 0xf3 0xff 0x0f 0x1f 0x44 0x00 0x00 0x55 0x48 
  0x89 0xe5 0x41 0x56 0x41 0x55 0x49 0x89 0xd5 0x41 0x54 0x49 0x89 0xfc 0x53 0x48 
  0x89 0xf3 0x48 0x83 0xec 0x30 0x48 0x8b 0x7e 0x20 0x64 0x48 0x8b 0x04 0x25 0x28 
  0x00 0x00 0x00 0x48 0x89 0x45 0xd8 0x31 0xc0 0x48 0x89 0x75 0xb0 0x48 0xc7 0x45 
  0xb8 0x00 0x00 0x00 0x00 0x48 0xc7 0x45 0xc0 0x00 0x00 0x00 0x00 0xe8 0xad 0xfa 
  
  buf2 (objdump):
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  
  test child finished with -1
  ---- end ----
  Object code reading: FAILED!
  # perf config test.objdump=/usr/bin/objdump
  # perf config test.objdump
  test.objdump=/usr/bin/objdump
  # perf test "object code reading"
   26: Object code reading                                             : Ok
  #

Signed-off-by: James Clark <james.clark@arm.com>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Re: [PATCH v2 2/2] perf test: Add support for setting objdump binary via perf config

[James Clark]

On 08/11/2023 20:53, Arnaldo Carvalho de Melo wrote:
> Em Mon, Nov 06, 2023 at 03:10:49PM +0000, James Clark escreveu:
>> Add a perf config variable that does the same thing as "perf test
>> --objdump <x>".
>>
>> Also update the man page.
> 
> That is ok, if one wants to change objdump just for testing, as a
> followup improvement it may be interesting to allow that for the other
> tools that have --objdump as well as to add this as a global option,
> that affects all tools, no?

For the tools they already all share annotate.objdump in the config. Do
you mean that the tests could share the same config instead?

Maybe I could have used annotate.objdump for the tests, but it was used
in a slightly different way, and I thought it would be easier for people
to find if it started with "test."

> 
> Anyway, applied both patches.
> 
> - Arnaldo
>  
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>  tools/perf/Documentation/perf-config.txt |  4 ++++
>>  tools/perf/tests/builtin-test.c          | 12 ++++++++++++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
>> index 0b4e79dbd3f6..16398babd1ef 100644
>> --- a/tools/perf/Documentation/perf-config.txt
>> +++ b/tools/perf/Documentation/perf-config.txt
>> @@ -722,6 +722,10 @@ session-<NAME>.*::
>>  		Defines new record session for daemon. The value is record's
>>  		command line without the 'record' keyword.
>>  
>> +test.*::
>> +
>> +	test.objdump::
>> +		objdump binary to use for disassembly and annotations.
>>  
>>  SEE ALSO
>>  --------
>> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
>> index a8d17dd50588..113e92119e1d 100644
>> --- a/tools/perf/tests/builtin-test.c
>> +++ b/tools/perf/tests/builtin-test.c
>> @@ -14,6 +14,7 @@
>>  #include <sys/wait.h>
>>  #include <sys/stat.h>
>>  #include "builtin.h"
>> +#include "config.h"
>>  #include "hist.h"
>>  #include "intlist.h"
>>  #include "tests.h"
>> @@ -514,6 +515,15 @@ static int run_workload(const char *work, int argc, const char **argv)
>>  	return -1;
>>  }
>>  
>> +static int perf_test__config(const char *var, const char *value,
>> +			     void *data __maybe_unused)
>> +{
>> +	if (!strcmp(var, "test.objdump"))
>> +		test_objdump_path = value;
>> +
>> +	return 0;
>> +}
>> +
>>  int cmd_test(int argc, const char **argv)
>>  {
>>  	const char *test_usage[] = {
>> @@ -541,6 +551,8 @@ int cmd_test(int argc, const char **argv)
>>          if (ret < 0)
>>                  return ret;
>>  
>> +	perf_config(perf_test__config, NULL);
>> +
>>  	/* Unbuffered output */
>>  	setvbuf(stdout, NULL, _IONBF, 0);
>>  
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH v2 2/2] perf test: Add support for setting objdump binary via perf config

[Arnaldo Carvalho de Melo]

Em Thu, Nov 09, 2023 at 10:26:34AM +0000, James Clark escreveu:
> 
> 
> On 08/11/2023 20:53, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Nov 06, 2023 at 03:10:49PM +0000, James Clark escreveu:
> >> Add a perf config variable that does the same thing as "perf test
> >> --objdump <x>".
> >>
> >> Also update the man page.
> > 
> > That is ok, if one wants to change objdump just for testing, as a
> > followup improvement it may be interesting to allow that for the other
> > tools that have --objdump as well as to add this as a global option,
> > that affects all tools, no?
> 
> For the tools they already all share annotate.objdump in the config. Do
> you mean that the tests could share the same config instead?

Probably.
 
> Maybe I could have used annotate.objdump for the tests, but it was used
> in a slightly different way, and I thought it would be easier for people
> to find if it started with "test."

Well, we can have both, with "test." overriding "annotate."?

Anyway, I applied this patch locally, now trying to fix some unrelated
problem that is making 'perf test' fail so that I can push
perf-tools-next publicly.

- Arnaldo
 
> > 
> > Anyway, applied both patches.
> > 
> > - Arnaldo
> >  
> >> Signed-off-by: James Clark <james.clark@arm.com>
> >> ---
> >>  tools/perf/Documentation/perf-config.txt |  4 ++++
> >>  tools/perf/tests/builtin-test.c          | 12 ++++++++++++
> >>  2 files changed, 16 insertions(+)
> >>
> >> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> >> index 0b4e79dbd3f6..16398babd1ef 100644
> >> --- a/tools/perf/Documentation/perf-config.txt
> >> +++ b/tools/perf/Documentation/perf-config.txt
> >> @@ -722,6 +722,10 @@ session-<NAME>.*::
> >>  		Defines new record session for daemon. The value is record's
> >>  		command line without the 'record' keyword.
> >>  
> >> +test.*::
> >> +
> >> +	test.objdump::
> >> +		objdump binary to use for disassembly and annotations.
> >>  
> >>  SEE ALSO
> >>  --------
> >> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> >> index a8d17dd50588..113e92119e1d 100644
> >> --- a/tools/perf/tests/builtin-test.c
> >> +++ b/tools/perf/tests/builtin-test.c
> >> @@ -14,6 +14,7 @@
> >>  #include <sys/wait.h>
> >>  #include <sys/stat.h>
> >>  #include "builtin.h"
> >> +#include "config.h"
> >>  #include "hist.h"
> >>  #include "intlist.h"
> >>  #include "tests.h"
> >> @@ -514,6 +515,15 @@ static int run_workload(const char *work, int argc, const char **argv)
> >>  	return -1;
> >>  }
> >>  
> >> +static int perf_test__config(const char *var, const char *value,
> >> +			     void *data __maybe_unused)
> >> +{
> >> +	if (!strcmp(var, "test.objdump"))
> >> +		test_objdump_path = value;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  int cmd_test(int argc, const char **argv)
> >>  {
> >>  	const char *test_usage[] = {
> >> @@ -541,6 +551,8 @@ int cmd_test(int argc, const char **argv)
> >>          if (ret < 0)
> >>                  return ret;
> >>  
> >> +	perf_config(perf_test__config, NULL);
> >> +
> >>  	/* Unbuffered output */
> >>  	setvbuf(stdout, NULL, _IONBF, 0);
> >>  
> >> -- 
> >> 2.34.1
> >>
> > 

-- 

- Arnaldo

Re: [PATCH v2 2/2] perf test: Add support for setting objdump binary via perf config

[James Clark]

On 09/11/2023 16:14, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 09, 2023 at 10:26:34AM +0000, James Clark escreveu:
>>
>>
>> On 08/11/2023 20:53, Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Nov 06, 2023 at 03:10:49PM +0000, James Clark escreveu:
>>>> Add a perf config variable that does the same thing as "perf test
>>>> --objdump <x>".
>>>>
>>>> Also update the man page.
>>>
>>> That is ok, if one wants to change objdump just for testing, as a
>>> followup improvement it may be interesting to allow that for the other
>>> tools that have --objdump as well as to add this as a global option,
>>> that affects all tools, no?
>>
>> For the tools they already all share annotate.objdump in the config. Do
>> you mean that the tests could share the same config instead?
> 
> Probably.
>  
>> Maybe I could have used annotate.objdump for the tests, but it was used
>> in a slightly different way, and I thought it would be easier for people
>> to find if it started with "test."
> 
> Well, we can have both, with "test." overriding "annotate."?
> 

Yeah overriding could work. IMO just one config value is probably enough
though, so we could change it to annotate.objdump in the tests too, and
remove the docs change. At least until someone has a usecase for a
separate config value for the tests and then add the overriding behavior.

I don't have any strong feelings about either way.

> Anyway, I applied this patch locally, now trying to fix some unrelated
> problem that is making 'perf test' fail so that I can push
> perf-tools-next publicly.
> 
> - Arnaldo
>  
>>>
>>> Anyway, applied both patches.
>>>
>>> - Arnaldo
>>>  
>>>> Signed-off-by: James Clark <james.clark@arm.com>
>>>> ---
>>>>  tools/perf/Documentation/perf-config.txt |  4 ++++
>>>>  tools/perf/tests/builtin-test.c          | 12 ++++++++++++
>>>>  2 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
>>>> index 0b4e79dbd3f6..16398babd1ef 100644
>>>> --- a/tools/perf/Documentation/perf-config.txt
>>>> +++ b/tools/perf/Documentation/perf-config.txt
>>>> @@ -722,6 +722,10 @@ session-<NAME>.*::
>>>>  		Defines new record session for daemon. The value is record's
>>>>  		command line without the 'record' keyword.
>>>>  
>>>> +test.*::
>>>> +
>>>> +	test.objdump::
>>>> +		objdump binary to use for disassembly and annotations.
>>>>  
>>>>  SEE ALSO
>>>>  --------
>>>> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
>>>> index a8d17dd50588..113e92119e1d 100644
>>>> --- a/tools/perf/tests/builtin-test.c
>>>> +++ b/tools/perf/tests/builtin-test.c
>>>> @@ -14,6 +14,7 @@
>>>>  #include <sys/wait.h>
>>>>  #include <sys/stat.h>
>>>>  #include "builtin.h"
>>>> +#include "config.h"
>>>>  #include "hist.h"
>>>>  #include "intlist.h"
>>>>  #include "tests.h"
>>>> @@ -514,6 +515,15 @@ static int run_workload(const char *work, int argc, const char **argv)
>>>>  	return -1;
>>>>  }
>>>>  
>>>> +static int perf_test__config(const char *var, const char *value,
>>>> +			     void *data __maybe_unused)
>>>> +{
>>>> +	if (!strcmp(var, "test.objdump"))
>>>> +		test_objdump_path = value;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  int cmd_test(int argc, const char **argv)
>>>>  {
>>>>  	const char *test_usage[] = {
>>>> @@ -541,6 +551,8 @@ int cmd_test(int argc, const char **argv)
>>>>          if (ret < 0)
>>>>                  return ret;
>>>>  
>>>> +	perf_config(perf_test__config, NULL);
>>>> +
>>>>  	/* Unbuffered output */
>>>>  	setvbuf(stdout, NULL, _IONBF, 0);
>>>>  
>>>> -- 
>>>> 2.34.1
>>>>
>>>
> 

Re: [PATCH v2 2/2] perf test: Add support for setting objdump binary via perf config

[Arnaldo Carvalho de Melo]

Em Fri, Nov 10, 2023 at 01:57:11PM +0000, James Clark escreveu:
> 
> 
> On 09/11/2023 16:14, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Nov 09, 2023 at 10:26:34AM +0000, James Clark escreveu:
> >>
> >>
> >> On 08/11/2023 20:53, Arnaldo Carvalho de Melo wrote:
> >>> Em Mon, Nov 06, 2023 at 03:10:49PM +0000, James Clark escreveu:
> >>>> Add a perf config variable that does the same thing as "perf test
> >>>> --objdump <x>".
> >>>>
> >>>> Also update the man page.
> >>>
> >>> That is ok, if one wants to change objdump just for testing, as a
> >>> followup improvement it may be interesting to allow that for the other
> >>> tools that have --objdump as well as to add this as a global option,
> >>> that affects all tools, no?
> >>
> >> For the tools they already all share annotate.objdump in the config. Do
> >> you mean that the tests could share the same config instead?
> > 
> > Probably.
> >  
> >> Maybe I could have used annotate.objdump for the tests, but it was used
> >> in a slightly different way, and I thought it would be easier for people
> >> to find if it started with "test."
> > 
> > Well, we can have both, with "test." overriding "annotate."?
> > 
> 
> Yeah overriding could work. IMO just one config value is probably enough
> though, so we could change it to annotate.objdump in the tests too, and
> remove the docs change. At least until someone has a usecase for a
> separate config value for the tests and then add the overriding behavior.
> 
> I don't have any strong feelings about either way.

We didn't publish any new version with test.objdump=, so please consider
submitting a patch for perf-tools-next making annotate.objdump= be used
in 'perf test' as well, update the documentation stating that this
applies to 'perf test' as well.

- Arnaldo
 
> > Anyway, I applied this patch locally, now trying to fix some unrelated
> > problem that is making 'perf test' fail so that I can push
> > perf-tools-next publicly.
> > 
> > - Arnaldo
> >  
> >>>
> >>> Anyway, applied both patches.
> >>>
> >>> - Arnaldo
> >>>  
> >>>> Signed-off-by: James Clark <james.clark@arm.com>
> >>>> ---
> >>>>  tools/perf/Documentation/perf-config.txt |  4 ++++
> >>>>  tools/perf/tests/builtin-test.c          | 12 ++++++++++++
> >>>>  2 files changed, 16 insertions(+)
> >>>>
> >>>> diff --git a/tools/perf/Documentation/perf-config.txt b/tools/perf/Documentation/perf-config.txt
> >>>> index 0b4e79dbd3f6..16398babd1ef 100644
> >>>> --- a/tools/perf/Documentation/perf-config.txt
> >>>> +++ b/tools/perf/Documentation/perf-config.txt
> >>>> @@ -722,6 +722,10 @@ session-<NAME>.*::
> >>>>  		Defines new record session for daemon. The value is record's
> >>>>  		command line without the 'record' keyword.
> >>>>  
> >>>> +test.*::
> >>>> +
> >>>> +	test.objdump::
> >>>> +		objdump binary to use for disassembly and annotations.
> >>>>  
> >>>>  SEE ALSO
> >>>>  --------
> >>>> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> >>>> index a8d17dd50588..113e92119e1d 100644
> >>>> --- a/tools/perf/tests/builtin-test.c
> >>>> +++ b/tools/perf/tests/builtin-test.c
> >>>> @@ -14,6 +14,7 @@
> >>>>  #include <sys/wait.h>
> >>>>  #include <sys/stat.h>
> >>>>  #include "builtin.h"
> >>>> +#include "config.h"
> >>>>  #include "hist.h"
> >>>>  #include "intlist.h"
> >>>>  #include "tests.h"
> >>>> @@ -514,6 +515,15 @@ static int run_workload(const char *work, int argc, const char **argv)
> >>>>  	return -1;
> >>>>  }
> >>>>  
> >>>> +static int perf_test__config(const char *var, const char *value,
> >>>> +			     void *data __maybe_unused)
> >>>> +{
> >>>> +	if (!strcmp(var, "test.objdump"))
> >>>> +		test_objdump_path = value;
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>  int cmd_test(int argc, const char **argv)
> >>>>  {
> >>>>  	const char *test_usage[] = {
> >>>> @@ -541,6 +551,8 @@ int cmd_test(int argc, const char **argv)
> >>>>          if (ret < 0)
> >>>>                  return ret;
> >>>>  
> >>>> +	perf_config(perf_test__config, NULL);
> >>>> +
> >>>>  	/* Unbuffered output */
> >>>>  	setvbuf(stdout, NULL, _IONBF, 0);
> >>>>  
> >>>> -- 
> >>>> 2.34.1
> >>>>
> >>>
> > 
> 

-- 

- Arnaldo