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@xxxxxxx>
>>>> ---
>>>>  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
>>>>
>>>
>