Re: [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation)

[Pedro Alves <palves at redhat dot com>]

On 04/01/2018 04:35 AM, Simon Marchi wrote:
> On 2018-03-25 03:19 PM, Pedro Alves wrote:

>>    /* This search algorithm is from _bfd_elf_canonicalize_dynamic_reloc.  */
>>    for (relplt = obfd->sections; relplt != NULL; relplt = relplt->next)
>> -    if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
>> -	&& (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
>> -	    || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
>> -      break;
>> +    {
>> +      const auto &this_hdr = elf_section_data (relplt)->this_hdr;
>> +
>> +      if (this_hdr.sh_type == SHT_REL || this_hdr.sh_type == SHT_RELA)
>> +	{
>> +	  asection *target_section = NULL;
>> +
>> +	  if (this_hdr.sh_info == plt_elf_idx)
>> +	    target_section = plt;
>> +	  else if (this_hdr.sh_info == got_plt_elf_idx)
>> +	    target_section = got_plt;
>> +
>> +	  if (target_section != NULL)
>> +	    break;
> 
> Is it really useful to have/set target_section?  Couldn't we just break out of the
> loop like this?
> 
>   if (this_hdr.sh_info == plt_elf_idx
>       || this_hdr.sh_info == got_plt_elf_idx)
>     break;
> 

Hmm, the original intention was to use target_section in the other
loop, but that didn't work, so I reverted it, but somehow not that
part.  :-P

>>  
>> @@ -573,6 +586,18 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
>>  
>>    std::string string_buffer;
>>  
>> +  /* Does ADDRESS reside in SECTION of OBFD?  */
>> +  auto within_section = [obfd] (asection *section, CORE_ADDR address)
>> +    {
>> +      if (section == NULL)
>> +	return false;
>> +
>> +      /* Does the pointer reside in the .got.plt section?  */
> 
> That comment should change, since it's not stricly .got.plt.
> 

I've removed it, since the intro comment already says it all.

> Or maybe you intended to use target_section here at some point?  Is there a
> relationship between the section that matched in the for loop above and the
> section that will contain the address?  In other words, could we save the
> target_section from above and do

Yeah, in an earlier version I tried doing that, but then testing on the
different systems found out that there's no relation between the
two sections.

Here's the updated patch.  WDYT?

>From 0e91b0c40141326243dbd1dd735ca1e1fe5c78ce Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 10 Apr 2018 18:11:27 +0100
Subject: [PATCH] Fix breakpoints in ifunc after inferior resolved it (@got.plt
 symbol creation)

Setting a breakpoint on an ifunc symbol after the ifunc has already
been resolved by the inferior should result in creating a breakpoint
location at the ifunc target.  However, that's not what happens on
current Fedora:

  (gdb) n
  53        i = gnu_ifunc (1);    /* break-at-call */
  (gdb)
  54        assert (i == 2);
  (gdb) b gnu_ifunc
  Breakpoint 2 at gnu-indirect-function resolver at 0x7ffff7bd36ee
  (gdb) info breakpoints
  Num     Type                   Disp Enb Address            What
  2       STT_GNU_IFUNC resolver keep y   0x00007ffff7bd36ee <gnu_ifunc+4>

The problem is that elf_gnu_ifunc_resolve_by_got never manages to
resolve an ifunc target.  The reason is that GDB never actually
creates the internal got.plt symbols:

 (gdb) p 'gnu_ifunc@got.plt'
 No symbol "gnu_ifunc@got.plt" in current context.

and this is because GDB expects that rela.plt has relocations for
.plt, while it actually has relocations for .got.plt:

 Relocation section [10] '.rela.plt' for section [22] '.got.plt' at offset 0x570 contains 2 entries:
   Offset              Type            Value               Addend Name
   0x0000000000601018  X86_64_JUMP_SLOT 000000000000000000      +0 __assert_fail
   0x0000000000601020  X86_64_JUMP_SLOT 000000000000000000      +0 gnu_ifunc


Using an older system on the GCC compile farm (machine gcc15, an
x86-64 running Debian 6.0.8, with GNU ld 2.20.1), we see that it used
to be that we'd get a .rela.plt section for .plt:

 Relocation section [ 9] '.rela.plt' for section [11] '.plt' at offset 0x578 contains 3 entries:
   Offset              Type            Value               Addend Name
   0x0000000000600cc0  X86_64_JUMP_SLOT 000000000000000000      +0 __assert_fail
   0x0000000000600cc8  X86_64_JUMP_SLOT 000000000000000000      +0 __libc_start_main
   0x0000000000600cd0  X86_64_JUMP_SLOT 000000000000000000      +0 gnu_ifunc

Those offsets did point into .got.plt, as seen with objdump -h:

  20 .got.plt      00000030  0000000000600ca8  0000000000600ca8  00000ca8  2**3
     		   CONTENTS, ALLOC, LOAD, DATA

I also tested on gcc110 on the compile farm (PPC64 running CentOS
7.4.1708, with GNU ld 2.25.1), and there we see instead:

 Relocation section [ 9] '.rela.plt' for section [23] '.plt' at offset 0x5d0 contains 4 entries:
   Offset              Type            Value               Addend Name
   0x0000000010020148  PPC64_JMP_SLOT  000000000000000000      +0 __libc_start_main
   0x0000000010020160  PPC64_JMP_SLOT  000000000000000000      +0 __gmon_start__
   0x0000000010020178  PPC64_JMP_SLOT  000000000000000000      +0 __assert_fail
   0x0000000010020190  PPC64_JMP_SLOT  000000000000000000      +0 gnu_ifunc

But note that those offsets point into .plt, not .got.plt, as seen
with objdump -h:

 22 .plt          00000078  0000000010020130  0000000010020130  00010130  2**3
                  ALLOC

This commit makes us support all the different combinations above.

With that addressed, we now get:

 (gdb) p 'gnu_ifunc@got.plt'
 $1 = (<text from jump slot in .got.plt, no debug info>) 0x400753 <final>

And setting a breakpoint on the ifunc finds the ifunc target:

 (gdb) b gnu_ifunc
 Breakpoint 2 at 0x400753
 (gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 2       breakpoint     keep y   0x0000000000400753 <final>

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* elfread.c (elf_rel_plt_read): Look for relocations for .got.plt too.
	not .plt.
---
 gdb/elfread.c | 57 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/gdb/elfread.c b/gdb/elfread.c
index 260789062d0..16a692d3713 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -535,8 +535,7 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
 {
   bfd *obfd = objfile->obfd;
   const struct elf_backend_data *bed = get_elf_backend_data (obfd);
-  asection *plt, *relplt, *got_plt;
-  int plt_elf_idx;
+  asection *relplt, *got_plt;
   bfd_size_type reloc_count, reloc;
   struct gdbarch *gdbarch = get_objfile_arch (objfile);
   struct type *ptr_type = builtin_type (gdbarch)->builtin_data_ptr;
@@ -545,11 +544,6 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
   if (objfile->separate_debug_objfile_backlink)
     return;
 
-  plt = bfd_get_section_by_name (obfd, ".plt");
-  if (plt == NULL)
-    return;
-  plt_elf_idx = elf_section_data (plt)->this_idx;
-
   got_plt = bfd_get_section_by_name (obfd, ".got.plt");
   if (got_plt == NULL)
     {
@@ -559,12 +553,25 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
 	return;
     }
 
+  /* Depending on system, we may find jump slots in a relocation
+     section for either .got.plt or .plt.  */
+  asection *plt = bfd_get_section_by_name (obfd, ".plt");
+  int plt_elf_idx = (plt != NULL) ? elf_section_data (plt)->this_idx : -1;
+
+  int got_plt_elf_idx = elf_section_data (got_plt)->this_idx;
+
   /* This search algorithm is from _bfd_elf_canonicalize_dynamic_reloc.  */
   for (relplt = obfd->sections; relplt != NULL; relplt = relplt->next)
-    if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
-	&& (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
-	    || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
-      break;
+    {
+      const auto &this_hdr = elf_section_data (relplt)->this_hdr;
+
+      if (this_hdr.sh_type == SHT_REL || this_hdr.sh_type == SHT_RELA)
+	{
+	  if (this_hdr.sh_info == plt_elf_idx
+	      || this_hdr.sh_info == got_plt_elf_idx)
+	    break;
+	}
+    }
   if (relplt == NULL)
     return;
 
@@ -573,6 +580,17 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
 
   std::string string_buffer;
 
+  /* Does ADDRESS reside in SECTION of OBFD?  */
+  auto within_section = [obfd] (asection *section, CORE_ADDR address)
+    {
+      if (section == NULL)
+	return false;
+
+      return (bfd_get_section_vma (obfd, section) <= address
+	      && (address < bfd_get_section_vma (obfd, section)
+		  + bfd_get_section_size (section)));
+    };
+
   reloc_count = relplt->size / elf_section_data (relplt)->this_hdr.sh_entsize;
   for (reloc = 0; reloc < reloc_count; reloc++)
     {
@@ -585,10 +603,15 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
       name = bfd_asymbol_name (*relplt->relocation[reloc].sym_ptr_ptr);
       address = relplt->relocation[reloc].address;
 
-      /* Does the pointer reside in the .got.plt section?  */
-      if (!(bfd_get_section_vma (obfd, got_plt) <= address
-            && address < bfd_get_section_vma (obfd, got_plt)
-			 + bfd_get_section_size (got_plt)))
+      asection *msym_section;
+
+      /* Does the pointer reside in either the .got.plt or .plt
+	 sections?  */
+      if (within_section (got_plt, address))
+	msym_section = got_plt;
+      else if (within_section (plt, address))
+	msym_section = plt;
+      else
 	continue;
 
       /* We cannot check if NAME is a reference to mst_text_gnu_ifunc as in
@@ -600,8 +623,8 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
 
       msym = record_minimal_symbol (reader, string_buffer.c_str (),
 				    string_buffer.size (),
-                                    true, address, mst_slot_got_plt, got_plt,
-				    objfile);
+				    true, address, mst_slot_got_plt,
+				    msym_section, objfile);
       if (msym)
 	SET_MSYMBOL_SIZE (msym, ptr_size);
     }
-- 
2.14.3