Re: [PATCH] plugins: add plugin API to get args passed to binary

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 11/4/24 19:31, Demin Han wrote:
> 
> 
>> -----Original Message-----
>> From: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> Sent: 2024年11月5日 10:50
>> To: Demin Han <demin.han@starfivetech.com>; qemu-devel@nongnu.org
>> Cc: alex.bennee@linaro.org; erdnaxe@crans.org; ma.mandourr@gmail.com
>> Subject: Re: [PATCH] plugins: add plugin API to get args passed to binary
>>
>> On 11/4/24 18:29, Demin Han wrote:
>>>
>>>> -----Original Message-----
>>>> From: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>> Sent: 2024年11月5日 5:22
>>>> To: Demin Han <demin.han@starfivetech.com>; qemu-devel@nongnu.org
>>>> Cc: alex.bennee@linaro.org; erdnaxe@crans.org;
>> ma.mandourr@gmail.com
>>>> Subject: Re: [PATCH] plugins: add plugin API to get args passed to
>>>> binary
>>>>
>>>> On 11/1/24 22:10, Demin Han wrote:
>>>>> Hi,
>>>>>
>>>>> Many benchmarks have their own build and run system, such as
>>>>> specint, we don’t want to change their code.
>>>>>
>>>>
>>>> I don't think those benchmarks (such as specint) integrate calling
>>>> qemu with a specific plugin on command line, so I guess you have a
>>>> wrapper or something where you could pass necessary information, or
>>>> tweak output file, without changing the benchmark itself. In case I'm
>> wrong, feel free to correct me.
>>>
>>> I have two methods without change test code, but they can't get args
>> passed to binary:
>>> 1. for those without hook, such as specint, we can utilize binfmt_misc.
>>>     We may need a simple wrapper just to load plugin or set some common
>>> options and register this wrapper to binfmt_misc 2. for those with
>>> hook, such as llvm-test-suite, we can set TEST_SUITE_RUN_UNDER or
>>> utilize binfmt_misc
>>>
>>> I have no idea to write a wrapper which can get args passed to binary
>> without change code.
>>> If have, please give a example or some hint.
>>>
>>
>> If you use binfmt_misc, how are you passing the argument to use a plugin?
>   qemu-riscv64.conf:
> 	:qemu-riscv64:M::\x7fELF\x02\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\xf3\x00:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff:/opt/bin/qemu-riscv64.sh:
>   qemu-riscv64.sh:
> 	#!/bin/bash
>      /opt/bin/qemu-riscv64 --plugin /opt/bin/libxxx.so -L xxx $@
>   
>   We have no information about binary to pass.
> 

It seems that you already have a wrapper script (qemu-risv64.sh). You 
can replace it and read command line from $@, which contains the command 
passed to it.

>> In the case of llvm-test-suite, you can set TEST_SUITE_RUN_UNDER to a
>> wrapper adding a specific plugin, its options, and generating the log filename
>> as expected.
> 
> We cant set TEST_SUITE_RUN_UNDER="qemu-riscv64 --plugin /xxx/libxxx.so -L xxx".
> We have no any information about binary under test even binary name.
> We can get binary name in plugin using qemu_plugin_path_to_binary, but can't solve the issue mentioned in commit msg.
> 

If you pass a script instead, calling the same command, you can get 
access to command line by reading the content of $@.

