From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from omta34.uswest2.a.cloudfilter.net (omta34.uswest2.a.cloudfilter.net [35.89.44.33]) by sourceware.org (Postfix) with ESMTPS id EB9373858C42 for ; Sun, 24 Mar 2024 21:12:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EB9373858C42 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=tromey.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tromey.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EB9373858C42 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=35.89.44.33 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711314765; cv=none; b=heHnuTz92Zq12oIJpvEjiQwTrc7TcYYE/Yq3YqzimQ6gmsfYx+TH84uFheTlEbI+NuiGu3KEc7DWZgZxeiF/0US8RSDSf9NKnTucTjnF34WdnKYNoEpjg7q9cmOtLcRM3udlvGkw+AiavwRO5cfA0ioV3Ai7dcyiiaFLTKJbtKs= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711314765; c=relaxed/simple; bh=icanpZsRM89rquQvJY0j1mCh+0IQmMbLAS0dvCcM34k=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=olQ/QhHgc85E7peEeizCbw0pfbzCegmw5HjSV1QJceCg+ZCnPEyRX/0+CRs3dNelMRcR4FQ0+WP8DTvRCkvnNp9RFT7GmoRnuEY58yni1uaXWWYVOQeX1nRyKDgs7hXLdExtUU+U1xmwCP3qRzRNQ1aqAt3265f3HK42d0tN180= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from eig-obgw-5007a.ext.cloudfilter.net ([10.0.29.141]) by cmsmtp with ESMTPS id oFI3rLp2EHXmAoV8zrOnfM; Sun, 24 Mar 2024 21:12:41 +0000 Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTPS id oV8yr95xEeXgGoV8yreU0S; Sun, 24 Mar 2024 21:12:40 +0000 X-Authority-Analysis: v=2.4 cv=Q/PU4Z2a c=1 sm=1 tr=0 ts=66009748 a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=K6JAEmCyrfEA:10 a=Qbun_eYptAEA:10 a=CCpqsmhAAAAA:8 a=M3_qGfGMB3_OmUcFXy8A:9 a=3ZKOabzyN94A:10 a=ul9cdbp4aOFLsgKbc677:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Sender:Reply-To:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=HkW2PK8lqHnbfAzI3lwI60/8ovYRpzOny2rThCASJcs=; b=PxlNvrC+rRFNQYNy7/xJZpsLAJ gpPLzAE1ia8OQG8gwHKt9m1qKxgT1es3qTf9NG1XQk5Npkg+AvuLxHyieiZhhVSUdoVWZwwp6m1jP jy/lXWfXuzTxkNLyHZjO4Yu1V; Received: from 97-122-82-115.hlrn.qwest.net ([97.122.82.115]:33654 helo=localhost.localdomain) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96.2) (envelope-from ) id 1roV8x-001ww7-2w; Sun, 24 Mar 2024 15:12:39 -0600 From: Tom Tromey To: binutils@sourceware.org Cc: Tom Tromey Subject: [PATCH 1/2] Thread-safety improvements for bfd_check_format_matches Date: Sun, 24 Mar 2024 15:08:05 -0600 Message-ID: <20240324211229.1444550-2-tom@tromey.com> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240324211229.1444550-1-tom@tromey.com> References: <20240324211229.1444550-1-tom@tromey.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - box5379.bluehost.com X-AntiAbuse: Original Domain - sourceware.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - tromey.com X-BWhitelist: no X-Source-IP: 97.122.82.115 X-Source-L: No X-Exim-ID: 1roV8x-001ww7-2w X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 97-122-82-115.hlrn.qwest.net (localhost.localdomain) [97.122.82.115]:33654 X-Source-Auth: tom+tromey.com X-Email-Count: 2 X-Org: HG=bhshared;ORG=bluehost; X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfGPkpZeY/t9xkhnnSfE7u47wfA9EhQJgCxWF94ci1RyM78wVQLwGTvqN8q2qnUkM5x7pkhsEnjQVXLMV/5f3Q05MU3al1eKG29+MuCLoqP5meH+/it4E npmlPVrhhSQWFZ5hqWVthsnYjacIrNTTqgrTRDZlHMhIBL6BFwWO7/XXuGTRCOVkAVJilwWaBvyhntYObrTXR7L9WQPQLl27jE4= X-Spam-Status: No, score=-3021.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,GIT_PATCH_0,JMQ_SPF_NEUTRAL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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(-) diff --git a/bfd/bfd.c b/bfd/bfd.c index 54061a34240..11ad7f24456 100644 --- a/bfd/bfd.c +++ b/bfd/bfd.c @@ -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; } /* diff --git a/bfd/format.c b/bfd/format.c index 8f3fc7e7b96..5a5b01975ac 100644 --- a/bfd/format.c +++ b/bfd/format.c @@ -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; } diff --git a/bfd/libbfd.h b/bfd/libbfd.h index f15b5f27db8..04dbe720edb 100644 --- a/bfd/libbfd.h +++ b/bfd/libbfd.h @@ -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 } diff --git a/bfd/targets.c b/bfd/targets.c index f94992d031d..baf3cc5266c 100644 --- a/bfd/targets.c +++ b/bfd/targets.c @@ -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. */ -- 2.43.0