[1/2] Thread-safety improvements for bfd_check_format_matches
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_binutils_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_binutils_check--master-arm |
success
|
Testing passed
|
Commit Message
A gdb bug found that bfd_check_format_matches has some data races when
called from multiple threads.
In particular, it changes the BFD error handler, which is a global.
It also has a local static variable ("in_check_format") that is used
for recursion detection. And, finally, it may emit warnings to the
per-xvec warning array, which is a global.
This patch removes all the races here. I believe it does not change
the current semantics, but I was unable to get the test file that
would let me test that.
The first part of patch is to change _bfd_error_handler to directly
handle the needs of bfd_check_format_matches. This way, the error
handler does not need to be changed.
This change lets us use the new per-thread global
(error_handler_messages, replacing error_handler_bfd) to also remove
the need for in_check_format -- a single variable suffices.
Finally, the global per-xvec array is replaced with a new type that
holds the error messages. The outermost such type is stack-allocated
in bfd_check_format_matches.
I tested this using the binutils test suite. I also built gdb with
thread sanitizer and ran the test case that was noted as failing.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264
---
bfd/bfd.c | 68 ++++++++++++++++++++++++++++++++++++++-----------
bfd/format.c | 70 +++++++++++++++++++++++----------------------------
bfd/libbfd.h | 17 +++++++++++--
bfd/targets.c | 59 ++++++++++++++++++++++++++++++-------------
4 files changed, 142 insertions(+), 72 deletions(-)
Comments
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> This patch removes all the races here. I believe it does not change
Tom> the current semantics, but I was unable to get the test file that
Tom> would let me test that.
Alan sent me the relevant file off-list, and I verified that the output
of 'nm-new' does not change after this series is applied.
I've locally updated the commit message for this patch to reflect that.
thanks,
Tom
On Sun, Mar 24, 2024 at 03:08:05PM -0600, Tom Tromey wrote:
> A gdb bug found that bfd_check_format_matches has some data races when
> called from multiple threads.
>
> In particular, it changes the BFD error handler, which is a global.
> It also has a local static variable ("in_check_format") that is used
> for recursion detection. And, finally, it may emit warnings to the
> per-xvec warning array, which is a global.
>
> This patch removes all the races here. I believe it does not change
> the current semantics, but I was unable to get the test file that
> would let me test that.
I suspect you have changed things a little. The curent per_xvec_warn
array has an extra element to cache messages emitted when xvec is
NULL. I added that just in case such a thing ever happened. I don't
think it does, so I'm not at all concerned about a change in behaviour
when xvec is NULL.
However, the new _bfd_per_xvec_warn does seem to be a little weird
should messages->abfd-xvec ever be NULL. It seems it will create a
new entry in the list for a NULL targ that then might be reused for a
later non-NULL targ message. Also, the description for the new
_bfd_per_xvec_warn needs updating and maybe the function should be
made static and moved from targets.c to bfd.c.
> The first part of patch is to change _bfd_error_handler to directly
> handle the needs of bfd_check_format_matches. This way, the error
> handler does not need to be changed.
>
> This change lets us use the new per-thread global
> (error_handler_messages, replacing error_handler_bfd) to also remove
> the need for in_check_format -- a single variable suffices.
>
> Finally, the global per-xvec array is replaced with a new type that
> holds the error messages. The outermost such type is stack-allocated
> in bfd_check_format_matches.
>
> I tested this using the binutils test suite. I also built gdb with
> thread sanitizer and ran the test case that was noted as failing.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31264
> ---
> bfd/bfd.c | 68 ++++++++++++++++++++++++++++++++++++++-----------
> bfd/format.c | 70 +++++++++++++++++++++++----------------------------
> bfd/libbfd.h | 17 +++++++++++--
> bfd/targets.c | 59 ++++++++++++++++++++++++++++++-------------
> 4 files changed, 142 insertions(+), 72 deletions(-)
>
> SYNOPSIS
> - bfd_error_handler_type _bfd_set_error_handler_caching (bfd *);
> + struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *);
>
> DESCRIPTION
> Set the BFD error handler function to one that stores messages
> - to the per_xvec_warn array. Returns the previous function.
> + to the per_xvec_warn array. Returns the previous BFD to which
> + messages are stored. Note that two sequential calls to this
> + with a non-NULL BFD will cause output to be dropped, rather
> + than gathered.
> */
non-NULL BFD?
>>>>> "Alan" == Alan Modra <amodra@gmail.com> writes:
Alan> I suspect you have changed things a little. The curent per_xvec_warn
Alan> array has an extra element to cache messages emitted when xvec is
Alan> NULL. I added that just in case such a thing ever happened. I don't
Alan> think it does, so I'm not at all concerned about a change in behaviour
Alan> when xvec is NULL.
Alan> However, the new _bfd_per_xvec_warn does seem to be a little weird
Alan> should messages->abfd-xvec ever be NULL. It seems it will create a
Alan> new entry in the list for a NULL targ that then might be reused for a
Alan> later non-NULL targ message. Also, the description for the new
Alan> _bfd_per_xvec_warn needs updating and maybe the function should be
Alan> made static and moved from targets.c to bfd.c.
Thanks for the review.
I've made these changes, and since I was already in there I went ahead
and fixed the NULL xvec issue as well.
I'll send v2 momentarily.
Tom
@@ -1534,10 +1534,17 @@ err_sprintf (void *stream, const char *fmt, ...)
return total;
}
-/* Communicate the bfd processed by bfd_check_format_matches to the
- error handling function error_handler_sprintf. */
+/* Communicate the error-message container processed by
+ bfd_check_format_matches to the error handling function
+ error_handler_sprintf. When non-NULL, _bfd_error_handler will call
+ error_handler_sprintf; when NULL, _bfd_error_internal will be used
+ instead. */
-static bfd *error_handler_bfd;
+static TLS struct per_xvec_messages *error_handler_messages;
+
+/* A special value for error_handler_messages that indicates that the
+ error should simply be ignored. */
+#define IGNORE_ERROR_MESSAGES ((struct per_xvec_messages *) -1)
/* An error handler that prints to a string, then dups that string to
a per-xvec cache. */
@@ -1554,12 +1561,12 @@ error_handler_sprintf (const char *fmt, va_list ap)
_bfd_print (err_sprintf, &error_stream, fmt, ap);
size_t len = error_stream.ptr - error_buf;
- struct per_xvec_message **warn
- = _bfd_per_xvec_warn (error_handler_bfd->xvec, len + 1);
- if (*warn)
+ struct per_xvec_message *warn
+ = _bfd_per_xvec_warn (error_handler_messages, len + 1);
+ if (warn)
{
- memcpy ((*warn)->message, error_buf, len);
- (*warn)->message[len] = 0;
+ memcpy (warn->message, error_buf, len);
+ warn->message[len] = 0;
}
}
@@ -1597,7 +1604,14 @@ _bfd_error_handler (const char *fmt, ...)
va_list ap;
va_start (ap, fmt);
- _bfd_error_internal (fmt, ap);
+ if (error_handler_messages == IGNORE_ERROR_MESSAGES)
+ {
+ /* Nothing. */
+ }
+ else if (error_handler_messages != NULL)
+ error_handler_sprintf (fmt, ap);
+ else
+ _bfd_error_internal (fmt, ap);
va_end (ap);
}
@@ -1628,18 +1642,42 @@ INTERNAL_FUNCTION
_bfd_set_error_handler_caching
SYNOPSIS
- bfd_error_handler_type _bfd_set_error_handler_caching (bfd *);
+ struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *);
DESCRIPTION
Set the BFD error handler function to one that stores messages
- to the per_xvec_warn array. Returns the previous function.
+ to the per_xvec_warn array. Returns the previous BFD to which
+ messages are stored. Note that two sequential calls to this
+ with a non-NULL BFD will cause output to be dropped, rather
+ than gathered.
*/
-bfd_error_handler_type
-_bfd_set_error_handler_caching (bfd *abfd)
+struct per_xvec_messages *
+_bfd_set_error_handler_caching (struct per_xvec_messages *messages)
+{
+ struct per_xvec_messages *old = error_handler_messages;
+ if (old == NULL)
+ error_handler_messages = messages;
+ else
+ error_handler_messages = IGNORE_ERROR_MESSAGES;
+ return old;
+}
+
+/*
+INTERNAL_FUNCTION
+ _bfd_restore_error_handler_caching
+
+SYNOPSIS
+ void _bfd_restore_error_handler_caching (struct per_xvec_messages *);
+
+DESCRIPTION
+ Reset the BFD error handler function an earlier value.
+*/
+
+void
+_bfd_restore_error_handler_caching (struct per_xvec_messages *old)
{
- error_handler_bfd = abfd;
- return bfd_set_error_handler (error_handler_sprintf);
+ error_handler_messages = old;
}
/*
@@ -272,10 +272,33 @@ clear_warnmsg (struct per_xvec_message **list)
*list = NULL;
}
+/* Free all the storage in LIST. Note that the first element of LIST
+ is special and is assumed to be stack-allocated. TARG is used for
+ re-issuing warning messages. If TARG is non-NULL, then any
+ messages in LIST with a matching target are issued. If TARG is
+ NULL, then messages are only reissued if they are all associated
+ with a single BFD target. */
+
static void
-null_error_handler (const char *fmt ATTRIBUTE_UNUSED,
- va_list ap ATTRIBUTE_UNUSED)
+print_and_clear_messages (struct per_xvec_messages *list,
+ const bfd_target *targ)
{
+ struct per_xvec_messages *iter = list;
+
+ if (targ == NULL && list->next == NULL)
+ print_warnmsg (&list->messages);
+
+ while (iter != NULL)
+ {
+ struct per_xvec_messages *next = iter->next;
+
+ if (iter->targ == targ)
+ print_warnmsg (&iter->messages);
+ clear_warnmsg (&iter->messages);
+ if (iter != list)
+ free (iter);
+ iter = next;
+ }
}
/*
@@ -313,8 +336,8 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
unsigned int initial_section_id = _bfd_section_id;
struct bfd_preserve preserve, preserve_match;
bfd_cleanup cleanup = NULL;
- bfd_error_handler_type orig_error_handler;
- static int in_check_format;
+ struct per_xvec_messages messages = { abfd, NULL, NULL, NULL };
+ struct per_xvec_messages *orig_messages;
if (matching != NULL)
*matching = NULL;
@@ -345,11 +368,7 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
/* Don't report errors on recursive calls checking the first element
of an archive. */
- if (in_check_format)
- orig_error_handler = bfd_set_error_handler (null_error_handler);
- else
- orig_error_handler = _bfd_set_error_handler_caching (abfd);
- ++in_check_format;
+ orig_messages = _bfd_set_error_handler_caching (&messages);
preserve_match.marker = NULL;
if (!bfd_preserve_save (abfd, &preserve, NULL))
@@ -591,15 +610,9 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
if (preserve_match.marker != NULL)
bfd_preserve_finish (abfd, &preserve_match);
bfd_preserve_finish (abfd, &preserve);
- bfd_set_error_handler (orig_error_handler);
+ _bfd_restore_error_handler_caching (orig_messages);
- struct per_xvec_message **list = _bfd_per_xvec_warn (abfd->xvec, 0);
- if (*list)
- print_warnmsg (list);
- list = _bfd_per_xvec_warn (NULL, 0);
- for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
- clear_warnmsg (list++);
- --in_check_format;
+ print_and_clear_messages (&messages, abfd->xvec);
/* File position has moved, BTW. */
return true;
@@ -643,27 +656,8 @@ bfd_check_format_matches (bfd *abfd, bfd_format format, char ***matching)
if (preserve_match.marker != NULL)
bfd_preserve_finish (abfd, &preserve_match);
bfd_preserve_restore (abfd, &preserve);
- bfd_set_error_handler (orig_error_handler);
- struct per_xvec_message **list = _bfd_per_xvec_warn (NULL, 0);
- struct per_xvec_message **one = NULL;
- for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
- {
- if (list[i])
- {
- if (!one)
- one = list + i;
- else
- {
- one = NULL;
- break;
- }
- }
- }
- if (one)
- print_warnmsg (one);
- for (size_t i = 0; i < _bfd_target_vector_entries + 1; i++)
- clear_warnmsg (list++);
- --in_check_format;
+ _bfd_restore_error_handler_caching (orig_messages);
+ print_and_clear_messages (&messages, NULL);
return false;
}
@@ -934,7 +934,9 @@ void _bfd_clear_error_data (void) ATTRIBUTE_HIDDEN;
char *bfd_asprintf (const char *fmt, ...) ATTRIBUTE_HIDDEN;
-bfd_error_handler_type _bfd_set_error_handler_caching (bfd *) ATTRIBUTE_HIDDEN;
+struct per_xvec_messages *_bfd_set_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_HIDDEN;
+
+void _bfd_restore_error_handler_caching (struct per_xvec_messages *) ATTRIBUTE_HIDDEN;
const char *_bfd_get_error_program_name (void) ATTRIBUTE_HIDDEN;
@@ -3699,7 +3701,18 @@ struct per_xvec_message
char message[];
};
-struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t) ATTRIBUTE_HIDDEN;
+/* A list of per_xvec_message objects. The targ field
+ indicates which xvec this list holds. The abfd field is
+ only needed in the root entry of the list. */
+struct per_xvec_messages
+{
+ bfd *abfd;
+ const bfd_target *targ;
+ struct per_xvec_message *messages;
+ struct per_xvec_messages *next;
+};
+
+struct per_xvec_message *_bfd_per_xvec_warn (struct per_xvec_messages *, size_t) ATTRIBUTE_HIDDEN;
#ifdef __cplusplus
}
@@ -1457,9 +1457,6 @@ const bfd_target *const *const bfd_associated_vector = _bfd_associated_vector;
number of entries that the array could possibly need. */
const size_t _bfd_target_vector_entries = ARRAY_SIZE (_bfd_target_vector);
-/* A place to stash a warning from _bfd_check_format. */
-static struct per_xvec_message *per_xvec_warn[ARRAY_SIZE (_bfd_target_vector)
- + 1];
/* This array maps configuration triplets onto BFD vectors. */
@@ -1488,11 +1485,22 @@ INTERNAL
. char message[];
.};
.
+.{* A list of per_xvec_message objects. The targ field
+. indicates which xvec this list holds. The abfd field is
+. only needed in the root entry of the list. *}
+.struct per_xvec_messages
+.{
+. bfd *abfd;
+. const bfd_target *targ;
+. struct per_xvec_message *messages;
+. struct per_xvec_messages *next;
+.};
+.
INTERNAL_FUNCTION
_bfd_per_xvec_warn
SYNOPSIS
- struct per_xvec_message **_bfd_per_xvec_warn (const bfd_target *, size_t);
+ struct per_xvec_message *_bfd_per_xvec_warn (struct per_xvec_messages *, size_t);
DESCRIPTION
Return a location for the given target xvec to use for
@@ -1505,19 +1513,36 @@ DESCRIPTION
pointer to a NULL pointer on allocation failure.
*/
-struct per_xvec_message **
-_bfd_per_xvec_warn (const bfd_target *targ, size_t alloc)
+struct per_xvec_message *
+_bfd_per_xvec_warn (struct per_xvec_messages *messages, size_t alloc)
{
- size_t idx;
-
- if (!targ)
- return per_xvec_warn;
- for (idx = 0; idx < ARRAY_SIZE (_bfd_target_vector); idx++)
- if (_bfd_target_vector[idx] == targ)
- break;
- struct per_xvec_message **m = per_xvec_warn + idx;
- if (!alloc)
- return m;
+ const bfd_target *targ = messages->abfd->xvec;
+
+ struct per_xvec_messages *prev = NULL;
+ struct per_xvec_messages *iter = messages;
+
+ for (iter = messages; iter != NULL; iter = iter->next)
+ {
+ if (iter->targ == targ || iter->targ == NULL)
+ break;
+ prev = iter;
+ }
+
+ if (iter == NULL)
+ {
+ iter = bfd_malloc (sizeof (*iter));
+ if (iter == NULL)
+ return NULL;
+ iter->abfd = messages->abfd;
+ iter->targ = targ;
+ iter->messages = NULL;
+ iter->next = NULL;
+ prev->next = iter;
+ }
+ else if (iter->targ == NULL)
+ iter->targ = targ;
+
+ struct per_xvec_message **m = &iter->messages;
int count = 0;
while (*m)
{
@@ -1531,7 +1556,7 @@ _bfd_per_xvec_warn (const bfd_target *targ, size_t alloc)
if (*m != NULL)
(*m)->next = NULL;
}
- return m;
+ return *m;
}
/* Find a target vector, given a name or configuration triplet. */