[Ltrace-devel] [PATCH] Add support for using elfutils as unwinder.

[Petr Machata]

Hi,

this looks mostly fine, except for a couple mostly coding-style nits.
I haven't tested yet though.

Mark Wielaard <mjw at redhat.com> writes:

> diff --git a/output.c b/output.c
> index f804cd0..0cbd651 100644
> --- a/output.c
> +++ b/output.c

[...]

> +#if defined(HAVE_LIBDW)
> +static int
> +frame_callback (Dwfl_Frame *state, void *arg)
> +{
> +	Dwarf_Addr pc;
> +	bool isactivation;
> +
> +	int *frames = (int *) arg;
> +
> +	if (! dwfl_frame_pc (state, &pc, &isactivation))

Space before paren.

> +		return -1;
> +
> +	if (! isactivation)
> +		pc--;
> +
> +	Dwfl *dwfl = dwfl_thread_dwfl (dwfl_frame_thread (state));

Spaces.

> +	Dwfl_Module *mod = dwfl_addrmodule (dwfl, pc);

And here.  And many places below, I'll just stop here.

> +	const char *modname = NULL;
> +	const char *symname = NULL;
> +	GElf_Off off = 0;
> +	if (mod) {

This should be mod != NULL.

> +		GElf_Sym sym;
> +		modname = dwfl_module_info(mod, NULL, NULL, NULL, NULL,
> +					   NULL, NULL, NULL);
> +		symname = dwfl_module_addrinfo(mod, pc, &off, &sym,
> +					       NULL, NULL, NULL);
> +	}
> +
> +	/* This mimics the output produced by libunwind below. */

Two spaces after a period.

> +	fprintf(options.output, " > %s(%s+0x%" PRIx64 ") [%" PRIx64 "]\n",
> +		modname, symname, off, pc);
> +
> +	/* If we wanted fancy we could try to extract the source line too...
> +	if (mod) {

!= NULL.

> +		Dwfl_Line *l = dwfl_module_getsrc(mod, pc);
> +		if (l) {

!= NULL.

> +			int line, col;
> +			const char* src;
> +			line = col = -1;
> +			src = dwfl_lineinfo (l, NULL, &line, &col, NULL, NULL);

The definition of SRC can be merged with its initialization.

> +			if (src != NULL) {
> +				fprintf (options.output, "\n\t%s", src);
> +				if (line > 0) {
> +					fprintf (options.output, ":%d", line);
> +					if (col > 0)
> +			                        fprintf (options.output,
> +							":%d", col);
> +				}
> +				fprintf (options.output, "\n");
> +			}
> +
> +		}
> +	}
> +	... not fancy for now. */

I'm not against the fancy FWIW, though ltrace doesn't currently
recognize existence of Dwarf.  Any reason not to include the above code?
Any reason not to drop it altogether then?

> +
> +	/* Max number of frames to print reached? */
> +	if ((*frames)-- == 0)
> +		return DWARF_CB_ABORT;
> +
> +	return DWARF_CB_OK;
> +}
> +#endif /* defined(HAVE_LIBDW) */
> +
>  void
>  output_right(enum tof type, struct process *proc, struct library_symbol *libsym,
>  	     struct timedelta *spent)
> @@ -702,6 +766,17 @@ output_right(enum tof type, struct process *proc, struct library_symbol *libsym,
>  	}
>  #endif /* defined(HAVE_LIBUNWIND) */
>  
> +#if defined(HAVE_LIBDW)
> +	if (options.bt_depth > 0 && proc->leader->dwfl != NULL) {
> +		int frames = options.bt_depth;
> +		if (dwfl_getthread_frames(proc->leader->dwfl, proc->pid,
> +					  frame_callback, &frames) < 0)

Here you rely on the fact that DWARF_CB_ABORT is > 0, which is not
obvious (though likely always true).  But dwfl_getthread_frames doesn't
care about particular value returned from the callback as long as it's
non-zero, so why not change the DWARF_CB_ABORT above to 1?

> +			fprintf(stderr, "dwfl_getthread_frames tid %d: %s\n",
> +				proc->pid, dwfl_errmsg(-1));
> +		fprintf(options.output, "\n");
> +	  }
> +#endif /* defined(HAVE_LIBDW) */
> +
>  	current_proc = NULL;
>  	current_column = 0;
>  }
> diff --git a/proc.c b/proc.c
> index 97bcb60..e89da6e 100644
> --- a/proc.c
> +++ b/proc.c

[...]

>  static int
> @@ -167,6 +172,10 @@ process_bare_init(struct process *proc, const char *filename,
>  	}
>  #endif /* defined(HAVE_LIBUNWIND) */
>  
> +#if defined(HAVE_LIBDW)
> +	proc->dwfl = NULL; /* Initialize for leader only on first library. */

Two spaces after period.

[...]

> +			if (leader->dwfl == NULL) {
> +				int r;
> +				r = dwfl_linux_proc_attach (dwfl, leader->pid,
> +							    true);

You can merge the above two lines.

Thanks,
PM