[PATCH] gdb: fix owner passed to remove_target_sections in clear_solib

[PATCH] gdb: fix owner passed to remove_target_sections in clear_solib

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

Commit 8971d2788e7 ("gdb: link so_list using intrusive_list") introduced
a bug in clear_solib.  Instead of passing an `so_list *` to
remove_target_sections, it passed an `so_list **`.  This was not caught
by the compiler, because remove_target_sections takes a `void *` as the
"owner", so you can pass it any pointer and it won't complain.

This happened because I previously had a patch to change the type of the
disposer parameter to be a reference rather than a pointer, so had to
change `so` to `&so`.  When dropping that patch, I forgot to revert this
bit and / or it got re-introduced when handling subsequent merge
conflicts.  And I didn't properly retest.

Fix that, but try to make things less error prone.  Make the current
add_target_sections and remove_target_sections methods of program_space
private, and add wrappers that are typed, with the types we expect
(bfd *, objfile *, so_list *).  Perhaps there is a more elegant way to
do this, but I think that this is already better / safer than what we
have, as it would have caught my mistake.

Change-Id: I600cab5ea0408ccc5638467b760768161ca3036c
---
 gdb/exec.c      |  4 ++--
 gdb/progspace.h | 29 ++++++++++++++++++++++++++---
 gdb/solib.c     |  2 +-
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 0f9f9d076c68..5a21154c6f13 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -492,8 +492,8 @@ exec_file_attach (const char *filename, int from_tty)
       /* Add the executable's sections to the current address spaces'
 	 list of sections.  This possibly pushes the exec_ops
 	 target.  */
-      current_program_space->add_target_sections (&current_program_space->ebfd,
-						  sections);
+      current_program_space->add_target_sections
+	(current_program_space->ebfd.get (), sections);
 
       /* Tell display code (if any) about the changed file name.  */
       if (deprecated_exec_file_display_hook)
diff --git a/gdb/progspace.h b/gdb/progspace.h
index a480f560a77a..c8efc64965ae 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -284,12 +284,28 @@ struct program_space
   bool empty ();
 
   /* Remove all target sections owned by OWNER.  */
-  void remove_target_sections (const void *owner);
+  void remove_target_sections (const objfile *owner)
+  { remove_target_sections ((const void *) owner); }
+
+  void remove_target_sections (const shobj *owner)
+  { remove_target_sections ((const void *) owner); }
+
+  void remove_target_sections (const bfd *owner)
+  { remove_target_sections ((const void *) owner); }
 
   /* Add the sections array defined by SECTIONS to the
      current set of target sections.  */
-  void add_target_sections (const void *owner,
-			    const std::vector<target_section> &sections);
+  void add_target_sections (const objfile *owner,
+			    const std::vector<target_section> &sections)
+  { add_target_sections ((const void *) owner, sections); }
+
+  void add_target_sections (const shobj *owner,
+			    const std::vector<target_section> &sections)
+  { add_target_sections ((const void *) owner, sections); }
+
+  void add_target_sections (const bfd *owner,
+			    const std::vector<target_section> &sections)
+  { add_target_sections ((const void *) owner, sections); }
 
   /* Add the sections of OBJFILE to the current set of target
      sections.  They are given OBJFILE as the "owner".  */
@@ -376,6 +392,13 @@ struct program_space
   registry<program_space> registry_fields;
 
 private:
+  /* Helper for the public remove_target_sections methods.  */
+  void remove_target_sections (const void *owner);
+
+  /* Helper for the public add_target_sections methods.  */
+  void add_target_sections (const void *owner,
+			    const std::vector<target_section> &sections);
+
   /* The set of target sections matching the sections mapped into
      this program space.  Managed by both exec_ops and solib.c.  */
   std::vector<target_section> m_target_sections;
diff --git a/gdb/solib.c b/gdb/solib.c
index 71970636dc58..b9fb911a8101 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1195,7 +1195,7 @@ clear_solib (void)
   current_program_space->so_list.clear_and_dispose ([] (shobj *so)
     {
       notify_solib_unloaded (current_program_space, *so);
-      current_program_space->remove_target_sections (&so);
+      current_program_space->remove_target_sections (so);
       delete so;
     });
 

base-commit: f005ccb4bcc2fa79f005973728088d590b5e74fd
-- 
2.42.0

Re: [PATCH] gdb: fix owner passed to remove_target_sections in clear_solib

[Simon Marchi]

On 2023-10-19 16:52, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
> 
> Commit 8971d2788e7 ("gdb: link so_list using intrusive_list") introduced
> a bug in clear_solib.  Instead of passing an `so_list *` to
> remove_target_sections, it passed an `so_list **`.  This was not caught
> by the compiler, because remove_target_sections takes a `void *` as the
> "owner", so you can pass it any pointer and it won't complain.
> 
> This happened because I previously had a patch to change the type of the
> disposer parameter to be a reference rather than a pointer, so had to
> change `so` to `&so`.  When dropping that patch, I forgot to revert this
> bit and / or it got re-introduced when handling subsequent merge
> conflicts.  And I didn't properly retest.
> 
> Fix that, but try to make things less error prone.  Make the current
> add_target_sections and remove_target_sections methods of program_space
> private, and add wrappers that are typed, with the types we expect
> (bfd *, objfile *, so_list *).  Perhaps there is a more elegant way to
> do this, but I think that this is already better / safer than what we
> have, as it would have caught my mistake.

Huh, so this causes many test failures again.  I missed updating a
remove_target_sections call in program_space::exec_close.  I updated the
call to add_target_sections to pass a `bfd *` instead of a
`gdb_bfd_ref_ptr *`, but missed updating the matching call to
remove_target_sections.  It builds because the call is inside
program_space, so is able to reach the private remove_target_sections,
despite passing an unexpected type.

I'll send a v2 shortly.

Simon