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

[Pedro Alves <palves@redhat.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