[v2,00/15] Fixing GNU ifunc support

Message ID 20180325191943.8246-1-palves@redhat.com
Headers show
Series
  • Fixing GNU ifunc support
Related show

Message

Pedro Alves March 25, 2018, 7:19 p.m.
What changed in v2:

  After Simon asked about it in response to patch #2 in v1, I
  investigated whether rela.plt ever contained relocations for .plt,
  or whether that patch fixing a mistake that was always there.
  Testing on some older systems I discovered that yes, indeed it used
  to be the case that rela.plt contained relocations for .plt on
  x86-64, so we still need to support that.  And, testing on PPC64
  showed another variant that we need to support as well.

  Also, testing on PPC64 (ELFv1) on the compile farm I discovered that
  most of the new tests added by the series failed there...  The main
  reason is that we don't currently handle gnu ifunc symbols on PPC64
  / function descriptors very well.  This is now fixed in this version
  of the series, and is the reason the series is now bigger.

Blurb from v1 follows:

Jakub Jelinek noticed that on Fedora 28, GDB can't call strlen:

  (top-gdb) p strlen("hello")
  $1 = (size_t (*)(const char *)) 0x7ffff554aac0 <__strlen_avx2>

That's clearly GDB printing the pointer to the ifunc target function
that implements strlen, instead of calling that function and printing
the result...

Suspecting that that might have been caused by my earlier improvements
to calling functions with no debug info, and improved support for
function aliases, I took a look.  And then I started writing a test,
which then uncovered a ton of problems...  All fixed by this series.

The main issue is that GDB's current ifunc support assumes that (and
the testcase exercises that) the resolver must be compiled without
debug info, and that the resolver has the same name as the user
visible function.

However, glibc nowadays implements ifunc resolvers in C using GCC's
__attribute__((ifunc)), and compiles them with debug info.
With __attribute__((ifunc)), the ifunc symbol has the user visible
name, and the resolver gets a regular function symbol with a different
name (what is passed to the attribute).

While fixing that, I thought I'd extend the existing testcase to
exercise all combination of

 - An ifunc set with __attribute__(ifunc) [different name as the
   user-visible symbol], vs set with

     asm (".type gnu_ifunc, %gnu_indirect_function");

   i.e., with the same name as the user-visible symbol.

 - ifunc resolver compiled with and without debug info.

 - ifunc target function compiled with and without debug info.

Of course that uncovered a whole slew of problems...

And then along the way noticed several other issues and added several
tests for them.  The testcase patch is added torward the end of the
series, because I honestly don't think I can effectively split it down
and split chunks into the patches that implement the fix.  Most of the
testcase changes need all the fixes in place to do any meaningful
testing.  The exception is the last patch in the series.

Pedro Alves (15):
  Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol
    creation)
  Fix calling ifunc functions when resolver has debug info and different
    name
  Calling ifunc functions when target has no debug info but resolver has
  Calling ifunc functions when resolver has debug info, user symbol same
    name
  Fix elf_gnu_ifunc_resolve_by_got buglet
  Fix setting breakpoints on ifunc functions after they're already
    resolved
  Breakpoints, don't skip prologue of ifunc resolvers with debug info
  Eliminate find_pc_partial_function_gnu_ifunc
  Factor out minsym_found/find_function_start_sal overload
  For PPC64: elf_gnu_ifunc_record_cache: handle plt symbols in .text
    section
  Fix stepping past GNU ifunc resolvers (introduce lookup_msym_prefer)
  For PPC64/ELFv1: Introduce mst_data_gnu_ifunc
  PPC64: always make synthetic .text symbols for GNU ifunc symbols
  Extend GNU ifunc testcases
  Fix resolving GNU ifunc bp locations when inferior runs resolver

 bfd/elf64-ppc.c                             |  22 +-
 gdb/blockframe.c                            |  62 +++--
 gdb/breakpoint.c                            |  31 +--
 gdb/breakpoint.h                            |   8 +
 gdb/c-exp.y                                 |  25 +-
 gdb/elfread.c                               | 102 ++++---
 gdb/eval.c                                  |  25 +-
 gdb/gdbtypes.c                              |   4 -
 gdb/infcall.c                               |  58 ++--
 gdb/infcall.h                               |   9 +-
 gdb/linespec.c                              | 123 +++++---
 gdb/minsyms.c                               | 130 +++++----
 gdb/minsyms.h                               |  39 ++-
 gdb/parse.c                                 |  45 ++-
 gdb/symmisc.c                               |   1 +
 gdb/symtab.c                                |  88 ++++--
 gdb/symtab.h                                |  48 +++-
 gdb/testsuite/gdb.base/gnu-ifunc-final.c    |  22 ++
 gdb/testsuite/gdb.base/gnu-ifunc-lib.c      |  12 +-
 gdb/testsuite/gdb.base/gnu-ifunc.c          |   6 -
 gdb/testsuite/gdb.base/gnu-ifunc.exp        | 418 ++++++++++++++++++++++------
 gdb/testsuite/gdb.compile/compile-ifunc.exp |   9 +-
 22 files changed, 905 insertions(+), 382 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/gnu-ifunc-final.c

-- 
2.14.3

Comments

Pedro Alves March 25, 2018, 7:33 p.m. | #1
Dear binutils friends,

This is a bfd patch that is part of a larger gdb patch series,
whose cover letter is found here:
  https://sourceware.org/ml/gdb-patches/2018-03/msg00504.html

