[PATCH][gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie

[PATCH][gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie

[Tom de Vries]

Hi,

When running gdb.arch/amd64-tailcall-*.exp with target board
unix/-fPIE/-pie, we get:
...
FAIL: gdb.arch/amd64-tailcall-cxx.exp: bt
FAIL: gdb.arch/amd64-tailcall-noret.exp: bt
FAIL: gdb.arch/amd64-tailcall-self.exp: bt
...

The first FAIL in more detail, compared to a nopie run:
...
(gdb) bt
 #0  b ()
-#1  0x00000000004004df in a (q=<optimized out>)
+#1  0x0000000000000672 in ?? ()
-#2  0x00000000004003d5 in main (argc=<optimized out>, argv=<optimized out>)
+#2  0x0000555555554535 in main (argc=<optimized out>, argv=<optimized out>)
-(gdb) PASS: gdb.arch/amd64-tailcall-self.exp: bt
+(gdb) FAIL: gdb.arch/amd64-tailcall-self.exp: bt
...
shows an unrelocated address for function a.

The problem is that pretend_pc uses the pc field from a struct call_site
without adding the missing relocation offset.

Fix this by adding the appropriate offset.

Tested on x86_64-linux, with and without -fPIE/-pie.

OK for trunk?

Thanks,
- Tom

[gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie

gdb/ChangeLog:

2019-08-09  Tom de Vries  <tdevries@suse.de>

	* dwarf2-frame-tailcall.c (pretend_pc): Add relocation offset to pc
	field of struct call_site.

---
 gdb/dwarf2-frame-tailcall.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2-frame-tailcall.c b/gdb/dwarf2-frame-tailcall.c
index e8f5aaf9c7..02c50db3af 100644
--- a/gdb/dwarf2-frame-tailcall.c
+++ b/gdb/dwarf2-frame-tailcall.c
@@ -29,6 +29,8 @@
 #include "value.h"
 #include "dwarf2-frame.h"
 #include "gdbarch.h"
+#include "dwarf2read.h"
+#include "objfiles.h"
 
 /* Contains struct tailcall_cache indexed by next_bottom_frame.  */
 static htab_t cache_htab;
@@ -240,14 +242,38 @@ pretend_pc (struct frame_info *this_frame, struct tailcall_cache *cache)
   gdb_assert (next_levels >= 0);
 
   if (next_levels < chain->callees)
-    return chain->call_site[chain->length - next_levels - 1]->pc;
+    {
+      struct call_site *call_site
+	= chain->call_site[chain->length - next_levels - 1];
+      CORE_ADDR addr = call_site->pc;
+      struct dwarf2_per_objfile *dwarf2_per_objfile
+	= call_site->per_cu->dwarf2_per_objfile;
+       struct objfile *objfile = dwarf2_per_objfile->objfile;
+       CORE_ADDR baseaddr
+	 = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
+       struct gdbarch *gdbarch = get_objfile_arch (objfile);
+       addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
+      return addr;
+    }
   next_levels -= chain->callees;
 
   /* Otherwise CHAIN->CALLEES are already covered by CHAIN->CALLERS.  */
   if (chain->callees != chain->length)
     {
       if (next_levels < chain->callers)
-	return chain->call_site[chain->callers - next_levels - 1]->pc;
+	{
+	  struct call_site *call_site
+	    = chain->call_site[chain->callers - next_levels - 1];
+	  CORE_ADDR addr = call_site->pc;
+	  struct dwarf2_per_objfile *dwarf2_per_objfile
+	    = call_site->per_cu->dwarf2_per_objfile;
+	  struct objfile *objfile = dwarf2_per_objfile->objfile;
+	  CORE_ADDR baseaddr
+	    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
+	  struct gdbarch *gdbarch = get_objfile_arch (objfile);
+	  addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
+	  return addr;
+	}
       next_levels -= chain->callers;
     }
 

Re: [PATCH][gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie

[Pedro Alves]

On 8/9/19 4:03 PM, Tom de Vries wrote:

>  
> +/* Given an unrelocated address ADDR belonging to the text section of OBJFILE,
> +   return the relocated address.  */
> +
> +static CORE_ADDR
> +relocate_text_addr (CORE_ADDR addr, struct objfile *objfile)
> +{
> +  CORE_ADDR baseaddr
> +    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
> +  struct gdbarch *gdbarch = get_objfile_arch (objfile);
> +  addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
> +  return addr;

I'd write:

 return gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);

> +}
> +
>  /* Find PC to be unwound from THIS_FRAME.  THIS_FRAME must be a part of
>     CACHE.  */
>  
> @@ -240,14 +255,25 @@ pretend_pc (struct frame_info *this_frame, struct tailcall_cache *cache)
>    gdb_assert (next_levels >= 0);
>  
>    if (next_levels < chain->callees)
> -    return chain->call_site[chain->length - next_levels - 1]->pc;
> +    {
> +      struct call_site *call_site
> +	= chain->call_site[chain->length - next_levels - 1];
> +      struct objfile *objfile = call_site->per_cu->dwarf2_per_objfile->objfile;
> +      return relocate_text_addr (call_site->pc, objfile);
> +    }
>    next_levels -= chain->callees;
>  
>    /* Otherwise CHAIN->CALLEES are already covered by CHAIN->CALLERS.  */
>    if (chain->callees != chain->length)
>      {
>        if (next_levels < chain->callers)
> -	return chain->call_site[chain->callers - next_levels - 1]->pc;
> +	{
> +	  struct call_site *call_site
> +	    = chain->call_site[chain->callers - next_levels - 1];
> +	  struct objfile *objfile
> +	    = call_site->per_cu->dwarf2_per_objfile->objfile;
> +	  return relocate_text_addr (call_site->pc, objfile);
> +	}

That seems fine, but it seems you could have factored out more, like:

static CORE_ADDR
call_site_relocated_pc (struct call_site *call_site)
{
  struct objfile *objfile
    = call_site->per_cu->dwarf2_per_objfile->objfile;
  CORE_ADDR baseaddr
    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  struct gdbarch *gdbarch = get_objfile_arch (objfile);
  return gdbarch_adjust_dwarf2_addr (gdbarch, call_size->pc + baseaddr);
}

Then the other hunks would look like:

  struct call_site *call_site
    = chain->call_site[chain->length - next_levels - 1];
  return call_site_relocated_pc (call_site);

...

  struct call_site *call_site
    = chain->call_site[chain->callers - next_levels - 1];
  return call_site_relocated_pc (call_site);

call_site_relocated_pc could even be a method of struct call_site, I guess,
so you'd write:

  return call_site->relocated_pc ();

Any reason you didn't do something like that?

Thanks,
Pedro Alves

Re: [PATCH][gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 3331 bytes --]

On 09-08-19 19:38, Pedro Alves wrote:
> On 8/9/19 4:03 PM, Tom de Vries wrote:
> 
>>  
>> +/* Given an unrelocated address ADDR belonging to the text section of OBJFILE,
>> +   return the relocated address.  */
>> +
>> +static CORE_ADDR
>> +relocate_text_addr (CORE_ADDR addr, struct objfile *objfile)
>> +{
>> +  CORE_ADDR baseaddr
>> +    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>> +  struct gdbarch *gdbarch = get_objfile_arch (objfile);
>> +  addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
>> +  return addr;
> 
> I'd write:
> 
>  return gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
> 

Updated accordingly.

>> +}
>> +
>>  /* Find PC to be unwound from THIS_FRAME.  THIS_FRAME must be a part of
>>     CACHE.  */
>>  
>> @@ -240,14 +255,25 @@ pretend_pc (struct frame_info *this_frame, struct tailcall_cache *cache)
>>    gdb_assert (next_levels >= 0);
>>  
>>    if (next_levels < chain->callees)
>> -    return chain->call_site[chain->length - next_levels - 1]->pc;
>> +    {
>> +      struct call_site *call_site
>> +	= chain->call_site[chain->length - next_levels - 1];
>> +      struct objfile *objfile = call_site->per_cu->dwarf2_per_objfile->objfile;
>> +      return relocate_text_addr (call_site->pc, objfile);
>> +    }
>>    next_levels -= chain->callees;
>>  
>>    /* Otherwise CHAIN->CALLEES are already covered by CHAIN->CALLERS.  */
>>    if (chain->callees != chain->length)
>>      {
>>        if (next_levels < chain->callers)
>> -	return chain->call_site[chain->callers - next_levels - 1]->pc;
>> +	{
>> +	  struct call_site *call_site
>> +	    = chain->call_site[chain->callers - next_levels - 1];
>> +	  struct objfile *objfile
>> +	    = call_site->per_cu->dwarf2_per_objfile->objfile;
>> +	  return relocate_text_addr (call_site->pc, objfile);
>> +	}
> 
> That seems fine, but it seems you could have factored out more, like:
> 
> static CORE_ADDR
> call_site_relocated_pc (struct call_site *call_site)
> {
>   struct objfile *objfile
>     = call_site->per_cu->dwarf2_per_objfile->objfile;
>   CORE_ADDR baseaddr
>     = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>   struct gdbarch *gdbarch = get_objfile_arch (objfile);
>   return gdbarch_adjust_dwarf2_addr (gdbarch, call_size->pc + baseaddr);
> }
> 
> Then the other hunks would look like:
> 
>   struct call_site *call_site
>     = chain->call_site[chain->length - next_levels - 1];
>   return call_site_relocated_pc (call_site);
> 
> ...
> 
>   struct call_site *call_site
>     = chain->call_site[chain->callers - next_levels - 1];
>   return call_site_relocated_pc (call_site);
> 
> call_site_relocated_pc could even be a method of struct call_site, I guess,
> so you'd write:
> 
>   return call_site->relocated_pc ();
> 
> Any reason you didn't do something like that?

I went for a utility function that can be maximally reused, as opposed
to a function that maximally factors out the code in this particular
patch. But we can do both, so this patch adds:
- relocate_text_addr (I've moved it to objfiles.h/objfiles.c)
- call_site::relocated_pc

[ BTW, I should mention that this fix is dependent on the patch "[gdb]
Fix gdb.dwarf2/amd64-entry-value-param.exp with -fPIE/-pie" (
https://sourceware.org/ml/gdb-patches/2019-08/msg00215.html ). ]

OK like this?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-Fix-gdb.arch-amd64-tailcall-.exp-with-fPIE-pie.patch --]
[-- Type: text/x-patch, Size: 4730 bytes --]

[gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie

When running gdb.arch/amd64-tailcall-*.exp with target board
unix/-fPIE/-pie, we get:
...
FAIL: gdb.arch/amd64-tailcall-cxx.exp: bt
FAIL: gdb.arch/amd64-tailcall-noret.exp: bt
FAIL: gdb.arch/amd64-tailcall-self.exp: bt
...

The first FAIL in more detail, compared to a nopie run:
...
(gdb) bt
 #0  b ()
-#1  0x00000000004004df in a (q=<optimized out>)
+#1  0x0000000000000672 in ?? ()
-#2  0x00000000004003d5 in main (argc=<optimized out>, argv=<optimized out>)
+#2  0x0000555555554535 in main (argc=<optimized out>, argv=<optimized out>)
-(gdb) PASS: gdb.arch/amd64-tailcall-self.exp: bt
+(gdb) FAIL: gdb.arch/amd64-tailcall-self.exp: bt
...
shows an unrelocated address for function a.

The problem is that pretend_pc uses the pc field from a struct call_site
without adding the missing relocation offset.

Fix this by adding the appropriate offset.

Tested on x86_64-linux, with and without -fPIE/-pie.

gdb/ChangeLog:

2019-08-09  Tom de Vries  <tdevries@suse.de>

	* gdbtypes.c (call_site::relocated_pc): New function.
	* gdbtypes.h (call_site::relocated_pc): Declare.
	* objfiles.c (relocate_text_addr): New function.
	* objfiles.h (relocate_text_addr): Declare.
	* dwarf2-frame-tailcall.c (pretend_pc): Add relocation offset to pc
	field of struct call_site.

---
 gdb/dwarf2-frame-tailcall.c | 12 ++++++++++--
 gdb/gdbtypes.c              |  8 ++++++++
 gdb/gdbtypes.h              |  4 ++++
 gdb/objfiles.c              | 11 +++++++++++
 gdb/objfiles.h              |  5 +++++
 5 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/gdb/dwarf2-frame-tailcall.c b/gdb/dwarf2-frame-tailcall.c
index e8f5aaf9c7..d2c9a813f2 100644
--- a/gdb/dwarf2-frame-tailcall.c
+++ b/gdb/dwarf2-frame-tailcall.c
@@ -240,14 +240,22 @@ pretend_pc (struct frame_info *this_frame, struct tailcall_cache *cache)
   gdb_assert (next_levels >= 0);
 
   if (next_levels < chain->callees)
-    return chain->call_site[chain->length - next_levels - 1]->pc;
+    {
+      struct call_site *call_site
+	= chain->call_site[chain->length - next_levels - 1];
+      return call_site->relocated_pc ();
+    }
   next_levels -= chain->callees;
 
   /* Otherwise CHAIN->CALLEES are already covered by CHAIN->CALLERS.  */
   if (chain->callees != chain->length)
     {
       if (next_levels < chain->callers)
-	return chain->call_site[chain->callers - next_levels - 1]->pc;
+	{
+	  struct call_site *call_site
+	    = chain->call_site[chain->callers - next_levels - 1];
+	  return call_site->relocated_pc ();
+	}
       next_levels -= chain->callers;
     }
 
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 177455e612..7466300572 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -39,6 +39,7 @@
 #include "dwarf2loc.h"
 #include "gdbcore.h"
 #include "floatformat.h"
+#include "dwarf2read.h"
 
 /* Initialize BADNESS constants.  */
 
@@ -5614,3 +5615,10 @@ _initialize_gdbtypes (void)
 			   show_strict_type_checking,
 			   &setchecklist, &showchecklist);
 }
+
+/* See gdbtypes.h.  */
+
+CORE_ADDR call_site::relocated_pc ()
+{
+  return relocate_text_addr (pc, per_cu->dwarf2_per_objfile->objfile);
+}
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 7268d3e4aa..d25398e7c4 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1238,6 +1238,10 @@ struct call_site
     /* * Describe DW_TAG_call_site's DW_TAG_formal_parameter.  */
 
     struct call_site_parameter parameter[1];
+
+    /* Return a relocated call_site->pc.  */
+
+    CORE_ADDR relocated_pc ();
   };
 
 /* * The default value of TYPE_CPLUS_SPECIFIC(T) points to this shared
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 7cbcbbd01b..18d9540849 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -1493,3 +1493,14 @@ objfile_flavour_name (struct objfile *objfile)
     return bfd_flavour_name (bfd_get_flavour (objfile->obfd));
   return NULL;
 }
+
+/* See objfiles.h.  */
+
+CORE_ADDR
+relocate_text_addr (CORE_ADDR addr, struct objfile *objfile)
+{
+  CORE_ADDR baseaddr
+    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
+  struct gdbarch *gdbarch = get_objfile_arch (objfile);
+  return gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
+}
diff --git a/gdb/objfiles.h b/gdb/objfiles.h
index 239aba2c2a..a86bf3b5f9 100644
--- a/gdb/objfiles.h
+++ b/gdb/objfiles.h
@@ -789,4 +789,9 @@ extern void objfile_register_static_link
 extern const struct dynamic_prop *objfile_lookup_static_link
   (struct objfile *objfile, const struct block *block);
 
+/* Given an unrelocated address ADDR belonging to the text section of OBJFILE,
+   return the relocated address.  */
+
+extern CORE_ADDR relocate_text_addr (CORE_ADDR addr, struct objfile *objfile);
+
 #endif /* !defined (OBJFILES_H) */

Re: [PATCH][gdb] Fix gdb.arch/amd64-tailcall-*.exp with -fPIE/-pie

[Pedro Alves]

On 8/10/19 6:44 AM, Tom de Vries wrote:
> On 09-08-19 19:38, Pedro Alves wrote:
>> On 8/9/19 4:03 PM, Tom de Vries wrote:
>>
>>>  
>>> +/* Given an unrelocated address ADDR belonging to the text section of OBJFILE,
>>> +   return the relocated address.  */
>>> +
>>> +static CORE_ADDR
>>> +relocate_text_addr (CORE_ADDR addr, struct objfile *objfile)
>>> +{
>>> +  CORE_ADDR baseaddr
>>> +    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>>> +  struct gdbarch *gdbarch = get_objfile_arch (objfile);
>>> +  addr = gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
>>> +  return addr;
>>
>> I'd write:
>>
>>  return gdbarch_adjust_dwarf2_addr (gdbarch, addr + baseaddr);
>>
> 
> Updated accordingly.
> 
>>> +}
>>> +
>>>  /* Find PC to be unwound from THIS_FRAME.  THIS_FRAME must be a part of
>>>     CACHE.  */
>>>  
>>> @@ -240,14 +255,25 @@ pretend_pc (struct frame_info *this_frame, struct tailcall_cache *cache)
>>>    gdb_assert (next_levels >= 0);
>>>  
>>>    if (next_levels < chain->callees)
>>> -    return chain->call_site[chain->length - next_levels - 1]->pc;
>>> +    {
>>> +      struct call_site *call_site
>>> +	= chain->call_site[chain->length - next_levels - 1];
>>> +      struct objfile *objfile = call_site->per_cu->dwarf2_per_objfile->objfile;
>>> +      return relocate_text_addr (call_site->pc, objfile);
>>> +    }
>>>    next_levels -= chain->callees;
>>>  
>>>    /* Otherwise CHAIN->CALLEES are already covered by CHAIN->CALLERS.  */
>>>    if (chain->callees != chain->length)
>>>      {
>>>        if (next_levels < chain->callers)
>>> -	return chain->call_site[chain->callers - next_levels - 1]->pc;
>>> +	{
>>> +	  struct call_site *call_site
>>> +	    = chain->call_site[chain->callers - next_levels - 1];
>>> +	  struct objfile *objfile
>>> +	    = call_site->per_cu->dwarf2_per_objfile->objfile;
>>> +	  return relocate_text_addr (call_site->pc, objfile);
>>> +	}
>>
>> That seems fine, but it seems you could have factored out more, like:
>>
>> static CORE_ADDR
>> call_site_relocated_pc (struct call_site *call_site)
>> {
>>   struct objfile *objfile
>>     = call_site->per_cu->dwarf2_per_objfile->objfile;
>>   CORE_ADDR baseaddr
>>     = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
>>   struct gdbarch *gdbarch = get_objfile_arch (objfile);
>>   return gdbarch_adjust_dwarf2_addr (gdbarch, call_size->pc + baseaddr);
>> }
>>
>> Then the other hunks would look like:
>>
>>   struct call_site *call_site
>>     = chain->call_site[chain->length - next_levels - 1];
>>   return call_site_relocated_pc (call_site);
>>
>> ...
>>
>>   struct call_site *call_site
>>     = chain->call_site[chain->callers - next_levels - 1];
>>   return call_site_relocated_pc (call_site);
>>
>> call_site_relocated_pc could even be a method of struct call_site, I guess,
>> so you'd write:
>>
>>   return call_site->relocated_pc ();
>>
>> Any reason you didn't do something like that?
> 
> I went for a utility function that can be maximally reused, as opposed
> to a function that maximally factors out the code in this particular
> patch. But we can do both, so this patch adds:
> - relocate_text_addr (I've moved it to objfiles.h/objfiles.c)
> - call_site::relocated_pc
> 
> [ BTW, I should mention that this fix is dependent on the patch "[gdb]
> Fix gdb.dwarf2/amd64-entry-value-param.exp with -fPIE/-pie" (
> https://sourceware.org/ml/gdb-patches/2019-08/msg00215.html ). ]
> 
> OK like this?

Thanks.  It looks much better to me this way, but I think we
should resolve the SECT_OFF_TEXT discussion in the other patch
before moving forward.

Thanks,
Pedro Alves