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

[PATCH v2] 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

---

New in v2:

 - Adjust call in program_space::exec_close
 - Rename private methods (suffix with `_1`) to avoid using them by
   mistake

---
 gdb/exec.c      | 10 +++++-----
 gdb/progspace.c |  3 ++-
 gdb/progspace.h | 29 ++++++++++++++++++++++++++---
 gdb/solib.c     |  2 +-
 4 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 0f9f9d076c68..f07d9d3ef338 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)
@@ -599,8 +599,8 @@ build_section_table (struct bfd *some_bfd)
    current set of target sections.  */
 
 void
-program_space::add_target_sections (const void *owner,
-				    const std::vector<target_section> &sections)
+program_space::add_target_sections_1
+  (const void *owner, const std::vector<target_section> &sections)
 {
   if (!sections.empty ())
     {
@@ -651,7 +651,7 @@ program_space::add_target_sections (struct objfile *objfile)
    OWNER must be the same value passed to add_target_sections.  */
 
 void
-program_space::remove_target_sections (const void *owner)
+program_space::remove_target_sections_1 (const void *owner)
 {
   gdb_assert (owner != NULL);
 
diff --git a/gdb/progspace.c b/gdb/progspace.c
index cca651fdb6f8..839707e9d71f 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -207,10 +207,11 @@ program_space::exec_close ()
     {
       /* Removing target sections may close the exec_ops target.
 	 Clear ebfd before doing so to prevent recursion.  */
+      bfd *saved_ebfd = ebfd.get ();
       ebfd.reset (nullptr);
       ebfd_mtime = 0;
 
-      remove_target_sections (&ebfd);
+      remove_target_sections (saved_ebfd);
 
       exec_filename.reset (nullptr);
     }
diff --git a/gdb/progspace.h b/gdb/progspace.h
index a480f560a77a..4db544b7d2e6 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_1 ((const void *) owner); }
+
+  void remove_target_sections (const shobj *owner)
+  { remove_target_sections_1 ((const void *) owner); }
+
+  void remove_target_sections (const bfd *owner)
+  { remove_target_sections_1 ((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_1 ((const void *) owner, sections); }
+
+  void add_target_sections (const shobj *owner,
+			    const std::vector<target_section> &sections)
+  { add_target_sections_1 ((const void *) owner, sections); }
+
+  void add_target_sections (const bfd *owner,
+			    const std::vector<target_section> &sections)
+  { add_target_sections_1 ((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_1 (const void *owner);
+
+  /* Helper for the public add_target_sections methods.  */
+  void add_target_sections_1 (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: 2d1777b530d7832db5d8d7017378354c28816554
-- 
2.42.0

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

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

Thank you for doing this.  I think the approach you've taken is a good
improvement over the status quo.

IIRC I wrote the current code back in the C days, when overloading was
more of a pain.

Simon>  - Rename private methods (suffix with `_1`) to avoid using them by
Simon>    mistake

C++ doesn't mean freedom from the _1 stuff :)
But it seems fine anyway, it's a private method.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

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

[Pedro Alves]

On 2023-10-20 04:31, 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.

Did you consider making the owner a union of (bfd *, objfile *, so_list *)
instead of void *?  Unions can have ctors, which would take care of the
implicit conversion.  Like:

union foo
{
  foo (int *i)
  {
    intptr = i;
  }
  foo (char *c)
  {
    charptr = c;
  }

  int *intptr;
  char *charptr;
};

void
function (foo owner)
{
  
}

int main ()
{
  int i;
  char c;
  function (&i);
  function (&c);
}

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

[Simon Marchi]

On 10/20/23 13:40, Pedro Alves wrote:
> On 2023-10-20 04:31, 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.
> 
> Did you consider making the owner a union of (bfd *, objfile *, so_list *)
> instead of void *?  Unions can have ctors, which would take care of the
> implicit conversion.  Like:
> 
> union foo
> {
>   foo (int *i)
>   {
>     intptr = i;
>   }
>   foo (char *c)
>   {
>     charptr = c;
>   }
> 
>   int *intptr;
>   char *charptr;
> };
> 
> void
> function (foo owner)
> {
>   
> }
> 
> int main ()
> {
>   int i;
>   char c;
>   function (&i);
>   function (&c);
> }
> 

Good idea!  I thought about using std::variant, but we're not requiring
C++17 just yet.  I'l try the union approach.

Simon