This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch 1/3] Refactor/simplify (+fix) svr4_current_sos
On Thursday 13 October 2011 00:17:55, Jan Kratochvil wrote:
> On Thu, 06 Oct 2011 20:30:15 +0200, Pedro Alves wrote:
> > On Monday 03 October 2011 22:54:24, Jan Kratochvil wrote:
> > > gdb/
> > > 2011-10-03 Jan Kratochvil <jan.kratochvil@redhat.com>
> > > Paul Pluzhnikov <ppluzhnikov@google.com>
> > >
> > > * defs.h (struct so_list): New forward declaration.
> > > (make_cleanup_free_so): New declaration.
> > > * solib-svr4.c (ignore_first_link_map_entry): Remove.
> > > (svr4_free_so): Move here. Handle NULL so->lm_info.
> >
> > Move what here? Looks like a stale comment.
>
> svr4_free_so is moved upwards in this code, as it is used earlier.
>
> From what I know about the coding style for GDB it is preferred to move
> functions rather than to create new forward declarations for them.
Yes.
>
> Also ChangeLog enties should be present in the same order as chunks of the
> patch.
Yes, I do the same.
>
> Which all leads to this statement. Used now:
> (svr4_free_so): Move the function here from downwards. Handle NULL
> so->lm_info.
Yes, much better thanks. "Move here" made is sound like you were
moving code from some other function into svr4_free_so.
(foofunction): Delete handling of bar.
(barfunction): Move here.
>
> > > @@ -1029,58 +1028,93 @@ svr4_current_sos (void)
> > > SVR4, it has no name. For others (Solaris 2.3 for example), it
> > > does have a name, so we can no longer use a missing name to
> > > decide when to ignore it. */
> > > - else if (ignore_first_link_map_entry (new) && ldsomap == 0)
> > > + if (ignore_first && lm_prev (new) == 0)
> > > {
> > > + struct svr4_info *info = get_svr4_info ();
> > > +
> > > info->main_lm_addr = new->lm_info->lm_addr;
> >
> > A shame that this is still hidden here IMO, and dependent on
> > how you call the function, though not documented in the function
> > header. An alternative would be to move this "ignore first" logic to
> > the caller, making the caller itself delete the first entry in the list
> > if it wanted to, and setting main_lm_addr. We already have to allocate/free
> > this so_list, so nothing seems be to lost that way.
>
> While the idea is good the implementation has some issues, it also adds LoCs:
> gdb/solib-svr4.c | 81 ++++++++++++++++++++++++++++++------------------------
> 1 files changed, 45 insertions(+), 36 deletions(-)
>
> "For some versions of SVR4, it has no name." - I am not sure if it means the
> name is "" (empty) or that the name is unreadable and thus it would print
> false warning messages (while trying to create the list entry going to be
> freed by the caller).
>
> The caller will need to reiterate the list and free entries without names,
> which is more complicated than in the callee.
Ah, I was only thinking of the `ignore_first' argument handling. I agree
that to move the `ignore_first' argument handling to the caller, you'd want
to move the no-names entries handling too, and then, I agree that the result
isn't better. Thanks for trying it out.
--
Pedro Alves