From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) by sourceware.org (Postfix) with ESMTPS id 578083858C20 for ; Thu, 10 Mar 2022 12:32:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 578083858C20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wm1-f45.google.com with SMTP id l10so3177525wmb.0 for ; Thu, 10 Mar 2022 04:32:58 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:from:to:cc:references:in-reply-to :content-transfer-encoding; bh=nyL2u24jI6TpGhhI2R+G5uxLm8cuXq7mIT+5kXmG8Ds=; b=k6QhG/2EhLgtgbdaRlBcVNUFFCRyv5DhsTuN0R9RRk+leXFHihciSBCVuSSoIiE+vC tQpkjKdAkn9IQWBzedWeo2iuhKWTWHA7sGSxa8iQFhR5ilSprbKP+tL0NUzzQKvUVo4g zOebEHMKDk0S2eUuOwpLP+86koo1iECHgaalOzASaK41cEF635uozTnXwj89kaja+PD0 IRTHyY3E0NctFM+wCdZ5qsGUSzWyUwD+30XKOGAU0TMj1dBN0Z6ZFNJK+c7/dNtt6Hol R1qy2juZMUKjTFvk7ACXOMH+Ie+9QAHNhBMnhdQWaBoEGXW+OLmXnp86BSZ0UmFUvlaG s7Lg== X-Gm-Message-State: AOAM531sP2pgeX4yTriqY8FmCaBThUant6OJ/JboTK61hkuP9xXSd41U 9zB41f10jjHkIdf+3u+v62g= X-Google-Smtp-Source: ABdhPJwm30baydJsr/Cwz+aoYlZG9lu2s+4vlC1Rwk++nqdi95iGJ1HSNwmv6Wpeqjxn/ZVjJ9HRSg== X-Received: by 2002:a1c:2b41:0:b0:380:e379:b8b0 with SMTP id r62-20020a1c2b41000000b00380e379b8b0mr3421049wmr.87.1646915577228; Thu, 10 Mar 2022 04:32:57 -0800 (PST) Received: from ?IPV6:2001:8a0:f924:2600:209d:85e2:409e:8726? ([2001:8a0:f924:2600:209d:85e2:409e:8726]) by smtp.gmail.com with ESMTPSA id l18-20020adfe592000000b001f064ae9830sm4035765wrm.37.2022.03.10.04.32.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 10 Mar 2022 04:32:55 -0800 (PST) Message-ID: <7dc4402f-374e-482a-6612-8f4a527cb711@palves.net> Date: Thu, 10 Mar 2022 12:32:54 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.1 Subject: Re: [EXT] Re: Semihosting in GDB 11.1 | Proposed patch for interrupting in sync mode Content-Language: en-US From: Pedro Alves To: Adrian Oltean , Tom Tromey Cc: Adrian Oltean via Gdb References: <87k0d2gb44.fsf@tromey.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.8 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Mar 2022 12:33:00 -0000 On 2022-03-10 11:30, Pedro Alves wrote: > > HOWEVER. Note that the File I/O packets, like "Fwrite,1,80026300,12" don't say > at all which thread is doing the File I/O access! IOW, this packet is not useable > in multi-process scenarios as is... It would need to be extended to also pass down > an extra thread id field, like "Fwrite,1,80026300,12,pPID.TID". FSF GDBserver doesn't > support File I/O, so these File I/O packets didn't get attention when the multi-process > extensions were devised. > > So alright, the current "Hg0" packet you see in your logs is a bit spurious, but, it should > be harmless, and you should be able to select any valid thread, as per the Hg0 packet's > description, as all threads share the same address space, in GDB's view. > > [remote] Packet received: Fwrite,1,80026300,12 > [remote] Sending packet: $Hg0#df > [remote] Received Ack > [remote] Packet received: OK > [remote] Sending packet: $m80026300,12#8f > [remote] Received Ack > [remote] Packet received: 48656c6c6f2066726f6d20436f726523370a > [remote] Sending packet: $F12#a9 > > What I think you should do is, when you get the reply to Fwrite -- the "$F12#a9" > packet -- your stub should make sure that that resumes the same thread that initiated > the Fwrite, not whatever Hg selected. That $F12 reply is linked with the last Fwrite, > there can only be one File I/O request ongoing at a given time. Thinking a bit more about this, I guess GDB could just avoid changing the selected thread when handling a File I/O request. That seems quite easy to do, so I gave it a try. However, a realized that that runs into a difficulty. With multi-process, if the server doesn't say which process the request is for, GDB's target_read_memory/target_write_memory can end up reading/writing from the wrong dcache. A workaround is to flush the dcache around memory accesses that come from File I/O. Not great, but at least it turns a correctness issue into an optimization issue... I don't have a target that does File I/O, so I can't really test this. >From c3093d0fa3ff527b542a141e93c0a8a580e7cce8 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 10 Mar 2022 11:59:52 +0000 Subject: [PATCH] File I/O - avoid changing the server's current thread Change-Id: I3a69e36988cb1fba746be5a4d8dd65002c8b7711 --- gdb/remote-fileio.c | 52 +++++++++++++++++++++++++++++++++++---------- gdb/remote.c | 23 ++++++++++++++++---- gdb/target-dcache.c | 23 ++++++++++++++++---- gdb/target-dcache.h | 3 +++ 4 files changed, 82 insertions(+), 19 deletions(-) diff --git a/gdb/remote-fileio.c b/gdb/remote-fileio.c index 9963f1ebc01..3fbf5ec4090 100644 --- a/gdb/remote-fileio.c +++ b/gdb/remote-fileio.c @@ -29,6 +29,7 @@ #include "target.h" #include "filenames.h" #include "gdbsupport/filestuff.h" +#include "target-dcache.h" #include #include "gdbsupport/gdb_sys_time.h" @@ -37,6 +38,35 @@ #endif #include +/* Wrappers around target_read_memory/target_write_memory that + invalidate the dcache on entry and exit. This is needed because + File I/O packets don't tell us which process initiated the request, + so we don't know whether the cache for the current process is the + right one. */ + +static int +fileio_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len) +{ + target_dcache_invalidate (); + SCOPE_EXIT { target_dcache_invalidate (); }; + + return target_read_memory (memaddr, myaddr, len); +} + +static int +fileio_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len) +{ + target_dcache_invalidate (); + SCOPE_EXIT + { + /* After writing, the cache of some other inferior may be stale, + so we need to invalidate all. */ + target_dcache_invalidate_all (); + }; + + return target_write_memory (memaddr, myaddr, len); +} + static struct { int *fd_map; int fd_map_size; @@ -401,7 +431,7 @@ remote_fileio_func_open (remote_target *remote, char *buf) /* Request pathname. */ pathname = (char *) alloca (length); - if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0) + if (fileio_read_memory (ptrval, (gdb_byte *) pathname, length) != 0) { remote_fileio_ioerror (remote); return; @@ -574,7 +604,7 @@ remote_fileio_func_read (remote_target *remote, char *buf) if (ret > 0) { - errno = target_write_memory (ptrval, buffer, ret); + errno = fileio_write_memory (ptrval, buffer, ret); if (errno != 0) ret = -1; } @@ -625,7 +655,7 @@ remote_fileio_func_write (remote_target *remote, char *buf) length = (size_t) num; buffer = (gdb_byte *) xmalloc (length); - if (target_read_memory (ptrval, buffer, length) != 0) + if (fileio_read_memory (ptrval, buffer, length) != 0) { xfree (buffer); remote_fileio_ioerror (remote); @@ -740,7 +770,7 @@ remote_fileio_func_rename (remote_target *remote, char *buf) /* Request oldpath using 'm' packet */ oldpath = (char *) alloca (old_len); - if (target_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0) + if (fileio_read_memory (old_ptr, (gdb_byte *) oldpath, old_len) != 0) { remote_fileio_ioerror (remote); return; @@ -748,7 +778,7 @@ remote_fileio_func_rename (remote_target *remote, char *buf) /* Request newpath using 'm' packet */ newpath = (char *) alloca (new_len); - if (target_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0) + if (fileio_read_memory (new_ptr, (gdb_byte *) newpath, new_len) != 0) { remote_fileio_ioerror (remote); return; @@ -825,7 +855,7 @@ remote_fileio_func_unlink (remote_target *remote, char *buf) } /* Request pathname using 'm' packet */ pathname = (char *) alloca (length); - if (target_read_memory (ptrval, (gdb_byte *) pathname, length) != 0) + if (fileio_read_memory (ptrval, (gdb_byte *) pathname, length) != 0) { remote_fileio_ioerror (remote); return; @@ -874,7 +904,7 @@ remote_fileio_func_stat (remote_target *remote, char *buf) /* Request pathname using 'm' packet */ pathname = (char *) alloca (namelength); - if (target_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0) + if (fileio_read_memory (nameptr, (gdb_byte *) pathname, namelength) != 0) { remote_fileio_ioerror (remote); return; @@ -898,7 +928,7 @@ remote_fileio_func_stat (remote_target *remote, char *buf) host_to_fileio_stat (&st, &fst); host_to_fileio_uint (0, fst.fst_dev); - errno = target_write_memory (statptr, (gdb_byte *) &fst, sizeof fst); + errno = fileio_write_memory (statptr, (gdb_byte *) &fst, sizeof fst); if (errno != 0) { remote_fileio_return_errno (remote, -1); @@ -975,7 +1005,7 @@ remote_fileio_func_fstat (remote_target *remote, char *buf) { host_to_fileio_stat (&st, &fst); - errno = target_write_memory (ptrval, (gdb_byte *) &fst, sizeof fst); + errno = fileio_write_memory (ptrval, (gdb_byte *) &fst, sizeof fst); if (errno != 0) { remote_fileio_return_errno (remote, -1); @@ -1026,7 +1056,7 @@ remote_fileio_func_gettimeofday (remote_target *remote, char *buf) { remote_fileio_to_fio_timeval (&tv, &ftv); - errno = target_write_memory (ptrval, (gdb_byte *) &ftv, sizeof ftv); + errno = fileio_write_memory (ptrval, (gdb_byte *) &ftv, sizeof ftv); if (errno != 0) { remote_fileio_return_errno (remote, -1); @@ -1071,7 +1101,7 @@ remote_fileio_func_system (remote_target *remote, char *buf) { /* Request commandline using 'm' packet */ cmdline = (char *) alloca (length); - if (target_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0) + if (fileio_read_memory (ptrval, (gdb_byte *) cmdline, length) != 0) { remote_fileio_ioerror (remote); return; diff --git a/gdb/remote.c b/gdb/remote.c index aa6a67a96e0..e9230060927 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -274,6 +274,13 @@ class remote_state because we allow GDB commands while the target is running. */ bool waiting_for_stop_reply = false; + /* True if handling a File I/O request. When this is true, we make + sure to skip telling the server to change its selected thread to + match the GDB-selected thread, which has no relation to the + thread that initiated the File I/O request. Note the File I/O + packet doesn't tell us which thread or process that was. */ + bool handling_fileio_request = false; + /* The status of the stub support for the various vCont actions. */ vCont_action_support supports_vCont; /* Whether vCont support was probed already. This is a workaround @@ -8232,11 +8239,13 @@ remote_target::wait_as (ptid_t ptid, target_waitstatus *status, for a stop reply. See the comments in putpkt_binary. Set waiting_for_stop_reply to 0 temporarily. */ rs->waiting_for_stop_reply = 0; + rs->handling_fileio_request = true; remote_fileio_request (this, buf, rs->ctrlc_pending_p); rs->ctrlc_pending_p = 0; /* GDB handled the File-I/O request, and the target is running again. Keep waiting for events. */ rs->waiting_for_stop_reply = 1; + rs->handling_fileio_request = false; break; case 'N': case 'T': case 'S': case 'X': case 'W': { @@ -11208,16 +11217,22 @@ remote_target::xfer_partial (enum target_object object, const gdb_byte *writebuf, ULONGEST offset, ULONGEST len, ULONGEST *xfered_len) { - struct remote_state *rs; int i; char *p2; char query_type; int unit_size = gdbarch_addressable_memory_unit_size (target_gdbarch ()); - set_remote_traceframe (); - set_general_thread (inferior_ptid); + remote_state *rs = get_remote_state (); - rs = get_remote_state (); + /* If we're handling a File I/O read/write request, we'll want to + access memory off of the thread that initiated the File I/O + request, not from whatever thread GDB had selected. We don't + know what thread that is, but we don't need to know. */ + if (!rs->handling_fileio_request) + { + set_remote_traceframe (); + set_general_thread (inferior_ptid); + } /* Handle memory using the standard memory routines. */ if (object == TARGET_OBJECT_MEMORY) diff --git a/gdb/target-dcache.c b/gdb/target-dcache.c index 96d4611ee02..0e10e42218f 100644 --- a/gdb/target-dcache.c +++ b/gdb/target-dcache.c @@ -38,18 +38,33 @@ target_dcache_init_p (void) return (dcache != NULL); } -/* Invalidate the target dcache. */ +/* Invalidate the target dcache of PSPACE. */ -void -target_dcache_invalidate (void) +static void +target_dcache_invalidate_pspace (program_space *pspace) { DCACHE *dcache - = target_dcache_aspace_key.get (current_program_space->aspace); + = target_dcache_aspace_key.get (pspace->aspace); if (dcache != NULL) dcache_invalidate (dcache); } +/* Invalidate the target dcache. */ + +void +target_dcache_invalidate (void) +{ + target_dcache_invalidate_pspace (current_program_space); +} + +void +target_dcache_invalidate_all () +{ + for (program_space *pspace : program_spaces) + target_dcache_invalidate_pspace (pspace); +} + /* Return the target dcache. Return NULL if target dcache is not initialized yet. */ diff --git a/gdb/target-dcache.h b/gdb/target-dcache.h index 6f31a3d4482..2def8a2ee30 100644 --- a/gdb/target-dcache.h +++ b/gdb/target-dcache.h @@ -22,6 +22,9 @@ extern void target_dcache_invalidate (void); +/* Invalidate the dcache of all program spaces. */ +extern void target_dcache_invalidate_all (); + extern DCACHE *target_dcache_get (void); extern DCACHE *target_dcache_get_or_init (void); base-commit: c6479f8b2ab32c8a1659054104f2d0176a466cb3 -- 2.26.2