This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Speed up find_pc_section

On Wednesday 22 July 2009 19:10:48, Paul Pluzhnikov wrote:
> On Wed, Jul 22, 2009 at 10:16 AM, Paul Pluzhnikov<> wrote:
> > On Wed, Jul 22, 2009 at 9:32 AM, Pedro Alves<> wrote:
> >
> >> In the OBJF_USERLOADED case, you rebuild the map when you don't
> >> really need to.
> >
> > I think it's pretty clear that what I really need is a new 'remove_objfile'
> > kind of observer.
> I am not thinking clearly today :-(


> I don't need a new observer: free_objfile is in the same source, so I just
> need to set the flag there as well.


> I feel going in circles here. The exec_changed observer was to address
> reread_symbols, which doesn't create a new objfile. I believe that's
> still necessary.

Ah!  I see.  That, is pretty ugly/nasty.  I think that objfiles.c would
be a better home for reread_symbols, which would also remove that
requirement.  A new function objfiles.c:objfiles_changed that is called
from reread_symbols, would still be better, that abusing that
observer, IMHO.  Do you feel like adding it?  If not, I won't insist;
the observer is fine for now, *if* you do something for me please:  Could
you please add a comment in _initialize_objfiles explaining that why
it is needed, and spell out "reread_symbols" and "objfile_updated_p" in
the comment, so that grepping finds it.

> The solib_load/unload observers don't appear to be needed though: the load
> case will create a new objfile, the unload case (when !OBJF_USERLOADED)
> will do free_objfile).


> On Wed, Jul 22, 2009 at 10:40 AM, Pedro Alves<> wrote:
> >> I think it's pretty clear that what I really need is a new 'remove_objfile'
> >> kind of observer.
> >
> > Would setting objfiles_updated_p from e.g., unlink_objfile work?
> Exactly. Though I think free_objfile is a more logical place for it.

I was thinking that when you look for a section, you loop over linked in
objfiles, while free_objfile could in principle also be called even
if the objfile hadn't been linked in.  But that's a minor detail;
free_objfile is fine.

> > I think that the need for the solib load/unload observer
> > would go away too if the map is flushed on objfile
> > removal/freeing/unlinking as well?
> Yes.


> > Come to look at it deeper, what is happening with
> > symbol_file_add_with_addrs_or_offsets, if the objfile has only
> > minimal symbols (but still has sections)? ?allocate_objfile is called,
> > which builds the section table, but, there's a path that does an
> > early return before calling the new_objfile observer, if there are
> > no symbols.
> That sounds like a bug: we created a new_objfile, but didn't notify
> observers.


> Eeasy enough to work around though: I'll set the flag in allocate_objfile
> as well.
> > It would be cleaner and easier to review, easier for you (I think),
> > and better for the archives in the future, IMHO. ?But I don't mind
> > much if a new patch is cooked on top.
> Let's try for one more fix before reverting ...
> Re-tested on Linux/x86_64 with no new failures.

Looks good to me now (minus missing comment).  Thank you!

Pedro Alves

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]