Message ID | 20240217-dwarf-race-relocate-v1-0-d3d2d908c1e8@tromey.com |
---|---|
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DE3AF386076C for <patchwork@sourceware.org>; Sun, 18 Feb 2024 01:11:20 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from omta040.useast.a.cloudfilter.net (omta040.useast.a.cloudfilter.net [44.202.169.39]) by sourceware.org (Postfix) with ESMTPS id D9082385E02B for <gdb-patches@sourceware.org>; Sun, 18 Feb 2024 01:10:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D9082385E02B 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 D9082385E02B Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=44.202.169.39 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708218611; cv=none; b=bXn6A456u+6K9/jWJUPHdWmv0T+BUmgNBvll95su/7aNAGvUaP8yei3oEKZJhTQqY4lSw9+eniyDbJixeP3RiQO8blmRZW24GdFFi4L8wxsxlXqZwwQms4c1fFKTFbsQXKf0I6JGV1WZ8s7yDExveDpULd/ADx5/2hKCWmBxhwY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708218611; c=relaxed/simple; bh=vZrz8nonXmkH97mX8ewsf3GVcBM+U9XsMK9n6YqNSn0=; h=DKIM-Signature:From:Subject:Date:Message-Id:MIME-Version:To; b=rqP8515HgF6T8fu5IYNkAdgmA/BnObQl1R8XgXt+D8JtkjdwcRU8Y8OH31flE5znClvsj6ZtgQ9lQOL9t6ekTdliCQxdeiJR6uuf+9gPQuOqWEo3Yp4Fv6jh6we+j7RDa4Zx7lvYZz83WbhFYLSxZ40KL2fybXeYsu5lCwqs+Gg= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from eig-obgw-6005a.ext.cloudfilter.net ([10.0.30.201]) by cmsmtp with ESMTPS id bKCdr6we2THHubVgzrgAfS; Sun, 18 Feb 2024 01:10:05 +0000 Received: from box5379.bluehost.com ([162.241.216.53]) by cmsmtp with ESMTPS id bVgyrqdfqtT7QbVgyr5yrl; Sun, 18 Feb 2024 01:10:04 +0000 X-Authority-Analysis: v=2.4 cv=M9/uKDws c=1 sm=1 tr=0 ts=65d158ec a=ApxJNpeYhEAb1aAlGBBbmA==:117 a=ApxJNpeYhEAb1aAlGBBbmA==:17 a=IkcTkHD0fZMA:10 a=k7vzHIieQBIA:10 a=Qbun_eYptAEA:10 a=zstS-IiYAAAA:8 a=A6QzgHIXcDPLvcqxvZQA:9 a=QEXdDO2ut3YA:10 a=4G6NA9xxw8l3yy4pmD5M:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tromey.com; s=default; h=To:Content-Transfer-Encoding:Content-Type:MIME-Version: Message-Id:Date:Subject:From:Sender:Reply-To:Cc:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=drNkEIC2EVmBVRmDm5Kj198YWIFobr2QzqVnr0zvy2Q=; b=qPZgU3hONxAoXS04uEfumQYQX9 Y6WawBgTWG9t73GnqdENzH5aaZEU8/ITnOofde8ARHXt8N0USm1zJnPumDBqQ29S896y87wNBrkmx 1+vUd8ghQ/vv2a3CDE613Ktwn; Received: from 71-211-170-195.hlrn.qwest.net ([71.211.170.195]:49770 helo=[192.168.0.21]) by box5379.bluehost.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96.2) (envelope-from <tom@tromey.com>) id 1rbVgy-003IPy-05 for gdb-patches@sourceware.org; Sat, 17 Feb 2024 18:10:04 -0700 From: Tom Tromey <tom@tromey.com> Subject: [PATCH 0/7] Fix race in DWARF reader Date: Sat, 17 Feb 2024 18:10:01 -0700 Message-Id: <20240217-dwarf-race-relocate-v1-0-d3d2d908c1e8@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-B4-Tracking: v=1; b=H4sIAOlY0WUC/x3MwQrCMAyA4VcZORvoqtDiq4iHLEu3wOwkGSqMv bvV43f4/x1cTMXh2u1g8lLXtTb0pw54pjoJ6tgMMcRLiH3C8U1W0IgFTZaVaROMOaUcypk4R2j l06To53+93ZsHcsHBqPL8ez3INzE4ji/zDy+tfgAAAA== To: gdb-patches@sourceware.org X-Mailer: b4 0.12.4 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: 71.211.170.195 X-Source-L: No X-Exim-ID: 1rbVgy-003IPy-05 X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: 71-211-170-195.hlrn.qwest.net ([192.168.0.21]) [71.211.170.195]:49770 X-Source-Auth: tom+tromey.com X-Email-Count: 1 X-Org: HG=bhshared;ORG=bluehost; X-Source-Cap: ZWx5bnJvYmk7ZWx5bnJvYmk7Ym94NTM3OS5ibHVlaG9zdC5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfOw5xYmCUZy6YMy2tGaAJsVFYWuVV9ii3SE/pbOYKRh4ZChsRZQezGvZ1/F+NT+iQZZK3RPM5dnPKl/JmxDQSF8t6PzRAj5UDjekhgf8Oz+HV5oD1fAV w4PvI7uxj/kkF8GSWrbgfS/RWy7yO7a2QhqAorT3nwm8VvZoF6S6+nRD72orv3vFvvBuVlXvjTpx6tQMT51R1Wt5QKLkMXhIxJ8= X-Spam-Status: No, score=-3016.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, JMQ_SPF_NEUTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org |
Series |
Fix race in DWARF reader
|
|
Message
Tom Tromey
Feb. 18, 2024, 1:10 a.m. UTC
The background DWARF reader turns out to have a few races. This series fixes one that occurs when the indexer runs at the same time as DWARF relocation. Most of the series is just cleanup / preparation. The main patch affects MIPS16. I can't test this -- I tried on a MIPS machine in the GCC compile farm, but unfortunately the relevant gdb.arch test says that the processor doesn't support MIPS16. It's possible this code is simply dead; I do not know. Regression tested on x86-64 Fedora 38. --- Tom Tromey (7): Compare section index in lookup_minimal_symbol_by_pc_section Remove unnecessary null check in lookup_minimal_symbol_by_pc_section Hoist a call to frob_address Add unrelocated overload of lookup_minimal_symbol_by_pc_section Fix race in background DWARF indexer Use bound_minimal_symbol more in lookup_minimal_symbol_by_pc_section Fix address comparison in lookup_minimal_symbol_by_pc_section gdb/arch-utils.c | 5 +- gdb/arch-utils.h | 3 +- gdb/dwarf2/frame.c | 13 +- gdb/dwarf2/read.c | 12 +- gdb/gdbarch-gen.h | 4 +- gdb/gdbarch.c | 6 +- gdb/gdbarch_components.py | 4 +- gdb/minsyms.c | 449 +++++++++++++++++++++++++--------------------- gdb/minsyms.h | 7 + gdb/mips-tdep.c | 49 +++++ gdb/mips-tdep.h | 2 + 11 files changed, 330 insertions(+), 224 deletions(-) --- base-commit: 989aa9b8e8e340ba65f386cbfd239009a3aba68f change-id: 20240217-dwarf-race-relocate-287780f3ac82 Best regards,
Comments
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:
Tom> The background DWARF reader turns out to have a few races. This
Tom> series fixes one that occurs when the indexer runs at the same time as
Tom> DWARF relocation.
Tom> Most of the series is just cleanup / preparation.
Tom> The main patch affects MIPS16. I can't test this -- I tried on a MIPS
Tom> machine in the GCC compile farm, but unfortunately the relevant
Tom> gdb.arch test says that the processor doesn't support MIPS16. It's
Tom> possible this code is simply dead; I do not know.
Elsewhere I mentioned that I had a different idea for this series.
It seems to me that most (or maybe even all) the calls to
dwarf2_per_objfile::adjust aren't really needed. Many of them only
affect lookup tables, where the adjustment isn't needed. This includes
all calls made by the indexer.
Some of the calls (like the one in read_attribute_value) even seem to be
wrong.
So, I wrote a short series to remove these. Unfortunately, though, it's
hard to know for sure if the result is correct, given that I don't know
how to test MIPS16.
I could probably test some simple things ("break") by debugging gdb
while examining (but not running) a MIPS16 program. I'm not sure if
that's really sufficient though.
I'd appreciate some insight if you have any.
thanks,
Tom
On 4/3/24 11:16 AM, Tom Tromey wrote: >>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes: > > Tom> The background DWARF reader turns out to have a few races. This > Tom> series fixes one that occurs when the indexer runs at the same time as > Tom> DWARF relocation. > > Tom> Most of the series is just cleanup / preparation. > > Tom> The main patch affects MIPS16. I can't test this -- I tried on a MIPS > Tom> machine in the GCC compile farm, but unfortunately the relevant > Tom> gdb.arch test says that the processor doesn't support MIPS16. It's > Tom> possible this code is simply dead; I do not know. > > Elsewhere I mentioned that I had a different idea for this series. > > It seems to me that most (or maybe even all) the calls to > dwarf2_per_objfile::adjust aren't really needed. Many of them only > affect lookup tables, where the adjustment isn't needed. This includes > all calls made by the indexer. > > Some of the calls (like the one in read_attribute_value) even seem to be > wrong. > > So, I wrote a short series to remove these. Unfortunately, though, it's > hard to know for sure if the result is correct, given that I don't know > how to test MIPS16. > > I could probably test some simple things ("break") by debugging gdb > while examining (but not running) a MIPS16 program. I'm not sure if > that's really sufficient though. > > I'd appreciate some insight if you have any. I haven't seen anyone active with submitting MIPS patches in several years. I no longer make use of MIPS myself (and we've removed it from FreeBSD entirely, though I know it's still present in Linux distros). Even when I was working with MIPS I never tested microMIPS / MIPS16. OTOH, I think MIPS16 is similar to Thumb on ARM, and it might even be using a similar trick from reading your series (setting the LSB to enter "compressed decoding mode" vs "regular decoding mode"). I think ARM uses special mapping symbols to mark Thumb vs non-Thumb code though instead of depending on the LSB? That is, I wonder why Thumb doesn't trip over this issue the way MIPS16 does?
On 4/9/24 19:16, John Baldwin wrote: > On 4/3/24 11:16 AM, Tom Tromey wrote: >>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes: >> >> Tom> The background DWARF reader turns out to have a few races. This >> Tom> series fixes one that occurs when the indexer runs at the same time as >> Tom> DWARF relocation. >> >> Tom> Most of the series is just cleanup / preparation. >> >> Tom> The main patch affects MIPS16. I can't test this -- I tried on a MIPS >> Tom> machine in the GCC compile farm, but unfortunately the relevant >> Tom> gdb.arch test says that the processor doesn't support MIPS16. It's >> Tom> possible this code is simply dead; I do not know. >> >> Elsewhere I mentioned that I had a different idea for this series. >> >> It seems to me that most (or maybe even all) the calls to >> dwarf2_per_objfile::adjust aren't really needed. Many of them only >> affect lookup tables, where the adjustment isn't needed. This includes >> all calls made by the indexer. >> >> Some of the calls (like the one in read_attribute_value) even seem to be >> wrong. >> >> So, I wrote a short series to remove these. Unfortunately, though, it's >> hard to know for sure if the result is correct, given that I don't know >> how to test MIPS16. >> >> I could probably test some simple things ("break") by debugging gdb >> while examining (but not running) a MIPS16 program. I'm not sure if >> that's really sufficient though. >> >> I'd appreciate some insight if you have any. > > I haven't seen anyone active with submitting MIPS patches in several years. > I no longer make use of MIPS myself (and we've removed it from FreeBSD > entirely, though I know it's still present in Linux distros). Even when I > was working with MIPS I never tested microMIPS / MIPS16. > > OTOH, I think MIPS16 is similar to Thumb on ARM, and it might even be > using a similar trick from reading your series (setting the LSB to enter > "compressed decoding mode" vs "regular decoding mode"). I think ARM uses > special mapping symbols to mark Thumb vs non-Thumb code though instead of > depending on the LSB? That is, I wonder why Thumb doesn't trip over this > issue the way MIPS16 does? > cc-ing Maciej, who might have a better idea on MIPS bits.
On Tue, 9 Apr 2024, Luis Machado wrote: > >> Tom> The main patch affects MIPS16. I can't test this -- I tried on a MIPS > >> Tom> machine in the GCC compile farm, but unfortunately the relevant > >> Tom> gdb.arch test says that the processor doesn't support MIPS16. It's > >> Tom> possible this code is simply dead; I do not know. > >> > >> Elsewhere I mentioned that I had a different idea for this series. > >> > >> It seems to me that most (or maybe even all) the calls to > >> dwarf2_per_objfile::adjust aren't really needed. Many of them only > >> affect lookup tables, where the adjustment isn't needed. This includes > >> all calls made by the indexer. > >> > >> Some of the calls (like the one in read_attribute_value) even seem to be > >> wrong. > >> > >> So, I wrote a short series to remove these. Unfortunately, though, it's > >> hard to know for sure if the result is correct, given that I don't know > >> how to test MIPS16. > >> > >> I could probably test some simple things ("break") by debugging gdb > >> while examining (but not running) a MIPS16 program. I'm not sure if > >> that's really sufficient though. > >> > >> I'd appreciate some insight if you have any. > > > > I haven't seen anyone active with submitting MIPS patches in several years. > > I no longer make use of MIPS myself (and we've removed it from FreeBSD > > entirely, though I know it's still present in Linux distros). Even when I > > was working with MIPS I never tested microMIPS / MIPS16. Support for the MIPS target in Linux is certainly far from being dead and I believe new MIPS hardware continues being made. Also a substantial MIPS patch for GDB for R6 ISA support is being pinged for review right now. > > OTOH, I think MIPS16 is similar to Thumb on ARM, and it might even be > > using a similar trick from reading your series (setting the LSB to enter > > "compressed decoding mode" vs "regular decoding mode"). I think ARM uses > > special mapping symbols to mark Thumb vs non-Thumb code though instead of > > depending on the LSB? That is, I wonder why Thumb doesn't trip over this > > issue the way MIPS16 does? > > > > cc-ing Maciej, who might have a better idea on MIPS bits. Thanks, Luis! I can certainly run MIPS16 GDB verification right away with actual hardware: macro@malta(1)~$ uname -a Linux malta 5.18.0-rc2-00254-gfb649bda6f56-dirty #2 Sat Nov 12 20:14:53 GMT 2022 mips unknown unknown GNU/Linux macro@malta(2)~$ grep mips16 /proc/cpuinfo ASEs implemented : mips16 dsp dsp2 macro@malta(3)~$ however to understand the impact I'd have to go through the code changes, which I can't guarantee any specific timeframe for. Indeed at the machine level it is the LSB of the PC that tells compressed and regular code apart: you just flip the bit as required either by using special instructions with direct calls/jumps or explicitly with indirect ones; it's also correctly set according to the execution mode in effect in PC values recorded by hardware, such as the return address for function calls or the exception PC for kernel traps. Then whether compressed code uses the MIPS16 instruction encoding or the microMIPS instruction encoding it is the property of the implementation. Offhand I recall for compressed functions DWARF line information has the LSB of the PC set according to compressed vs regular encoding, however other DWARF records or the static ELF symbol table do not. Compressed function symbols have appropriate flags set in `st_other' to tell them apart from regular function symbols. BFD uses that information to set the LSB appropriately in relocation processing where applicable. To call a compressed function by hand or for function pointer comparison (e.g. against a datum stored in a program's variable or in a CPU register) in expression evaluation GDB has to recreate the LSB from information available and apply it to symbol values obtained from the symbol table, and it's a bit messy due to how things happened in the past. Conversely, in certain contexts the LSB has to be removed instead, such as `x /i $pc'. I made all this at least work at one point, not without shortcomings (e.g. broken hex instruction dumps in `disassemble /r' output), which is what we have now. Later on Yao Qi came up with a better proposal building on a generalised property of some psABIs where a function pointer is not the function's address: <https://sourceware.org/ml/gdb-patches/2016-10/msg00430.html>. The proposal got stuck on an issue with the PPC64 target which got never resolved due to the shortage of time and higher priority tasks combined: <https://sourceware.org/ml/gdb-patches/2017-10/msg00096.html>. Maybe someone can pick it up from there? I could do the necessary MIPS bits then myself, I certainly find it important enough to preempt other stuff I might otherwise want doing instead. Maciej
>>>>> "Maciej" == Maciej W Rozycki <macro@orcam.me.uk> writes:
Maciej> Support for the MIPS target in Linux is certainly far from being dead and
Maciej> I believe new MIPS hardware continues being made. Also a substantial MIPS
Maciej> patch for GDB for R6 ISA support is being pinged for review right now.
Thanks. In terms of dead-ness I was really referring just to MIPS16.
Anyway, I'm sending a different & much simpler variant of this fix.
Any testing you could provide on that would be appreciated.
Tom