This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] stack: show binary and source file names where a function is defined


On Mon, 2013-12-16 at 15:55 +0900, Masatake YAMATO wrote:
> This patch added the binary file name where the function is
> defined when -v option is given, and the source file name
> where the function is defined when -v -v options are given.

Cool. I like this idea.

But I think it would be better to have separate options so the user can
precisely say what they want. So one -m, --modules option to add the
module name and one -s, --source option to add the source name. -v,
--verbose would then always be the most verbose output possible (and
enable all extra info).

> This is based on private discussion with Jan Kratochvil
> <jan.kratochvil@redhat.com>.
> 
> Signed-off-by: Masatake YAMATO <yamato@redhat.com>
> ---
>  src/ChangeLog |  9 +++++++++
>  src/stack.c   | 26 ++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ChangeLog b/src/ChangeLog
> index f899858..3bf2426 100644
> --- a/src/ChangeLog
> +++ b/src/ChangeLog
> @@ -1,3 +1,12 @@
> +2013-12-16  Masatake YAMATO  <yamato@redhat.com>
> +
> +	* stack.c (verbose): Change the type to int.
> +	(parse_opt): Increment verbose for each time when
> +	'-v' is found.
> +	(main): Added '-v' to the help message.
> +	(frame_callback): Print binary and source file name
> +	if verbose > 0.
> +
>  2013-11-25  Petr Machata  <pmachata@redhat.com>
>  
>  	* elflint.c (valid_e_machine): Add EM_AARCH64.
> diff --git a/src/stack.c b/src/stack.c
> index f428ed0..edb479f 100644
> --- a/src/stack.c
> +++ b/src/stack.c
> @@ -36,7 +36,7 @@ ARGP_PROGRAM_VERSION_HOOK_DEF = print_version;
>  /* Bug report address.  */
>  ARGP_PROGRAM_BUG_ADDRESS_DEF = PACKAGE_BUGREPORT;
>  
> -static bool verbose = false;
> +static int verbose = 0;
>  
>  static int
>  frame_callback (Dwfl_Frame *state, void *arg)
> @@ -78,7 +78,25 @@ frame_callback (Dwfl_Frame *state, void *arg)
>    printf ("#%-2u 0x%0*" PRIx64, (*framenop)++, width, (uint64_t) pc);
>    if (verbose)
>      printf ("%4s", ! isactivation ? "- 1" : "");
> -  printf (" %s\n", symname);
> +  printf (" %s", symname);
> +  if (verbose > 0)
> +    {
> +      const char* fname;
> +      Dwfl_Line * lineobj;
> +      int line, col;
> +      const char* sname;
> +
> +      fname = dwfl_module_info(mod, NULL, NULL, NULL, NULL, NULL, NULL, NULL);
> +      printf (" - %s", fname? fname: "?");

There are cases where dwfl_module_info returns the empty string "". In
that case it is nice to provide the basename of the mainfile argument
returned by dwfl_module_info instead.

> +      if (verbose > 1)
> +	{
> +	  lineobj = dwfl_module_getsrc(mod, pc_adjusted);
> +	  line = col = -1;
> +	  sname = lineobj? (dwfl_lineinfo(lineobj, NULL, &line, &col, NULL, NULL)?: "?"): "?";
> +	  printf(" (%s:%d:%d)", sname, line, col);
> +	}

Since line and/or col can be zero or minus one when they don't exist I
don't think you should print them then. Only print the sname:line when
line <= 0 and only print sname:line:col when col <= 0.

You could also look at how the src/addr2line.c print_src function
formats source lines, but that would introduce two more options
(basename and compdir), which might or might not be overkill.

Thanks,

Mark



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]