[PATCH 4/4] gdb: move get_section_table from exec_target to dummy_target

[Andrew Burgess <andrew.burgess@embecosm.com>]

The only target that implements target_ops::get_section_table in a
meaningful way is exec_target.  This target calls back into the
program space to return the current global section_table.

The global section table is populated whenever the user provides GDB
with an executable, or when a symbol file is loaded, e.g. when a
dynamic library is loaded, or when the user does add-symbol-file.

I recently ran into a situation where a user, debugging a remote
target, was not supplying GDB with a main executable at all.  Instead
the user attached to the target then did add-symbol-file, and then
proceeded to debug the target.

This works fine, but it was noticed that even when
trust-readonly-sections was on GDB was still accessing the target to
get the contents of readonly sections.

The problem is that by not providing an executable there was no
exec_target in the target stack, and so when GDB calls the
target_ops::get_section_table function GDB ends up in
dummy_target::get_section_table, which just returns NULL.

What I want is that even when GDB doesn't have an exec_target in the
target stack, a call to target_ops::get_section_table will still
return the section_table from the current program space.

When considering how to achieve this my first though was, why is the
request for the section table going via the target stack at all?  The
set of sections loaded is a property of the program space, not the
target.  This is, after all, why the data is being stored in the
program space.

So I initially tried changing target_get_section_table so that,
instead of calling into the target it just returns
current_program_space->target_sections ().

This would be fine except for one issue, target_bfd (from
bfd-target.c).  This code is used from solib-svr4.c to create a
temporary target_ops structure that implements two functions
target_bfd::xfer_partial and target_bfd::get_section_table.

The purpose behind the code is to enable two targets, ppc64 and frv to
decode function descriptors from the dynamic linker, based on the
non-relocated addresses from within the dynamic linker bfd object.

Both of the implemented functions in target_bfd rely on the target_bfd
object holding a section table, and the ppc64 target requires that the
target_bfd implement ::get_section_table.

The frv target doesn't require ::get_section_table, instead it
requires the ::xfer_partial.  We could in theory change the ppc64
target to use the same approach as frv, however, this would be a bad
idea.  I believe that the frv target approach is broken.  I'll
explain:

The frv target calls get_target_memory_unsigned to read the function
descriptor.  The address being read is the non-relocated address read
from the dynamic linker in solib-srv4.c:enable_break.  Calling
get_target_memory_unsigned eventually ends up in target_xfer_partial
with an object type of TARGET_OBJECT_RAW_MEMORY.  This will then call
memory_xfer_check_region.  I believe that it is quite possible that a
the non-relocated addresses pulled from the dynamic linker could be in
a memory region that is not readable, while the relocated addresses
are in a readable memory region.  If this was ever the case for the
frv target then GDB would reject the attempt to read the non-relocated
function pointer.

In contrast the ppc64 target calls target_section_by_addr, which calls
target_get_section_table, which then calls the ::get_section_table
function on the target.

Thus, when reflecting on target_bfd we see two functions,
::xfer_partial and ::get_section_table.  The former is required by the
frv target, but that target is (I think) potentially broken.  While
the latter is required by the ppc64 target, but this forces
::get_section_table to exist as a target_ops member function.

So my original plan, have target_get_section_table NOT call a
target_ops member function appears to be flawed.

My next idea was to remove exec_target::get_section_table, and instead
move the implementation into dummy_target::get_section_table.
Currently the dummy_target implementation always returns NULL
indicating no section table, but plenty of other dummy_target member
functions do more than just return null values.

So now, dummy_target::get_section_table returns the section table from
the current program space.  This allows target_bfd to remain
unchanged, so ppc64 and frv should not be affected.

Making this change removes the requirement for the user to provide an
executable, GDB can now always access the section_table, as the
dummy_target always exists in the target stack.

Finally, there's a test that the target_section table is not empty in
the case where the user does add-symbol-file without providing an
executable.

gdb/ChangeLog:

	* exec.c (exec_target::get_section_table): Delete member function.
	(section_table_read_available_memory): Use current_top_target, not
	just the exec_ops target.
	* target-delegates.c: Regenerate.
	* target.c (default_get_section_table): New function.
	* target.h (target_ops::get_section_table): Change default
	behaviour to call default_get_section_table.
	(default_get_section_table): Declare.
