Re: [PATCH][gdb] Fix gdb.dwarf2/amd64-entry-value-param.exp with -fPIE/-pie

[Tom de Vries <tdevries at suse dot de>]

On 09-08-19 20:16, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Fix this by eliminating baseaddr from read_call_site_scope, and handling the
> Tom> relocation offset at the use sites in call_site_for_pc and
> Tom> call_site_to_target_addr.
> 
> I like this approach, since it represents some small progress on the
> objfile splitting project.
> 
> Tom> +    {
> Tom> +      struct obj_section *sec;
> Tom> +      sec = find_pc_section (pc);
> Tom> +      if (sec != NULL)
> Tom> +	{
> Tom> +	  struct objfile *objfile = sec->objfile;
> Tom> +	  CORE_ADDR baseaddr
> Tom> +	    = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
> 
> Why SECT_OFF_TEXT rather than the section "sec"?
> 

Fixed by using obj_section_offset:
...
        CORE_ADDR pc_unrelocated = pc - obj_section_offset (sec);
...

> Tom> +	  CORE_ADDR pc_unrelocated
> Tom> +	    = gdbarch_adjust_dwarf2_addr (gdbarch, pc - baseaddr);
> 
> I am not sure gdbarch_adjust_dwarf2_addr can be used "bidirectionally"
> like this.  It is probably fine in practice but I wonder about the
> documented contract.
> 

Thanks for pointing that out. Hmm, it seems there is no documented
contract (as you mentioned here:
https://sourceware.org/ml/gdb-patches/2018-05/msg00074.html ).

But, I guess we assume relocated addresses as arg to
gdbarch_adjust_dwarf2_addr because mips_adjust_dwarf2_addr uses
mips_pc_is_mips which uses lookup_minimal_symbol_by_pc which uses
lookup_minimal_symbol_by_pc_section which uses frob_address to
de-relocate the used address before comparing it to the unrelocated
address in MSYMBOL_VALUE_RAW_ADDRESS.

> Tom> +	CORE_ADDR baseaddr
> Tom> +	  = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
> 
> I guess this assumes the text section - but then can the call to
> find_pc_section give anything else?  Maybe it's just something to
> comment and move on.
> 

I suppose that find_pc_section also can return .init or .fini., but I
imagine these wil have the same sections offsets as .text.

> Tom> -  pc = attr_value_as_address (attr) + baseaddr;
> Tom> -  pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
> Tom> +  pc = attr_value_as_address (attr);
> 
> The approach taken elsewhere in dwarf2read.c is to bias, adjust, then
> unbias:
> 
>       CORE_ADDR low
> 	= (gdbarch_adjust_dwarf2_addr (gdbarch, best_lowpc + baseaddr)
> 	   - baseaddr);
> 
> Maybe this would be better here as well, or at least consistent.

Done. Though as pointed out in the rationale, baseaddr can be 0 here, so
effectively we pass at least sometimes unrelocated addresses into
gdbarch_adjust_dwarf2_addr. I'm not sure if frob_address is guaranteed
to be the identity function for unrelocated addresses.

Also, I've copied relocate_text_addr from
https://sourceware.org/ml/gdb-patches/2019-08/msg00241.html to use it in
call_site_to_target_addr.

OK like this?

Thanks,
- Tom

[gdb] Fix gdb.dwarf2/amd64-entry-value-param.exp with -fPIE/-pie

When running gdb.dwarf2/amd64-entry-value-param.exp with target board
unix/-fPIE/-pie, we get:
...
FAIL: gdb.arch/amd64-entry-value-param.exp: call 1: p y
...

The problem is that read_call_site_scope attempts to put relocated addresses
in cu->call_site_htab, for both the pc field:
...
  baseaddr = ANOFFSET (objfile->section_offsets, SECT_OFF_TEXT (objfile));
  ...
  pc = attr_value_as_address (attr) + baseaddr;
  pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
  ...
  call_site->pc = pc;
...
and the target field:
...
             lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
             SET_FIELD_PHYSADDR (call_site->target, lowpc);
...
but fails to do so because it is called before objfile_relocate, making
baseaddr 0.

Fix this by eliminating baseaddr from read_call_site_scope, and handling the
relocation offset at the use sites in call_site_for_pc and
call_site_to_target_addr.

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

gdb/ChangeLog:

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

	PR gdb/24892
	* objfiles.c (relocate_text_addr): New function.
	* objfiles.h (relocate_text_addr): Declare.
	* block.c (call_site_for_pc): Substract relocation offset before
	finding pc in COMPUNIT_CALL_SITE_HTAB.
	* dwarf2loc.c (call_site_to_target_addr): Add relocation offset to
	FIELD_STATIC_PHYSADDR (call_site->target).
	* dwarf2read.c (read_call_site_scope): Eliminate baseaddr.

---
 gdb/block.c      | 12 +++++++++++-
 gdb/dwarf2loc.c  |  7 ++++++-
 gdb/dwarf2read.c |  7 ++-----
 gdb/objfiles.c   | 11 +++++++++++
 gdb/objfiles.h   |  5 +++++
 5 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/gdb/block.c b/gdb/block.c
index 5c6faa8504..3113980259 100644
--- a/gdb/block.c
+++ b/gdb/block.c
@@ -226,7 +226,17 @@ call_site_for_pc (struct gdbarch *gdbarch, CORE_ADDR pc)
   cust = find_pc_compunit_symtab (pc - 1);
 
   if (cust != NULL && COMPUNIT_CALL_SITE_HTAB (cust) != NULL)
-    slot = htab_find_slot (COMPUNIT_CALL_SITE_HTAB (cust), &pc, NO_INSERT);
+    {
+      struct obj_section *sec;
+      sec = find_pc_section (pc);
+      if (sec != NULL)
+	{
+	  CORE_ADDR pc_unrelocated = pc - obj_section_offset (sec);
+	  slot = htab_find_slot (COMPUNIT_CALL_SITE_HTAB (cust),
+				 &pc_unrelocated, NO_INSERT);
+	}
+    }
+
 
   if (slot == NULL)
     {
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 63643cb45d..de8573f6d9 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -855,7 +855,12 @@ call_site_to_target_addr (struct gdbarch *call_site_gdbarch,
       }
 
     case FIELD_LOC_KIND_PHYSADDR:
-      return FIELD_STATIC_PHYSADDR (call_site->target);
+      {
+	CORE_ADDR addr = FIELD_STATIC_PHYSADDR (call_site->target);
+	struct objfile *objfile
+	  = call_site->per_cu->dwarf2_per_objfile->objfile;
+	return relocate_text_addr (addr, objfile);
+      }
 
     default:
       internal_error (__FILE__, __LINE__, _("invalid call site target kind"));
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index de9755f6ce..6a8218dc61 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -13868,7 +13868,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
       return;
     }
   pc = attr_value_as_address (attr) + baseaddr;
-  pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc);
+  pc = gdbarch_adjust_dwarf2_addr (gdbarch, pc) - baseaddr;
 
   if (cu->call_site_htab == NULL)
     cu->call_site_htab = htab_create_alloc_ex (16, core_addr_hash, core_addr_eq,
@@ -14019,10 +14019,7 @@ read_call_site_scope (struct die_info *die, struct dwarf2_cu *cu)
 		         "low pc, for referencing DIE %s [in module %s]"),
 		       sect_offset_str (die->sect_off), objfile_name (objfile));
 	  else
-	    {
-	      lowpc = gdbarch_adjust_dwarf2_addr (gdbarch, lowpc + baseaddr);
-	      SET_FIELD_PHYSADDR (call_site->target, lowpc);
-	    }
+	    SET_FIELD_PHYSADDR (call_site->target, lowpc);
 	}
     }
   else
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) */