Thanks Mr. Alex for your suggestions. I will send a v2 of this series
with the updates.
On Wed, Jun 17, 2020 at 2:16 PM Alex Bennée <alex.ben...@linaro.org> wrote:
> You will want the script to be +x if the user is to execute it.
Thanks for the reminder. Forgot to do this before sending the patch.
> > +# Print the top 25 most executed functions in QEMU using callgrind.
> > +# Example Usage:
> > +# callgrind_top_25.py <qemu-build>/x86_64-linux-user/qemu-x86_64
> > executable
>
> Why limit to 25, make the name generic and maybe just default to 25
> unless the user specifies a different option.
Very valid suggestion. Thanks!
>
> I would recommend using:
>
> from argparse import ArgumentParser
>
> from the start as adding options with hand parsing will be a pain. I
> would suggest a specific option for the qemu binary and then using a
> positional argument that can be read after -- so you don't confuse
> options.
>
Great, what do you think of the format below:
topN_callgrind.py -n20 -- /path/to/qemu executable -executable - arguments
> Direct os.system calls are discouraged, you tend to get weird effects
> like:
>
> ../../scripts/performance/callgrind_top_25.py
> ./aarch64-linux-user/qemu-aarch64 ./tests/tcg/aarch64-linux-user/fcvt
> sh: 1: Syntax error: "&" unexpected
> Traceback (most recent call last):
> File "../../scripts/performance/callgrind_top_25.py", line 52, in <module>
> with open('tmp.callgrind.data', 'r') as data:
> FileNotFoundError: [Errno 2] No such file or directory: 'tmp.callgrind.data'
>
> I would:
>
> - check for valgrind in path and fail gracefully if not found
> - use os.subprocess API for launching (with or without the shell)
>
This weird error was because of the space between "&&" and "2>/dev/null"
These were inserted by the autopep8 python formatter before committing.
When this is fixed, everything works fine, but I believe your
suggestion of using the os.subprocess is valid so I will implement it
and also check for valgrind as you've said.
> > +
> > +# Line number with the total number of instructions
> > +number_of_instructions_line = 20
> > +
> > +# Line number with the top function
> > +first_func_line = 25
>
> for example
>
> parser.add_argument('-n', dest="top", type=int, default=25,
> help="Hottest n functions")
Will also use:
parser.add_argument('command', type=str, nargs='+',
help="QEMU invocation to report the top functions for")
To parse all remaining arguments after "--".
> > +# Get the total number of instructions
> > +total_number_of_instructions = int(
> > + callgrind_data[number_of_instructions_line].split('
> > ')[0].replace(',', ''))
>
> There is no harm in having your steps split out a little.
Noted!
> > +# Remove intermediate files
> > +os.system('rm callgrind.data tmp.callgrind.data')
>
> os.unlink()
>
Noted!