Thanks,
Pedro Alves

On 03/25/2018 08:19 PM, Pedro Alves wrote:
> If you create an ifunc using GCC's __attribute__ ifunc, like:

> 

>  extern int gnu_ifunc (int arg);

>  static int gnu_ifunc_target (int arg) { return 0; }

>  __typeof (gnu_ifunc) *gnu_ifunc_resolver (unsigned long hwcap) { return gnu_ifunc_target; }

>  __typeof (gnu_ifunc) gnu_ifunc __attribute__ ((ifunc ("gnu_ifunc_resolver")));

> 

> then you end up with two (function descriptor) symbols, one for the

> ifunc itself, and another for the resolver:

> 

>    (...)

>    12: 0000000000020060    104 FUNC    GLOBAL DEFAULT       18 gnu_ifunc_resolver

>    (...)

>    16: 0000000000020060    104 GNU_IFUNC GLOBAL DEFAULT       18 gnu_ifunc

>    (...)

> 

> Both ifunc and resolver symbols have the same address/value, so

> ppc64_elf_get_synthetic_symtab only creates a synthetic text symbol

> for one of them.  In the case above, it ends up being created for the

> resolver, only:

> 

>   (gdb) maint print msymbols

>   (...)

>   [ 7] t 0x980 .frame_dummy section .text

>   [ 8] T 0x9e4 .gnu_ifunc_resolver section .text

>   [ 9] T 0xa58 __glink_PLTresolve section .text

>   (...)

> 

> GDB needs to know when a program stepped into an ifunc resolver, so

> that it can know whether to step past the resolver into the target

> function without the user noticing.  The way GDB does it is my

> checking whether the current PC points to an ifunc symbol (since

> resolver and ifunc have the same address by design).

> 

> The problem is then that ppc64_elf_get_synthetic_symtab never creates

> the synchetic symbol for the ifunc, so GDB stops stepping at the

> resolver (in a test added by the following patch):

> 

>   (gdb) step

>   gnu_ifunc_resolver (hwcap=21) at gdb/testsuite/gdb.base/gnu-ifunc-lib.c:33

>   33      {

>   (gdb) FAIL: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: step

> 

> After this commit, we get:

> 

>   [ 8] i 0x9e4 .gnu_ifunc section .text

>   [ 9] T 0x9e4 .gnu_ifunc_resolver section .text

> 

> And stepping an ifunc call takes to the final function:

>   (gdb) step

>   0x00000000100009e8 in .final ()

>   (gdb) PASS: gdb.base/gnu-ifunc.exp: resolver_attr=1: resolver_debug=1: final_debug=0: step

> 

> An alternative to touching bfd I considered was for GDB to check

> whether there's an ifunc data symbol / function descriptor that points

> to the current PC, whenever the program stops, but discarded it

> because we'd have to do a linear scan over .opd over an over to find a

> matching function descriptor for the current PC.  At that point I

> considered caching that info, but quickly dismissed it as then that

> has no advantage (memory or performance) over just creating the

> synthetic ifunc text symbol in the first place.

> 

> I ran the binutils and ld testsuites on PPC64 ELFv1 (machine gcc110 on

> the GCC compile farm), and saw no regressions.  This commit is part of

> a GDB patch series that includes GDB tests that fail without this fix.

> 

> OK to apply?

> 

> bfd/ChangeLog:

> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

> 

> 	* elf64-ppc.c (ppc64_elf_get_synthetic_symtab): Don't consider

> 	ifunc and non-ifunc symbols duplicates.

> ---

>  bfd/elf64-ppc.c | 22 ++++++++++++++++------

>  1 file changed, 16 insertions(+), 6 deletions(-)

> 

> diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c

> index 751ad778b26..791ec49b73d 100644

> --- a/bfd/elf64-ppc.c

> +++ b/bfd/elf64-ppc.c

> @@ -3330,13 +3330,23 @@ ppc64_elf_get_synthetic_symtab (bfd *abfd,

>  

>        if (!relocatable && symcount > 1)

>  	{

> -	  /* Trim duplicate syms, since we may have merged the normal and

> -	     dynamic symbols.  Actually, we only care about syms that have

> -	     different values, so trim any with the same value.  */

> +	  /* Trim duplicate syms, since we may have merged the normal

> +	     and dynamic symbols.  Actually, we only care about syms

> +	     that have different values, so trim any with the same

> +	     value.  Don't consider ifunc and ifunc resolver symbols

> +	     duplicates however, because GDB wants to know whether a

> +	     text symbol is an ifunc resolver.  */

>  	  for (i = 1, j = 1; i < symcount; ++i)

> -	    if (syms[i - 1]->value + syms[i - 1]->section->vma

> -		!= syms[i]->value + syms[i]->section->vma)

> -	      syms[j++] = syms[i];

> +	    {

> +	      const asymbol *s0 = syms[i - 1];

> +	      const asymbol *s1 = syms[i];

> +

> +	      if ((s0->value + s0->section->vma

> +		   != s1->value + s1->section->vma)

> +		  || ((s0->flags & BSF_GNU_INDIRECT_FUNCTION)

> +		      != (s1->flags & BSF_GNU_INDIRECT_FUNCTION)))

> +		syms[j++] = syms[i];

> +	    }

>  	  symcount = j;

>  	}

>  

>
Pedro Alves April 26, 2018, 12:22 p.m. | #2
Hello all,

FYI, I've pushed this in now.

Thanks,
Pedro Alves