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

[Mark Wielaard <mjw at redhat dot com>]

On Wed, 2013-12-04 at 21:26 +0100, Jan Kratochvil wrote: 
> As I see you mean it seriously I would find correct to at least wait with this
> patch check-in until IBM ABI authority will comment it.  I understand you
> think it is unrelated to ABI - but this is the subject of our disagreement.

Of course I mean it seriously. I think your solution is too complex and
mixes different goals causing your patch to do too much. Which is why I
am trying to provide a different solution.

Please try to focus on the actual elfutils/libdwfl interfaces and
functionality we want to provide. Asking imaginary outside authorities
for help if we don't agree on something is just silly.

> [...]
> > --- a/libdwfl/dwfl_module_getsym.c
> > +++ b/libdwfl/dwfl_module_getsym.c
> > @@ -113,6 +113,33 @@ dwfl_module_getsym_elf (Dwfl_Module *mod, int ndx,
> >        alloc = unlikely (shdr == NULL) || (shdr->sh_flags & SHF_ALLOC);
> >      }
> >  
> > +  /* In case of an value in an allocated section the main Elf Ebl
> > +     might know where the real value is (e.g. for function
> > +     descriptors).  */
> > +  if (mod->e_type != ET_REL && alloc
> > +      && GELF_ST_TYPE (sym->st_info) == STT_FUNC)
> 
> I think STT_GNU_IFUNC is missing here.

Yes, if we are looking at ELFOSABI_LINUX.

> > +    {
> > +      if (likely (__libdwfl_module_getebl (mod) == DWFL_E_NOERROR))
> > +	{
> > +	  GElf_Addr value = sym->st_value;
> > +	  if (elf != mod->main.elf)
> > +	    {
> > +	      value = dwfl_adjusted_st_value (mod, elf, value);
> > +	      value = dwfl_deadjust_st_value (mod, mod->main.elf, value);
> > +	    }
> > +
> > +	  if (ebl_resolve_sym_value (mod->ebl, &value))
> > +	    {
> > +	      elf = mod->main.elf;
> > +	      sym->st_value = value;
> > +
> > +	      value = dwfl_adjusted_st_value (mod, mod->main.elf, value);
> > +	      shndx = __libdwfl_find_section_ndx (mod, &value);
> > +	      sym->st_shndx = shndx > SHN_HIRESERVE ? SHN_XINDEX : shndx;
> > +	    }
> > +	}
> > +    }
> > +
> 
> I am fine with the patch as long as you remove this patch chunk.

grin. This is the actual patch :) The rest is just helper functions.
It is the main difference between mine and your approach. In your
approach there is an extra synthetic symbol table, in my approach the
existing symbols are filtered.

> This is corrupting existing ELF symbols.

Sigh. I can explain again that we are already "corrupting" the symbols
since we are already adjusting the values anyway to map them to the
address space we chose for the Dwfl_Module. All this patch does is make
sure the value is also adjusted for any arch backend indirection. But we
went over this multiple times already.

What you seem to be saying is that you would also like a new interface
that returns "pure" GElf_Syms that aren't adjusted in any way from how
they were read from the underlying Elf file. That is not a bad request,
but you didn't provide a convincing reason why you needed that new
interface. I can certainly see why you might also want that, but as far
as I can see nobody is actually needing it right now. But...

> It would be OK to create new libdwfl API with such behavior but it must not be
> called dwfl_module_getsym* as what this code returns is no longer "sym".

I think we will have to introduce a new interface anyway. Not because of
your notion of "ELF symbol corruption", but because it is an
incompatible change however we implement it. I had hoped that my simpler
approach would not cause problems for systemtap, but it looks like stap
is just making certain assumptions that can only be satisfied with the
original code. So it will be better to invent a new name. I think a new
variant on dwfl_module_getsym* is fine though, but please suggest a
different one if you have a good idea.

It is just a pity since I think it will be a bit confusing to have
slightly different functions doing essentially the same thing, except in
the case of one specific arch (I guess ia64 would be another). The new
ones would be what everybody who wants a name/address lookup in the
normal case. But keeping the old ones around would provide you with an
alternative "non-resolving" lookup if that is what is wanted. And in the
new case we could just never adjust or corrupt the GElf_Sym, and provide
the value or offset separately, which would give your "pure" symbol
tables as a bonus.

> >    if (shndxp != NULL)
> >      /* Yield -1 in case of a non-SHF_ALLOC section.  */
> >      *shndxp = alloc ? shndx : (GElf_Word) -1;
> [...]
> > --- a/libebl/libebl.h
> > +++ b/libebl/libebl.h
> > @@ -402,6 +402,12 @@ extern bool ebl_set_initial_registers_tid (Ebl *ebl,
> >  extern size_t ebl_frame_nregs (Ebl *ebl)
> >    __nonnull_attribute__ (1);
> >  
> > +/* Returns true if the value can be resolved to an address in an
> > +   allocated section, which will be returned in *SHNDXP.
> 
> s/SHNDXP/ADDR/

Fixed. 

> > +   (e.g. function descriptor resolving).  */
> > +extern bool ebl_resolve_sym_value (Ebl *ebl, GElf_Addr *addr)
> > +   __nonnull_attribute__ (2);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> [...]
> > diff --git a/tests/ChangeLog b/tests/ChangeLog
> > index 9bd2fe8..0c881e8 100644
> > --- a/tests/ChangeLog
> > +++ b/tests/ChangeLog
> > @@ -1,3 +1,14 @@
> > +2013-11-30  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> You should be written as coauthor here as you modified the files.

Added.

Cheers,

Mark