Re: [PATCH 1/2] gdb: move all bfd_cache_close_all calls in gdb_bfd.c

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> Better I think would be to track down where the bfd is being opened
> Andrew> and add a corresponding bfd_cache_close_all() call elsewhere in GDB
> Andrew> once we've finished doing whatever it is that caused us to open the
> Andrew> bfd in the first place.
>
> I had contemplated this solution as well, though using bfd_cache_close
> on the specific BFD, instead.
>
> Like you point out, though, it seems finicky to get right.  We could
> move up the call chain and add the close calls at some higher level, but
> even that seems hard to be assured of.
>
> Andrew>   (a) Just before we display a GDB prompt.  We display a prompt after
> Andrew>   completing a command, and GDB is about to enter an idle state
> Andrew>   waiting for further input from the user (or in async mode, for an
> Andrew>   inferior event).  If while we are in this idle state the user
> Andrew>   changes the on-disk file(s) then we would like GDB to notice this
> Andrew>   the next time it leaves its idle state, e.g. the next time the user
> Andrew>   executes a command, or when an inferior event arrives,
>
> Andrew>   (b) When we resume the inferior.  In synchronous mode, resuming the
> Andrew>   inferior is another time when GDB is blocked and sitting idle, but
> Andrew>   in this case we don't display a prompt.  As with (a) above, when an
> Andrew>   inferior event arrives we want GDB to notice any changes to on-disk
> Andrew>   files.
>
> I've been working on moving DWARF reading to the background.  For the
> most part, your plan won't be an issue -- gdb pre-reads the relevant
> section data on the main thread and doesn't require the BFD to be open
> when scanning.

I did remember you mentioning your background debug parsing work, but I
couldn't find any emails about it, but maybe we talked in IRC...

> However, there is one case where this will change: opening dwo files.
> Now, the way the background reading is implemented, the worst case here
> is some inefficiency: the main thread might close all the BFDs, and then
> the background reader might immediately reopen one.

Right now I don't see bfd/cache.c as thread safe, so are you planning to
make it thread safe?  Or place restrictions within GDB to prevent
multiple threads accessing BFDs at the same time (e.g. background
reading can only happen when GDB itself is known to be idle)?

> Maybe this isn't something to worry about?  I could make the background
> reader also call bfd_cache_close for the DWO BFDs, to ensure they are
> closed.  (This only really matters for the "rebuild" case, not any sort
> of exec case, because DWOs aren't really part of the inferior.)

I figured all I'm really doing is moving the bfd_cache_close_all()
calls, we already had them randomly scattered throughout GDB anyway, so
any background reader you implement would already need to deal with the
cache being cleared under its feet (or prevent that from happening when
its running), so I didn't think I'd made things harder for you ... just
moved the problem around a bit.

Also, I figured it's not that hard to implement a
gdb_bfd_cache_close_all() wrapper which counts the number of threads
that are actively using the cache, and only calls bfd_cache_close_all()
if the "other" thread isn't currently running.  Of course, that assumes
that the cache itself is somehow thread-safe.

Another option is to only perform background reading while GDB is
in-active, in which case, my current bfd_cache_close_all() calls are
actually the points at which your backgound threads can safely start
working without worrying about GDB also accessing the bfd cache --
though, you'd still need a mechanism to stop the background reader once
GDB becomes active again.

The other option I considered, is that you could add a mechanism to
allow BFDs to be opened without going through the BFD cache.  In this
case, I think your thread safety issues go away, and also, you no longer
care about my bfd_cache_close_all() calls.  The BFD cache exists to
handle systems that only allow a small number of open files (they quote
20 as being a limit on some systems), so avoiding the cache might run
into an open FD limit issue ... but I'm not sure if this low limit
problem is really a thing on systems that are going to be running
multiple threads?

>
> Andrew> Right now, the only other users of the observers that I'm connecting
> Andrew> too are the extension languages, specifically, Python.  I suspect it
> Andrew> must be possible for Python to carry out actions that can cause the
> Andrew> bfd cache to be populated, so maybe there is a risk here.
>
> Andrew> To counter this risk, we could move the bfd_cache_close_all() calls
> Andrew> out of observers, and move them into GDB core close too, but after the
> Andrew> two observers in questions are actually called, so into
> Andrew> top_level_prompt for the before_prompt observer and into
> Andrew> notify_target_resumed for the target_resumed observer.
>
> Andrew> Another choice would be to add a bfd_cache_close_all() into
> Andrew> gdbpy_enter::~gdbpy_enter, so whenever we call into a Python
> Andrew> extension, we always clear the bfd cache once we're done.
>
> Andrew> For now I haven't made either of these changes, maybe I'm worrying
> Andrew> about nothing?  I'd appreciate peoples thoughts.
>
> Do we know of specific Python APIs that could cause problems?

Well the generic (maybe unhelpful) answer is gdb.execute().

>
> I guess anything that reads memory if trust-readonly-sections is
> enabled?  That's the only one I could think of offhand.
>
> Maybe we need a combo approach where some calls call bfd_cache_close at
> the end.

I'd really prefer to avoid relying on folk to remember that they need to
call bfd_cache_close_all() in certain places.  I think its far too easy
to forget, and these are bugs that are only going to crop up once in a
while .... A user is running a particular Python extension, and performs
a particular action, just at the same time as they recompile their test
binary ... and suddenly GDB "misses" that the test executable changed.
Bugs like this can be a nightmare to track down.

Right now, I'm thinking I'll switch away from using the observers, and
just ensure that bfd_cache_close_all() is called after all the observers
have run.

Thanks,
Andrew