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:

>>> 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.
>
> Andrew> Right now I don't see bfd/cache.c as thread safe, so are you planning to
> Andrew> make it thread safe?
>
> Yeah.  I sent a series to the binutils list recently.

Neat.  It sounds like your background DWARF reader is further along than
I'd thought.

I'm trying to figure out what I should do to move this patch forward?

I guess the existing bfd_cache_close_all() calls could cause problems
for your background reader if the on-disk file changes between two reads
from a BFD, and if GDB's main thread calls bfd_cache_close_all() in
between those reads, e.g. one BFD section might indicate an offset into
another section, and after the on-disk file changes the section offset
is no longer valid...

But you could just ignore the existing bfd_cache_close_all() calls, and
rely on BFD to reopen the files as needed, and rely on GDB's error
detection to handle any race conditions caused by the on-disk file
changing...

I'm happy to do whatever is needed to integrate this patch with your
upcoming work, I'm just trying to figure out what work needs to be done:
if you're currently ignoring bfd_cache_close_all() calls, and relying on
BFD to reopen, and GDB's error detection, then I guess this patch is
fine as is.  But if you have some other solution for the
bfd_cache_close_all() calls, then I guess this patch is stepping on your
toes, in which case, I want to help combine these two work items if I
can.

>
> Andrew> The other option I considered, is that you could add a mechanism to
> Andrew> allow BFDs to be opened without going through the BFD cache.  In this
> Andrew> case, I think your thread safety issues go away, and also, you no longer
> Andrew> care about my bfd_cache_close_all() calls.  The BFD cache exists to
> Andrew> handle systems that only allow a small number of open files (they quote
> Andrew> 20 as being a limit on some systems), so avoiding the cache might run
> Andrew> into an open FD limit issue ... but I'm not sure if this low limit
> Andrew> problem is really a thing on systems that are going to be running
> Andrew> multiple threads?
>
> We could definitely do this for the files the DWARF reader opens in the
> worker threads.
>
> Not sure if I will do this or not though.  Adding an explicit
> bfd_cache_close call would also take care of the problem, and I think
> the reader only needs the file open from the call to open_dwo_file (in
> open_and_init_dwo_file) through mapping the sections -- and they could
> be mapped more eagerly in open_and_init_dwo_file.

It sounds like you're already heading towards making the bfd cache
thread safe, so I don't think these ideas would be needed.

Thanks,
Andrew

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