---
 gdb/ChangeLog                                  | 11 +++++++++++
 gdb/exec.c                                     | 10 ++--------
 gdb/target-delegates.c                         |  2 +-
 gdb/target.c                                   |  7 +++++++
 gdb/target.h                                   |  6 +++++-
 gdb/testsuite/gdb.base/maint-info-sections.exp |  4 +---
 6 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 2ad89af4798..e613409e8ef 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -75,7 +75,6 @@ struct exec_target final : public target_ops
 					const gdb_byte *writebuf,
 					ULONGEST offset, ULONGEST len,
 					ULONGEST *xfered_len) override;
-  const target_section_table *get_section_table () override;
   void files_info () override;
 
   bool has_memory () override;
@@ -788,7 +787,8 @@ enum target_xfer_status
 section_table_read_available_memory (gdb_byte *readbuf, ULONGEST offset,
 				     ULONGEST len, ULONGEST *xfered_len)
 {
-  const target_section_table *table = target_get_section_table (&exec_ops);
+  const target_section_table *table
+    = target_get_section_table (current_top_target ());
   std::vector<mem_range> available_memory
     = section_table_available_memory (offset, len, *table);
 
@@ -897,12 +897,6 @@ section_table_xfer_memory_partial (gdb_byte *readbuf, const gdb_byte *writebuf,
   return TARGET_XFER_EOF;		/* We can't help.  */
 }
 
-const target_section_table *
-exec_target::get_section_table ()
-{
-  return &current_program_space->target_sections ();
-}
-
 enum target_xfer_status
 exec_target::xfer_partial (enum target_object object,
 			   const char *annex, gdb_byte *readbuf,
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 69fbc0f3b23..1c7999724c7 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -2030,7 +2030,7 @@ target_ops::get_section_table ()
 const target_section_table *
 dummy_target::get_section_table ()
 {
-  return NULL;
+  return default_get_section_table ();
 }
 
 const target_section_table *
diff --git a/gdb/target.c b/gdb/target.c
index 78535b89e58..ba445e7fd34 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -836,6 +836,13 @@ target_section_by_addr (struct target_ops *target, CORE_ADDR addr)
   return NULL;
 }
 
+/* See target.h.  */
+
+const target_section_table *
+default_get_section_table ()
+{
+  return &current_program_space->target_sections ();
+}
 
 /* Helper for the memory xfer routines.  Checks the attributes of the
    memory region of MEMADDR against the read or write being attempted.
diff --git a/gdb/target.h b/gdb/target.h
index c11a8c91aa8..45b92f7d83f 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -688,7 +688,7 @@ struct target_ops
     virtual void log_command (const char *)
       TARGET_DEFAULT_IGNORE ();
     virtual const target_section_table *get_section_table ()
-      TARGET_DEFAULT_RETURN (NULL);
+      TARGET_DEFAULT_RETURN (default_get_section_table ());
 
     /* Provide default values for all "must have" methods.  */
     virtual bool has_all_memory () { return false; }
@@ -2422,6 +2422,10 @@ const struct target_section *target_section_by_addr (struct target_ops *target,
 extern const target_section_table *target_get_section_table
   (struct target_ops *target);
 
+/* Default implementation of get_section_table for dummy_target.  */
+
+extern const target_section_table *default_get_section_table ();
+
 /* From mem-break.c */
 
 extern int memory_remove_breakpoint (struct target_ops *,
diff --git a/gdb/testsuite/gdb.base/maint-info-sections.exp b/gdb/testsuite/gdb.base/maint-info-sections.exp
index b86812e4f12..06508de366d 100644
--- a/gdb/testsuite/gdb.base/maint-info-sections.exp
+++ b/gdb/testsuite/gdb.base/maint-info-sections.exp
@@ -223,9 +223,7 @@ gdb_test_multiple "maint info sections -all-objects" "" {
     }
 }
 
-# NOTE: We would like to check 'maint info target-sections' again
-# here, but GDB currently doesn't display the target sections table in
-# this case.  This is a bug and fill be fixed shortly!!
+check_maint_info_target_sections_output "with loaded symbol file"
 
 # Test the command line completion on 'maint info sections'.  First
 # the command line flag.
-- 
2.25.4