Re: [patch] Speed up find_pc_section

[Paul Pluzhnikov <ppluzhnikov@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2783 bytes --]

On Wed, Jul 22, 2009 at 10:16 AM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:

> On Wed, Jul 22, 2009 at 9:32 AM, Pedro Alves<pedro@codesourcery.com> 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.

>> You're also likely breaking xcoffsolib.c and the vmap code in exec.c, as
>> it calls free_objfile without going through observers.
>
> This would be handled by the 'remove_objfile' observer, I think.
>
>> Given the new_objfile observer, do we still need the exec_changed and
>> solib observers?
>
> No, I've removed that (new patch attached).

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.

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<pedro@codesourcery.com> 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 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.

Thanks,
-- 
Paul Pluzhnikov

2009-07-22  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* objfiles.c (allocate_objfile): Must rebuild section map.
	(free_objfile, objfile_relocate): Likewise.
	(set_objfiles_updated_on_solib_activity): Remove.
	(_initialize_objfiles): Adjust.

[-- Attachment #2: gdb-find_pc_section-20090722-3.txt --]
[-- Type: text/plain, Size: 1766 bytes --]

Index: objfiles.c
===================================================================
RCS file: /cvs/src/src/gdb/objfiles.c,v
retrieving revision 1.86
diff -u -p -u -r1.86 objfiles.c
--- objfiles.c	21 Jul 2009 20:54:30 -0000	1.86
+++ objfiles.c	22 Jul 2009 17:55:41 -0000
@@ -235,6 +235,8 @@ allocate_objfile (bfd *abfd, int flags)
   /* Save passed in flag bits. */
   objfile->flags |= flags;
 
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
+
   return (objfile);
 }
 
@@ -501,6 +503,7 @@ free_objfile (struct objfile *objfile)
   obstack_free (&objfile->objfile_obstack, 0);
   xfree (objfile);
   objfile = NULL;
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 
 static void
@@ -682,6 +685,7 @@ objfile_relocate (struct objfile *objfil
 
   /* Relocate breakpoints as necessary, after things are relocated. */
   breakpoint_re_set ();
+  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 \f
 /* Many places in gdb want to test just to see if we have any partial
@@ -998,19 +1002,8 @@ set_objfiles_updated_on_exe_change (void
   objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
 }
 
-/* Set objfiles_updated_p so section map will be rebuilt next time it
-   is used.  Called by solib_loaded/unloaded observer.  */
-
-static void
-set_objfiles_updated_on_solib_activity (struct so_list *so_list)
-{
-  objfiles_updated_p = 1;  /* Rebuild section map next time we need it.  */
-}
-
 void
 _initialize_objfiles (void)
 {
   observer_attach_executable_changed (set_objfiles_updated_on_exe_change);
-  observer_attach_solib_loaded (set_objfiles_updated_on_solib_activity);
-  observer_attach_solib_unloaded (set_objfiles_updated_on_solib_activity);
 }