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

  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(-)


base-commit: 2d1777b530d7832db5d8d7017378354c28816554
  

>>>>> "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
  

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);
}
  

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