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

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

Hi Mark,

On Wed, 04 Dec 2013 20:02:33 +0100, Mark Wielaard wrote:
> As I said before I don't think the ABI for object file link editor
> really matters much in the case of the libdwfl interfaces.

I have written my comment at the libdwfl/dwfl_module_getsym.c patch below.


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.


[...]
> --- 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.


> +    {
> +      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.
This is corrupting existing ELF symbols.

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".


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


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


> +
> +	* Makefile.am (EXTRA_DIST): Add testfile66.bz2 and testfile66.core.bz2.
> +	* dwflsyms.c (list_syms): Remove unused from parameter mod_name.  Print
> +	error on dwfl_module_getsymtab error.
> +	* run-addrname-test.sh (testfile66, testfile66.core): New tests.
> +	* run-dwflsyms.sh (testfile66, testfile66.core, hello_ppc64.ko): New
> +	tests.
> +	* testfile66.bz2: New file.
> +	* testfile66.core.bz2: New file.
> +
>  2013-12-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
>  
>  	* Makefile.am (check_PROGRAMS): Add backtrace, backtrace-child,


Thanks,
Jan