From: Andrew Burgess Date: Fri, 12 Feb 2021 11:39:31 +0000 (+0000) Subject: gdb: move get_section_table from exec_target to dummy_target X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=336aa7b740c64070ae14d2364edddb7df7bce011;p=binutils-gdb.git gdb: move get_section_table from exec_target to dummy_target 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. --- diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 841d51bc3ca..f50d89f25c1 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,14 @@ +2021-02-24 Andrew Burgess + + * 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. + 2021-02-24 Andrew Burgess * exec.c (exec_target::close): Call new clear_target_sections diff --git a/gdb/exec.c b/gdb/exec.c index c55a41aa8a1..8e3c19ec272 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; @@ -775,7 +774,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 available_memory = section_table_available_memory (offset, len, *table); @@ -884,12 +884,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 ¤t_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 ¤t_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 66d46e2facb..ee93c5cf395 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; } @@ -2436,6 +2436,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 17e38ebc416..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 will 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.