Re: [patchv4 4/4] Provide virtual symbols for ppc64 function descriptors

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Thu, 12 Dec 2013 16:07:30 +0100, Mark Wielaard wrote:
> Then the only feature you seem to want that is missing is a real search
> on name or identifier. Where the identifier might have some modifier(s)
> that have a possible arch specific meaning that matches it to some
> address and/or some other properties. I think that needs its own
> specific interface though.

When there will be a different interface for it then why / in which cases
would anyone use the current new interface?


> And it also doesn't seem immediately needed right now (in contrast with the
> address lookups which make the unwinder more useful).

I repeat myself but:

elfutils will output function descriptor identifiers for code addresses.
This is confusing to any ppc64 users used for many years for the standardized
naming.  IMO even matters the naming is moreover contained in ppc64 ABI.

I really do not understand which API should do what and all the politics about
APIs.  Just the end-user output matters to me.


> --- a/libdwfl/ChangeLog
> +++ b/libdwfl/ChangeLog
> @@ -1,3 +1,8 @@
> +2013-12-12  Mark Wielaard  <mjw@redhat.com>
> +
> +	* dwfl_module_addrsym.c (__libdwfl_addrsym): Search for both value
> +	and sym.st_value (adjusted) when different.
> +
>  2013-12-11  Mark Wielaard  <mjw@redhat.com>
>  
>  	* derelocate.c (__libdwfl_find_section_ndx): New internal function.
> diff --git a/libdwfl/dwfl_module_addrsym.c b/libdwfl/dwfl_module_addrsym.c
> index 5a72de1..22746c6 100644
> --- a/libdwfl/dwfl_module_addrsym.c
> +++ b/libdwfl/dwfl_module_addrsym.c
> @@ -128,6 +128,7 @@ __libdwfl_addrsym (Dwfl_Module *mod, GElf_Addr addr, GElf_Off *off,
>  	      /* Even if we don't choose this symbol, its existence excludes
>  		 any sizeless symbol (assembly label) that is below its upper
>  		 bound.  */
> +	    try_value:
>  	      if (value + sym.st_size > min_label)
>  		min_label = value + sym.st_size;
>  
> @@ -197,6 +198,20 @@ __libdwfl_addrsym (Dwfl_Module *mod, GElf_Addr addr, GElf_Off *off,
>  		      closest_name = name;
>  		    }
>  		}
> +
> +	      /* If this is an addrinfo variant and the value could be
> +		 resolved then also try matching the (adjusted) st_value.  */
> +	      if (! adjust_st_value && mod->e_type != ET_REL)
> +		{
> +		  GElf_Addr adjusted_st_value;
> +		  adjusted_st_value = dwfl_adjusted_st_value (mod, elf,
> +							      sym.st_value);
> +		  if (value != adjusted_st_value)
> +		    {
> +		      value = adjusted_st_value;
> +		      goto try_value;

I would rather move the code into a function and call it twice, instead of the
goto.  But fine even with that goto.


> +		    }
> +		}
>  	    }
>  	}
>      }
> diff --git a/src/ChangeLog b/src/ChangeLog
> index 9d7cd40..cddb632 100644
> --- a/src/ChangeLog
> +++ b/src/ChangeLog
> @@ -1,3 +1,10 @@
> +2013-12-12  Mark Wielaard  <mjw@redhat.com>
> +
> +	* addr2line.c (options): Add symbol-sections, 'x'.
> +	(show_symbol_sections): New static bool.
> +	(parse_opt): Handle 'x'.
> +	(print_addrsym): Also show section of address.
> +
>  2013-12-11  Mark Wielaard  <mjw@redhat.com>
>  
>  	* addr2line.c (print_addrsym): Use dwfl_module_addrinfo.
> diff --git a/src/addr2line.c b/src/addr2line.c
> index 2e541ac..c7e4cb0 100644
> --- a/src/addr2line.c
> +++ b/src/addr2line.c
> @@ -61,6 +61,7 @@ static const struct argp_option options[] =
>      N_("Show absolute file names using compilation directory"), 0 },
>    { "functions", 'f', NULL, 0, N_("Also show function names"), 0 },
>    { "symbols", 'S', NULL, 0, N_("Also show symbol or section names"), 0 },
> +  { "symbols-sections", 'x', NULL, 0, N_("Also show symbol and the section names"), 0 },
>    { "flags", 'F', NULL, 0, N_("Also show line table flags"), 0 },
>    { "section", 'j', "NAME", 0,
>      N_("Treat addresses as offsets relative to NAME section."), 0 },
> @@ -114,6 +115,9 @@ static bool show_functions;
>  /* True if ELF symbol or section info should be shown.  */
>  static bool show_symbols;
>  
> +/* True if section associated with a symbol address should be shown.  */
> +static bool show_symbol_sections;
> +
>  /* If non-null, take address parameters as relative to named section.  */
>  static const char *just_section;
>  
> @@ -234,6 +238,11 @@ parse_opt (int key, char *arg, struct argp_state *state)
>        show_symbols = true;
>        break;
>  
> +    case 'x':
> +      show_symbols = true;
> +      show_symbol_sections = true;
> +      break;
> +
>      case 'j':
>        just_section = arg;
>        break;
> @@ -355,10 +364,34 @@ print_addrsym (Dwfl_Module *mod, GElf_Addr addr)
>        else
>  	printf ("(%s)+%#" PRIx64 "\n", name, addr);
>      }
> -  else if (off == 0)
> -    puts (name);
>    else
> -    printf ("%s+%#" PRIx64 "\n", name, off);
> +    {
> +      if (off == 0)
> +	printf ("%s", name);
> +      else
> +	printf ("%s+%#" PRIx64 "", name, off);
> +
> +      // Also show section name for address.
> +      if (show_symbol_sections)
> +	{
> +	  Dwarf_Addr ebias;
> +	  Elf_Scn *scn = dwfl_module_address_section (mod, &addr, &ebias);
> +	  if (scn != NULL)
> +	    {
> +	      GElf_Shdr shdr_mem;
> +	      GElf_Shdr *shdr = gelf_getshdr (scn, &shdr_mem);
> +	      if (scn != NULL)

s/scn/shdr/

> +		{
> +		  Elf *elf = dwfl_module_getelf (mod, &ebias);

It is improbable but I miss here a NULL check.

> +		  GElf_Ehdr ehdr;
> +		  gelf_getehdr (elf, &ehdr);

It is improbable but I miss here a NULL check.

> +		  printf (" (%s)", elf_strptr (elf, ehdr.e_shstrndx,
> +					       shdr->sh_name));
> +		}
> +	    }
> +	}
> +      puts ("");
> +    }
>  }
>  
>  static void
[...]


Jan