Re: [PATCH] trace-cmd: [POC] Add APIs for extracting DWARF information from a file

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 21 May 2020 13:55:19 +0300
"Tzvetomir Stoyanov (VMware)" <tz.stoyanov@gmail.com> wrote:


Hi Tzvetomir,

Thanks for doing this.

> Added initial infrastructure and trace-cmd library APIs for working with
> DWARF debug information:
>  trace_obj_debug_create()
>  trace_obj_debug_destroy()
>  trace_obj_debug_get_fileoffset()
> These APIs utilize libdwarf and libbfd for parsing ELF and DWARF headers
> of the files. The trace_obj_debug_get_fileoffset() API retrieves the
> offset in the file of the given functions. These offsets can be used to
> set a uprobes to the functions.
> 
> Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> ---
>  Makefile                        |  12 ++
>  include/trace-cmd/trace-cmd.h   |  14 ++
>  lib/trace-cmd/Makefile          |  17 ++
>  lib/trace-cmd/trace-obj-debug.c | 340 ++++++++++++++++++++++++++++++++
>  4 files changed, 383 insertions(+)
>  create mode 100644 lib/trace-cmd/trace-obj-debug.c
> 
> diff --git a/Makefile b/Makefile
> index 2f9620e4..d737b588 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -243,6 +243,18 @@ endif
>  CUNIT_INSTALLED := $(shell if (echo -e "\#include <CUnit/Basic.h>\n void main(){CU_initialize_registry();}" | $(CC) -x c -lcunit - >/dev/null 2>&1) ; then echo 1; else echo 0 ; fi)
>  export CUNIT_INSTALLED
>  
> +DWARF_INSTALLED := $(shell if (echo -e "\#include <libdwarf/libdwarf.h>\n void main(){dwarf_init(-1, 0, 0, 0, 0, 0);}" | $(CC) -xc - -ldwarf >/dev/null 2>&1) ; then echo 1; else echo 0 ; fi)
> +export DWARF_INSTALLED
> +BFD_INSTALLED := $(shell if (echo -e "\#include <bfd.h>\n void main(){bfd_init();}" | $(CC) -xc  - -lbfd >/dev/null 2>&1) ; then echo 1; else echo 0 ; fi)
> +export BFD_INSTALLED
> +
> +ifeq ($(BFD_INSTALLED), 1)
> +LIBS += -lbfd
> +endif
> +ifeq ($(DWARF_INSTALLED), 1)
> +LIBS += -ldwarf
> +endif
> +
>  export CFLAGS
>  export INCLUDES
>  
> diff --git a/include/trace-cmd/trace-cmd.h b/include/trace-cmd/trace-cmd.h
> index f3c95f30..5ad75b30 100644
> --- a/include/trace-cmd/trace-cmd.h
> +++ b/include/trace-cmd/trace-cmd.h
> @@ -483,6 +483,20 @@ void tracecmd_plog(const char *fmt, ...);
>  void tracecmd_plog_error(const char *fmt, ...);
>  int tracecmd_set_logfile(char *logfile);
>  
> +/* --- Object debug --- */
> +struct trace_obj_debug;

Add a space here. It's fine to butt single lines declarations together,
but lets keep full struct declarations separated.

> +struct trace_obj_symbols {
> +	struct trace_obj_symbols *next;
> +	char *name;	/* symbol name */
> +	unsigned long long vma; /* symbol virtual memory address */
> +	unsigned long long foffset; /* symbol file offset */
> +};
> +
> +struct trace_obj_debug *trace_obj_debug_create(char *file);
> +void trace_obj_debug_destroy(struct trace_obj_debug *obj);
> +int trace_obj_debug_get_fileoffset(struct trace_obj_debug *obj,
> +				   struct trace_obj_symbols *symbols);

Are these something we want in the tracecmd library? If so they should
have a "tracecmd_" prefix and not "trace_". We use the "trace_" prefix
for internal functions that are not part of the libraries API.

