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

[Mark Wielaard <mjw at redhat dot com>]

On Fri, 2013-12-13 at 18:04 +0100, Jan Kratochvil wrote:
> 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?

The current interfaces are for enumerating all symbols and for matching
an address to an associated symbol. A new interface would be for
matching a name to either an address and/or a symbol. This name could
match on symbol names or not. We would probably also have a search table
for it to speed up such lookups.

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

I can see how that makes things difficult for our discussions. It really
helps to think of user tools output and the interfaces for the libraries
as different things. They really are different abstraction layers.

> > +	      /* 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.

Yeah, you are right. The goto is ugly.
Rewritten as an inline nested function instead.

> > +      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/

eep. typo. Thanks, fixed.

> > +		{
> > +		  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.

Yes, yes. If ELF == NULL then gelf_getehdr will return NULL.
Added a check just in case.

Also merged this with my original code.

Thanks,

Mark