From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 032A1385BAD0 for ; Wed, 29 Jun 2022 15:17:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 032A1385BAD0 Received: from [172.16.0.95] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B989B1E15C; Wed, 29 Jun 2022 11:17:01 -0400 (EDT) Message-ID: <87ea7153-6f53-06f9-3aa7-4ce90031b3e8@simark.ca> Date: Wed, 29 Jun 2022 11:17:01 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH v2] gdb+gdbserver/Linux: avoid reading registers while going through shell (was: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection) Content-Language: fr To: Pedro Alves , Andrew Burgess , gdb-patches@sourceware.org References: <83ceec06f74f4ce6a2dc8d794031fb233567ba6d.1655136816.git.aburgess@redhat.com> <48d211cc-96f3-8c84-5aec-7023054d45ac@palves.net> <87fsjqdqco.fsf@redhat.com> <6db0b4b8-25fb-8f4c-382e-d45f6edcff17@palves.net> <87a69xdqfo.fsf@redhat.com> From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE, WEIRD_PORT autolearn=ham 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.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Jun 2022 15:17:08 -0000 On 6/28/22 08:42, Pedro Alves wrote: > On 2022-06-28 11:37, Andrew Burgess wrote: >> Pedro Alves writes: >> >>> On 2022-06-27 17:27, Andrew Burgess wrote: >>>> Pedro Alves writes: >>> So I gave the patch below a try, and it seems to works well -- readcache_read_pc >>> is no longer called before target_find_description, and I saw no testsuite >>> regressions (this is without your series, just pristine master). >>> >>> Now, this patch as is makes sense for linux-nat.c, but then I went to do the >>> same thing for gdbserver, which has an equivalent save_stop_reason in linux-low.cc, >>> and there this approach won't work as is for the simple fact that lwp->stop_pc >>> is used in a lot more places. >>> >>> So to avoid gdb vs gdbserver divergence, it may be better to not take this patch >>> as is, but instead tweak save_stop_reason to avoid reading the stop pc while >>> the inferior is going through the shell, based on some per-inferior flag or some >>> such. Maybe we can repurpose progspace->executing_startup for this? That >>> flag used to be used by Cell (which is gone), but seems unused nowadays. There's >>> also other similar flags in inferior itself, like needs_setup, and >>> in_initial_library_scan. So we'd add a flag like one of those, set it while the >>> inferior is going through the startup_inferior dance, and make save_stop_reason avoid >>> reading the PC while that flag is set. > > ... > >> >> Thanks, this looks good. Maybe you should go ahead and merge this, and >> the attach problem can be solved separately. > > There's the gdb vs gdbserver divergence issue I pointed out above, though. > > I was thinking we'd instead fix it with the alternative patch below, which > works for both gdb and gdbserver. > > Tested with no regressions on x86-64 Ubuntu 20.04, native and gdbserver. > > WDYT? > > From 1b7b50e7c8359553af8b8858a82f09d5389d4a1b Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Mon, 27 Jun 2022 20:41:50 +0100 > Subject: [PATCH] gdb+gdbserver/Linux: avoid reading registers while going > through shell > > For every stop, linux-nat.c saves the stopped thread's PC, in > lwp->stop_pc. This is done in save_stop_reason, in both > gdb/linux-nat.c and gdbserver/linux-low.cc. However, while we're > going through the shell after "run", in startup_inferior, we shouldn't > be reading registers, as we haven't yet determined the target's > architecture -- the shell's architecture may not even be the same as > the final inferior's. > > In linux-nat.c, lwp->stop_pc is only needed when the thread has > stopped for a breakpoint, and since when going through the shell, no > breakpoint is going to hit, we could simply teach save_stop_reason to > only record the stop pc when the thread stopped for a breakpoint. > > However, in gdbserver/linux-low.cc, lwp->stop_pc is used in more cases > than breakpoint hits (e.g., it's used in tracepoints & the > "while-stepping" feature). > > So to avoid GDB vs GDBserver divergence, we apply the same approach to > both implementations. > > We set a flag in the inferior (process in gdbserver) whenever it is > being nursed through the shell, and when that flag is set, > save_stop_reason bails out early. While going through the shell, > we'll only ever get process exits (normal or signalled), and exec > events, so nothing is lost. > > Change-Id: If0f01831514d3a74d17efd102875de7d2c6401ad Hi, This introduces the following ASan crash for me: $ /home/smarchi/build/binutils-gdb/gdb/testsuite/../../gdb/../gdbserver/gdbserver stdio non-existing-program ================================================================= ==127719==ERROR: AddressSanitizer: heap-use-after-free on address 0x60f0000000e9 at pc 0x55bcbfa301f4 bp 0x7ffd238a7320 sp 0x7ffd238a7310 WRITE of size 1 at 0x60f0000000e9 thread T0 #0 0x55bcbfa301f3 in scoped_restore_tmpl::~scoped_restore_tmpl() /home/smarchi/src/binutils-gdb/gdbserver/../gdbsupport/scoped_restore.h:86 #1 0x55bcbfa2ffe9 in post_fork_inferior(int, char const*) /home/smarchi/src/binutils-gdb/gdbserver/fork-child.cc:120 #2 0x55bcbf9c9199 in linux_process_target::create_inferior(char const*, std::__debug::vector > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:991 #3 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941 #4 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084 #5 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2) #6 0x55bcbf8ef2bd in _start (/home/smarchi/build/binutils-gdb/gdbserver/gdbserver+0x1352bd) 0x60f0000000e9 is located 169 bytes inside of 176-byte region [0x60f000000040,0x60f0000000f0) freed by thread T0 here: #0 0x7ff9d6c6f0c7 in operator delete(void*) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:160 #1 0x55bcbf910d00 in remove_process(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/inferiors.cc:164 #2 0x55bcbf9c4ac7 in linux_process_target::remove_linux_process(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:454 #3 0x55bcbf9cdaa6 in linux_process_target::mourn(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:1599 #4 0x55bcbf988dc4 in target_mourn_inferior(ptid_t) /home/smarchi/src/binutils-gdb/gdbserver/target.cc:205 #5 0x55bcbfa32020 in startup_inferior(process_stratum_target*, int, int, target_waitstatus*, ptid_t*) /home/smarchi/src/binutils-gdb/gdbserver/../gdb/nat/fork-inferior.c:515 #6 0x55bcbfa2fdeb in post_fork_inferior(int, char const*) /home/smarchi/src/binutils-gdb/gdbserver/fork-child.cc:111 #7 0x55bcbf9c9199 in linux_process_target::create_inferior(char const*, std::__debug::vector > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:991 #8 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941 #9 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084 #10 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2) previously allocated by thread T0 here: #0 0x7ff9d6c6e5a7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99 #1 0x55bcbf910ad0 in add_process(int, int) /home/smarchi/src/binutils-gdb/gdbserver/inferiors.cc:144 #2 0x55bcbf9c477d in linux_process_target::add_linux_process_no_mem_file(int, int) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:425 #3 0x55bcbf9c8f4c in linux_process_target::create_inferior(char const*, std::__debug::vector > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:985 #4 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941 #5 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084 #6 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2) It looks like in that non-existing-program case, the process gets deleted before the starting_up flag gets restored to false. Simon