> 
>>>>> Actually the log maybe structural data such as in json format and
>>>>> may be output multiple log files with different statistics dimention for one
>> run.
>>>>>
>>>>> -D can’t satisfy this.
>>>>
>>>> Indeed, it can output only a single file. If your plugin needs
>>>> something more advanced, you can try to output something yourself.
>>>> However, a better and simpler way would be to prefix lines output
>>>> with a specific marker, and post process your plugin trace with a custom
>> script.
>>>>
>>>> Adding command line access to plugins does not solve any of those
>> problems.
>>>>
>>>> I see value in what this series offer, but I don't see how it's
>>>> related to the current need you express.
>>>
>>> Yes, this is not important and not most concerned.
>>> But if we can directly output json or yaml, it would be convenient for
>> post-processing.
>>> This is a bonus for this added api.
>>>
>>
>> It's something you can do directly in the plugin, by outputing json/yaml, or any
>> format wanted. It's probably not a feature we'll provide in the public API as
>> it's a very specific need.
>>
>>> Regards,
>>> Demin
>>>
>>>> Regards,
>>>> Pierrick
>>>>
>>>>>
>>>>> Regard,
>>>>> Denin
>>>>>
>>>>> 获取 Outlook for iOS <https://aka.ms/o0ukef>
>>>>> --------------------------------------------------------------------
>>>>> --
>>>>> --
>>>>> *发件人:* Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>> *发送时间:* Saturday, November 2, 2024 2:18:20 AM
>>>>> *收件人:* Demin Han <demin.han@starfivetech.com>;
>>>> qemu-devel@nongnu.org
>>>>> <qemu-devel@nongnu.org>
>>>>> *抄送:* alex.bennee@linaro.org <alex.bennee@linaro.org>;
>>>>> erdnaxe@crans.org <erdnaxe@crans.org>; ma.mandourr@gmail.com
>>>>> <ma.mandourr@gmail.com>
>>>>> *主题:* Re: [PATCH] plugins: add plugin API to get args passed to
>>>>> binary Hi Demin,
>>>>>
>>>>> thanks for your contribution.
>>>>>
>>>>> On 11/1/24 02:00, demin.han wrote:
>>>>>> Why we need args?
>>>>>> When plugin outputs log files, only binary path can't distinguish
>>>>>> multiple runs if the binary passed with different args.
>>>>>> This is bad for CI using plugin.
>>>>>>
>>>>>
>>>>> Can it be solved simply by encoding this in name of log file from
>>>>> the CI run script?
>>>>> $ cmd="/usr/bin/echo Hello world"
>>>>> $ out_file="$(echo "$cmd" | sed -e 's/\s/_/').log"
>>>>> $ qemu -plugin... -d plugin -D "$out_file" $cmd
>>>>>
>>>>> I can see some good points to add this new API, but for the use case
>>>>> presented in commit message, I'm not sure to see what it solves.
>>>>>
>>>>>> Signed-off-by: demin.han <demin.han@starfivetech.com>
>>>>>> ---
>>>>>>      include/qemu/qemu-plugin.h   | 11 +++++++++++
>>>>>>      plugins/api.c                | 16 ++++++++++++++++
>>>>>>      plugins/qemu-plugins.symbols |  1 +
>>>>>>      3 files changed, 28 insertions(+)
>>>>>>
>>>>>> diff --git a/include/qemu/qemu-plugin.h
>>>>>> b/include/qemu/qemu-plugin.h index 622c9a0232..daf75c9f5a 100644
>>>>>> --- a/include/qemu/qemu-plugin.h
>>>>>> +++ b/include/qemu/qemu-plugin.h
>>>>>> @@ -837,6 +837,17 @@ bool qemu_plugin_bool_parse(const char
>> *name,
>>>>>> const char *val, bool *ret);
>>>>>>      QEMU_PLUGIN_API
>>>>>>      const char *qemu_plugin_path_to_binary(void);
>>>>>>
>>>>>> +/**
>>>>>> + * qemu_plugin_argv_to_binary() - argv to binary file being
>>>>>> +executed
>>>>>> + *
>>>>>> + * Return a string array representing the argv to the binary. For
>>>>>> +user-mode
>>>>>> + * this is the main executable's argv. For system emulation we
>>>>>> +currently
>>>>>> + * return NULL. The user should g_free() the string array once no
>>>>>> +longer
>>>>>> + * needed.
>>>>>> + */
>>>>>> +QEMU_PLUGIN_API
>>>>>> +const char **qemu_plugin_argv_to_binary(void);
>>>>>> +
>>>>>>      /**
>>>>>>       * qemu_plugin_start_code() - returns start of text segment
>>>>>>       *
>>>>>> diff --git a/plugins/api.c b/plugins/api.c  index
>>>>>> 24ea64e2de..fa2735db03 100644
>>>>>> --- a/plugins/api.c
>>>>>> +++ b/plugins/api.c
>>>>>> @@ -485,6 +485,22 @@ const char
>> *qemu_plugin_path_to_binary(void)
>>>>>>          return path;
>>>>>>      }
>>>>>>
>>>>>> +const char **qemu_plugin_argv_to_binary(void)
>>>>>> +{
>>>>>> +    const char **argv = NULL;
>>>>>> +#ifdef CONFIG_USER_ONLY
>>>>>> +    int i, argc;
>>>>>> +    TaskState *ts = get_task_state(current_cpu);
>>>>>> +    argc = ts->bprm->argc;
>>>>>> +    argv = g_malloc(sizeof(char *) * (argc + 1));
>>>>>> +    for (i = 0; i < argc; ++i) {
>>>>>> +        argv[i] = g_strdup(ts->bprm->argv[i]);
>>>>>> +    }
>>>>>> +    argv[argc] = NULL;
>>>>>> +#endif
>>>>>> +    return argv;
>>>>>> +}
>>>>>> +
>>>>>>      uint64_t qemu_plugin_start_code(void)
>>>>>>      {
>>>>>>          uint64_t start = 0;
>>>>>> diff --git a/plugins/qemu-plugins.symbols
>>>>>> b/plugins/qemu-plugins.symbols  index 032661f9ea..532582effe
>> 100644
>>>>>> --- a/plugins/qemu-plugins.symbols
>>>>>> +++ b/plugins/qemu-plugins.symbols
>>>>>> @@ -1,4 +1,5 @@
>>>>>>      {
>>>>>> +  qemu_plugin_argv_to_binary;
>>>>>>        qemu_plugin_bool_parse;
>>>>>>        qemu_plugin_end_code;
>>>>>>        qemu_plugin_entry_code;
>>>>>
>>>>> Regards,
>>>>> Pierrick