[PATCH 1/3] Make several more BFD globals thread-local

[Tom Tromey]

>> Among other things, PR gdb/31264 points out a race in bfd_check_format
>> -- it sets the error handler, which is a global.
>> Looking into this a bit more, I found several other possible races:
>> the "in_check_format" static local variable in
>> bfd_check_format_matches, and the contents of per_xvec_warn.
>> This patch makes all of these thread-local.
>> I don't actually think this is the best way to approach this.
>> "in_check_format" and the per-xvec warnings could be done by setting a
>> flag on the BFD, avoiding globals entirely.  I can do that if you
>> want; I just wasn't sure if that is desirable or not.

Nick> Actually, I think that I would prefer this alternative solution.

Nick> I am particularly worried about making the error handler thread local
[...]

Ok, I looked back at the code again.

What I meant when I wrote the above is that basically there are two sets
of globals to consider in this patch: the ones related to error emission
(_bfd_error_internal and error_handler_bfd), and then the other ones
related to the caching error message code (per_xvec_warn,
in_check_format).

I thought that making the latter ones thread-local felt hackish, because
they could somewhat easily be attributes of a BFD and not globals at
all.

_bfd_error_internal is a lot harder to change.  It could be done in
theory -- the idea would be to pass a BFD to every call to
_bfd_error_handler.  However, there are ~1400 such calls in BFD, which
is a quite a lot, and then there's also the unfortunate:

#define opcodes_error_handler _bfd_error_handler

... where, I assume, many calls don't even have access to a BFD.

Now, to address your concern about _bfd_error_internal (i.e., not making
it thread-local), I think we can add another thread-local flag, just for
bfd_check_format_matches.  The idea here would be that
_bfd_error_handler would check this flag, and then
_bfd_set_error_handler_caching would simply set this flag -- and not
change the BFD error handler at all.

I think this approach would be invisible to BFD clients.

Nick> This sounds like a lot of work however, and probably something that
Nick> ought to be done as part of a larger project to turn the BFD library
Nick> into a thread-safe and multi-threaded library.

FWIW I'd like to say that BFD really isn't too bad on this front.  The
main thing BFD has going for it is that nearly everything (and
especially in the reading side) is done via different BFD objects, so
there's a kind of natural lack of globals.

The remaining big issue here is interacting with the fd cache code,
which we discussed a little bit a while ago (last year?).  Here I think
the problem is that bfd_check_format_matches can change the BFD's iovec,
which interacts weirdly with cache.c; and as the cache involves globals
it means that the lock has to be held somewhere.

Tom