> +
>  /* --- System --- */
>  unsigned long long tracecmd_generate_traceid(void);
>  int tracecmd_count_cpus(void);
> diff --git a/lib/trace-cmd/Makefile b/lib/trace-cmd/Makefile
> index ab7440ac..72d3864a 100644
> --- a/lib/trace-cmd/Makefile
> +++ b/lib/trace-cmd/Makefile
> @@ -22,6 +22,16 @@ OBJS += trace-timesync.o
>  OBJS += trace-blk-hack.o
>  OBJS += trace-ftrace.o
>  
> +ifeq ($(BFD_INSTALLED), 1)
> +ifeq ($(DWARF_INSTALLED), 1)
> +OBJS += trace-obj-debug.o
> +else
> +$(warning libdwarf is not installed)
> +endif
> +else
> +$(warning libbfd is not installed)
> +endif
> +
>  OBJS := $(OBJS:%.o=$(bdir)/%.o)
>  DEPS := $(OBJS:$(bdir)/%.o=$(bdir)/.%.d)
>  
> @@ -38,6 +48,13 @@ $(bdir)/libtracecmd.a: $(OBJS)
>  
>  LIBS = -L$(obj)/lib/traceevent -ltraceevent
>  
> +ifeq ($(BFD_INSTALLED), 1)
> +LIBS += -lbfd
> +endif
> +ifeq ($(DWARF_INSTALLED), 1)
> +LIBS += -ldwarf
> +endif
> +
>  $(bdir)/libtracecmd.so: $(OBJS)
>  	$(Q)$(call do_compile_shared_library)
>  
> diff --git a/lib/trace-cmd/trace-obj-debug.c b/lib/trace-cmd/trace-obj-debug.c
> new file mode 100644
> index 00000000..e98185a4
> --- /dev/null
> +++ b/lib/trace-cmd/trace-obj-debug.c
> @@ -0,0 +1,340 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +/*
> + * Copyright (C) 2009, 2010 Red Hat Inc, Steven Rostedt <srostedt@redhat.com>

This file looks completely written by you. You can remove my name from it.

> + * Copyright (C) 2020, VMware, Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
> + *
> + */
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <dwarf.h>
> +#include <libdwarf/libdwarf.h>
> +#include <bfd.h>
> +#include "trace-cmd-local.h"
> +#include "trace-cmd.h"
> +
> +struct trace_obj_debug {
> +	Dwarf_Debug dwarf;
> +	bfd *bfd;
> +};
> +
> +static void dwarf_error_handler(Dwarf_Error error, Dwarf_Ptr errarg)
> +{
> +	warning("\nlibdwarf error detected: 0x%" DW_PR_DUx " %s\n",
> +		dwarf_errno(error), dwarf_errmsg(error));
> +}
> +
> +static void get_addr(Dwarf_Attribute attr, Dwarf_Addr *val)
> +{
> +	Dwarf_Error error = 0;
> +	Dwarf_Addr uval = 0;
> +	int res;
> +
> +	res = dwarf_formaddr(attr, &uval, &error);
> +	if (res == DW_DLV_OK)
> +		*val = uval;

Hmm, shouldn't this function return a success or failure? Or does it
not matter?

> +}
> +
> +static void dwarf_get_subprog(Dwarf_Debug dbg, Dwarf_Die die,
> +			      struct trace_obj_symbols *data, const char *name)
> +{
> +	Dwarf_Attribute *attrbuf = 0;
> +	Dwarf_Signed attrcount = 0;
> +	Dwarf_Error error = 0;
> +	Dwarf_Addr lowpc = 0;
> +	Dwarf_Half aform;
> +	Dwarf_Signed i;
> +	int res;
> +
> +	res = dwarf_attrlist(die, &attrbuf, &attrcount, &error);
> +	if (res != DW_DLV_OK)
> +		return;

This function should probably return success or failure.


> +	for (i = 0; i < attrcount ; ++i) {
> +		res = dwarf_whatattr(attrbuf[i], &aform, &error);
> +		if (res == DW_DLV_OK) {
> +			if (aform == DW_AT_low_pc)
> +				get_addr(attrbuf[i], &lowpc);

Hmm, what if we get more than one match? Should we break out of the
loop on a match?

> +		}
> +		dwarf_dealloc(dbg, attrbuf[i], DW_DLA_ATTR);
> +	}
> +
> +	while (data) {
> +		if (lowpc && !strcmp(name, data->name))
> +			data->vma = lowpc;

Is it possible to have more than one match here too?

> +		data = data->next;
> +	}
> +
> +	dwarf_dealloc(dbg, attrbuf, DW_DLA_LIST);
> +}
> +
> +
> +static void dwarf_get_die_data(Dwarf_Debug dbg, Dwarf_Die die,
> +			       struct trace_obj_symbols *data)

This should probably return a status as well.

> +{
> +	const char *formname = 0;
> +	Dwarf_Attribute attr = 0;
> +	const char *tagname = 0;
> +	Dwarf_Half formnum = 0;
> +	Dwarf_Error error = 0;
> +	Dwarf_Half tag = 0;
> +	char *name = 0;

Pointers should be assigned to NULL not 0.

> +	int res = 0;
> +
> +	res = dwarf_diename(die, &name, &error);
> +	if (res == DW_DLV_ERROR) {
> +		warning("Error in dwarf_diename: %s\n", dwarf_errmsg(error));
> +		return;
> +	}
> +	if (res == DW_DLV_NO_ENTRY)
> +		name = "<no DW_AT_name attr>";
> +
> +	res = dwarf_tag(die, &tag, &error);
> +	if (res != DW_DLV_OK) {
> +		warning("Error in dwarf_tag: %s n", dwarf_errmsg(error));
> +		return;
> +	}
> +	res = dwarf_get_TAG_name(tag, &tagname);
> +	if (res != DW_DLV_OK) {
> +		warning("Error in dwarf_get_TAG_name: %s\n", dwarf_errmsg(error));
> +		return;
> +	}
> +
> +	res = dwarf_attr(die, DW_AT_name, &attr, &error);
> +	if (res == DW_DLV_OK) {
> +		res = dwarf_whatform(attr, &formnum, &error);
> +		if (res != DW_DLV_OK) {
> +			warning("Error in dwarf_whatform: %s\n",
> +				dwarf_errmsg(error));
> +			return;
> +		}
> +		formname = "form-name-unavailable";
> +		res = dwarf_get_FORM_name(formnum, &formname);
> +		if (res != DW_DLV_OK)
> +			formname = "UNKNoWn FORM!";
> +		dwarf_dealloc(dbg, attr, DW_DLA_ATTR);
> +	}
> +
> +	if (tag == DW_TAG_subprogram)
> +		dwarf_get_subprog(dbg, die, data, name);
> +}

The rest looks good. I'll play a little more with it.

-- Steve