From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 96324 invoked by alias); 3 Oct 2017 14:02:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 96313 invoked by uid 89); 3 Oct 2017 14:02:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=exporting, beneath X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Oct 2017 14:02:19 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 372082CE908; Tue, 3 Oct 2017 14:02:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 372082CE908 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3C3336268A; Tue, 3 Oct 2017 14:02:17 +0000 (UTC) Subject: Re: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors To: Yao Qi References: <1506957311-30028-1-git-send-email-palves@redhat.com> <1506957311-30028-4-git-send-email-palves@redhat.com> <867ewc793g.fsf@gmail.com> <2370cfd3-d54a-9b86-fde4-0bdd8c10e5b3@redhat.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Tue, 03 Oct 2017 14:02:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <2370cfd3-d54a-9b86-fde4-0bdd8c10e5b3@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-10/txt/msg00041.txt.bz2 On 10/03/2017 01:21 PM, Pedro Alves wrote: > > On 10/03/2017 12:40 PM, Yao Qi wrote: >> Pedro Alves writes: >> >>> struct gdbarch * >>> default_thread_architecture (struct target_ops *ops, ptid_t ptid) >>> { >>> - return target_gdbarch (); >>> + inferior *inf = find_inferior_ptid (ptid); >>> + gdb_assert (inf != NULL); >>> + return inf->gdbarch; >>> } >> >> It is right, but forgot to mention that we need to update >> spu_thread_architecture too, >> >> if (parse_spufs_run (ptid, &spufs_fd, &spufs_addr)) >> return spu_gdbarch (spufs_fd); >> >> return target_gdbarch (); >> >> it looks wrong to call target_gdbarch. We may need to replace >> target_gdbarch with default_thread_architecture. > > I think you're right. > > The target_gdbarch reference in process_stop_reply looks incorrect > as well (last hunk below). I'm current running test with the patch below > on top. Kind of curious that I didn't run into this one before. > Here's the updated patch. Instead of exporting default_thread_architecture, I'm deferring to the target beneath. In practice ends up being the same thing I think, but seems a bit more correct and future proof, and is also simpler. I've added the comment about target descriptions. WDYT? >From 8bcea3ea32a55a3cfac29dde1c97083843047e45 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 3 Oct 2017 13:26:42 +0100 Subject: [PATCH 3/3] Fix "Remote 'g' packet reply is too long" problems with multiple inferiors When debugging two inferiors (or more) against gdbserver, and the inferiors have different architectures, such as e.g., on x86_64 GNU/Linux and one inferior is 64-bit while the other is 32-bit, then GDB can get confused with the different architectures in a couple spots. In both cases I ran into, GDB incorrectly ended up using the architecture of whatever happens to be the selected inferior instead of the architecture of some other given inferior: #1 - When parsing the expedited registers in stop replies. #2 - In the default implementation of the target_thread_architecture target method. These resulted in instances of the infamous "Remote 'g' packet reply is too long" error. For example, with the test added in this commit, we get: ~~~ Continuing. Remote 'g' packet reply is too long (expected 440 bytes, got 816 bytes): ad064000000000000[snip] (gdb) FAIL: gdb.multi/multi-arch.exp: inf1 event with inf2 selected: continue to hello_loop c Continuing. Truncated register 50 in remote 'g' packet (gdb) PASS: gdb.multi/multi-arch.exp: inf2 event with inf1 selected: c ~~~ This commit fixes that. gdb/ChangeLog: yyyy-mm-dd Pedro Alves * remote.c (get_remote_arch_state): New 'gdbarch' parameter. Use it instead of target_gdbarch. (get_remote_state, get_remote_packet_size): Adjust get_remote_arch_state calls, passing down target_gdbarch explicitly. (packet_reg_from_regnum, packet_reg_from_pnum): New parameter 'gdbarch' and use it instead of target_gdbarch. (get_memory_packet_size): Adjust get_remote_arch_state calls, passing down target_gdbarch explicitly. (struct stop_reply) : New field. (remote_parse_stop_reply): Use the stopped thread's architecture, not the current inferior's. Save the architecture in the stop_reply. (process_stop_reply): Use the stop reply's architecture. (process_g_packet, remote_fetch_registers) (remote_prepare_to_store, store_registers_using_G) (remote_store_registers): Adjust get_remote_arch_state calls, using the regcache's architecture. (remote_get_trace_status): Adjust get_remote_arch_state calls, passing down target_gdbarch explicitly. * spu-multiarch.c (spu_thread_architecture): Defer to the target beneath instead of calling target_gdbarch. * target.c (default_thread_architecture): Use the specified inferior's architecture, instead of the current inferior's architecture (via target_gdbarch). gdb/testsuite/ChangeLog: yyyy-mm-dd Pedro Alves * gdb.multi/hangout.c: Include . (hangout_loop): New function. (main): Call alarm. Call hangout_loop in a loop. * gdb.multi/hello.c: Include . (hello_loop): New function. (main): Call alarm. Call hangout_loop in a loop. * gdb.multi/multi-arch.exp: Test running to a breakpoint one inferior with the other selected. --- gdb/remote.c | 100 ++++++++++++++++++++++++--------- gdb/spu-multiarch.c | 3 +- gdb/target.c | 4 +- gdb/testsuite/gdb.multi/hangout.c | 14 +++++ gdb/testsuite/gdb.multi/hello.c | 15 ++++- gdb/testsuite/gdb.multi/multi-arch.exp | 24 ++++++++ 6 files changed, 130 insertions(+), 30 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index 54d810f..9bc69c2 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -654,11 +654,11 @@ remote_get_noisy_reply () static struct gdbarch_data *remote_gdbarch_data_handle; static struct remote_arch_state * -get_remote_arch_state (void) +get_remote_arch_state (struct gdbarch *gdbarch) { - gdb_assert (target_gdbarch () != NULL); + gdb_assert (gdbarch != NULL); return ((struct remote_arch_state *) - gdbarch_data (target_gdbarch (), remote_gdbarch_data_handle)); + gdbarch_data (gdbarch, remote_gdbarch_data_handle)); } /* Fetch the global remote target state. */ @@ -671,7 +671,7 @@ get_remote_state (void) function which calls getpkt also needs to be mindful of changes to rs->buf, but this call limits the number of places which run into trouble. */ - get_remote_arch_state (); + get_remote_arch_state (target_gdbarch ()); return get_remote_state_raw (); } @@ -878,7 +878,7 @@ static long get_remote_packet_size (void) { struct remote_state *rs = get_remote_state (); - struct remote_arch_state *rsa = get_remote_arch_state (); + remote_arch_state *rsa = get_remote_arch_state (target_gdbarch ()); if (rs->explicit_packet_size) return rs->explicit_packet_size; @@ -887,9 +887,10 @@ get_remote_packet_size (void) } static struct packet_reg * -packet_reg_from_regnum (struct remote_arch_state *rsa, long regnum) +packet_reg_from_regnum (struct gdbarch *gdbarch, struct remote_arch_state *rsa, + long regnum) { - if (regnum < 0 && regnum >= gdbarch_num_regs (target_gdbarch ())) + if (regnum < 0 && regnum >= gdbarch_num_regs (gdbarch)) return NULL; else { @@ -901,11 +902,12 @@ packet_reg_from_regnum (struct remote_arch_state *rsa, long regnum) } static struct packet_reg * -packet_reg_from_pnum (struct remote_arch_state *rsa, LONGEST pnum) +packet_reg_from_pnum (struct gdbarch *gdbarch, struct remote_arch_state *rsa, + LONGEST pnum) { int i; - for (i = 0; i < gdbarch_num_regs (target_gdbarch ()); i++) + for (i = 0; i < gdbarch_num_regs (gdbarch); i++) { struct packet_reg *r = &rsa->regs[i]; @@ -1049,7 +1051,7 @@ static long get_memory_packet_size (struct memory_packet_config *config) { struct remote_state *rs = get_remote_state (); - struct remote_arch_state *rsa = get_remote_arch_state (); + remote_arch_state *rsa = get_remote_arch_state (target_gdbarch ()); long what_they_get; if (config->fixed_p) @@ -6361,6 +6363,9 @@ typedef struct stop_reply struct target_waitstatus ws; + /* The architecture associated with the expedited registers. */ + gdbarch *arch; + /* Expedited registers. This makes remote debugging a bit more efficient for those targets that provide critical registers as part of their normal status mechanism (as another roundtrip to @@ -6835,7 +6840,7 @@ strprefix (const char *p, const char *pend, const char *prefix) static void remote_parse_stop_reply (char *buf, struct stop_reply *event) { - struct remote_arch_state *rsa = get_remote_arch_state (); + remote_arch_state *rsa = NULL; ULONGEST addr; const char *p; int skipregs = 0; @@ -7015,9 +7020,47 @@ Packet: '%s'\n"), reason. */ if (p_temp == p1) { - struct packet_reg *reg = packet_reg_from_pnum (rsa, pnum); + /* If we haven't parsed the event's thread yet, find + it now, in order to find the architecture of the + reported expedited registers. */ + if (event->ptid == null_ptid) + { + const char *thr = strstr (p1 + 1, ";thread:"); + if (thr != NULL) + event->ptid = read_ptid (thr + strlen (";thread:"), + NULL); + else + event->ptid = magic_null_ptid; + } + + if (rsa == NULL) + { + inferior *inf = (event->ptid == null_ptid + ? NULL + : find_inferior_ptid (event->ptid)); + /* If this is the first time we learn anything + about this process, skip the registers + included in this packet, since we don't yet + know which architecture to use to parse them. + We'll determine the architecture later when + we process the stop reply and retrieve the + target description, via + remote_notice_new_inferior -> + post_create_inferior. */ + if (inf == NULL) + { + p = strchrnul (p1 + 1, ';'); + p++; + continue; + } + + event->arch = inf->gdbarch; + rsa = get_remote_arch_state (event->arch); + } + + packet_reg *reg + = packet_reg_from_pnum (event->arch, rsa, pnum); cached_reg_t cached_reg; - struct gdbarch *gdbarch = target_gdbarch (); if (reg == NULL) error (_("Remote sent bad register number %s: %s\n\ @@ -7026,13 +7069,13 @@ Packet: '%s'\n"), cached_reg.num = reg->regnum; cached_reg.data = (gdb_byte *) - xmalloc (register_size (gdbarch, reg->regnum)); + xmalloc (register_size (event->arch, reg->regnum)); p = p1 + 1; fieldsize = hex2bin (p, cached_reg.data, - register_size (gdbarch, reg->regnum)); + register_size (event->arch, reg->regnum)); p += 2 * fieldsize; - if (fieldsize < register_size (gdbarch, reg->regnum)) + if (fieldsize < register_size (event->arch, reg->regnum)) warning (_("Remote reply is too short: %s"), buf); VEC_safe_push (cached_reg_t, event->regcache, &cached_reg); @@ -7248,7 +7291,7 @@ process_stop_reply (struct stop_reply *stop_reply, if (stop_reply->regcache) { struct regcache *regcache - = get_thread_arch_regcache (ptid, target_gdbarch ()); + = get_thread_arch_regcache (ptid, stop_reply->arch); cached_reg_t *reg; int ix; @@ -7605,7 +7648,7 @@ process_g_packet (struct regcache *regcache) { struct gdbarch *gdbarch = get_regcache_arch (regcache); struct remote_state *rs = get_remote_state (); - struct remote_arch_state *rsa = get_remote_arch_state (); + remote_arch_state *rsa = get_remote_arch_state (gdbarch); int i, buf_len; char *p; char *regs; @@ -7739,7 +7782,8 @@ static void remote_fetch_registers (struct target_ops *ops, struct regcache *regcache, int regnum) { - struct remote_arch_state *rsa = get_remote_arch_state (); + struct gdbarch *gdbarch = get_regcache_arch (regcache); + remote_arch_state *rsa = get_remote_arch_state (gdbarch); int i; set_remote_traceframe (); @@ -7747,7 +7791,7 @@ remote_fetch_registers (struct target_ops *ops, if (regnum >= 0) { - struct packet_reg *reg = packet_reg_from_regnum (rsa, regnum); + packet_reg *reg = packet_reg_from_regnum (gdbarch, rsa, regnum); gdb_assert (reg != NULL); @@ -7773,7 +7817,7 @@ remote_fetch_registers (struct target_ops *ops, fetch_registers_using_g (regcache); - for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++) + for (i = 0; i < gdbarch_num_regs (gdbarch); i++) if (!rsa->regs[i].in_g_packet) if (!fetch_register_using_p (regcache, &rsa->regs[i])) { @@ -7789,7 +7833,7 @@ remote_fetch_registers (struct target_ops *ops, static void remote_prepare_to_store (struct target_ops *self, struct regcache *regcache) { - struct remote_arch_state *rsa = get_remote_arch_state (); + remote_arch_state *rsa = get_remote_arch_state (regcache->arch ()); int i; /* Make sure the entire registers array is valid. */ @@ -7855,7 +7899,7 @@ static void store_registers_using_G (const struct regcache *regcache) { struct remote_state *rs = get_remote_state (); - struct remote_arch_state *rsa = get_remote_arch_state (); + remote_arch_state *rsa = get_remote_arch_state (regcache->arch ()); gdb_byte *regs; char *p; @@ -7894,7 +7938,8 @@ static void remote_store_registers (struct target_ops *ops, struct regcache *regcache, int regnum) { - struct remote_arch_state *rsa = get_remote_arch_state (); + struct gdbarch *gdbarch = regcache->arch (); + remote_arch_state *rsa = get_remote_arch_state (gdbarch); int i; set_remote_traceframe (); @@ -7902,7 +7947,7 @@ remote_store_registers (struct target_ops *ops, if (regnum >= 0) { - struct packet_reg *reg = packet_reg_from_regnum (rsa, regnum); + packet_reg *reg = packet_reg_from_regnum (gdbarch, rsa, regnum); gdb_assert (reg != NULL); @@ -7926,7 +7971,7 @@ remote_store_registers (struct target_ops *ops, store_registers_using_G (regcache); - for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++) + for (i = 0; i < gdbarch_num_regs (gdbarch); i++) if (!rsa->regs[i].in_g_packet) if (!store_register_using_P (regcache, &rsa->regs[i])) /* See above for why we do not issue an error here. */ @@ -12653,7 +12698,8 @@ remote_get_trace_status (struct target_ops *self, struct trace_status *ts) if (packet_support (PACKET_qTStatus) == PACKET_DISABLE) return -1; - trace_regblock_size = get_remote_arch_state ()->sizeof_g_packet; + trace_regblock_size + = get_remote_arch_state (target_gdbarch ())->sizeof_g_packet; putpkt ("qTStatus"); diff --git a/gdb/spu-multiarch.c b/gdb/spu-multiarch.c index a935a72..e521c7e 100644 --- a/gdb/spu-multiarch.c +++ b/gdb/spu-multiarch.c @@ -121,7 +121,8 @@ spu_thread_architecture (struct target_ops *ops, ptid_t ptid) if (parse_spufs_run (ptid, &spufs_fd, &spufs_addr)) return spu_gdbarch (spufs_fd); - return target_gdbarch (); + target_ops *beneath = find_target_beneath (ops); + return beneath->to_thread_architecture (beneath, ptid); } /* Override the to_region_ok_for_hw_watchpoint routine. */ diff --git a/gdb/target.c b/gdb/target.c index aded0ba..f2dccd2 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -3137,7 +3137,9 @@ default_watchpoint_addr_within_range (struct target_ops *target, static struct gdbarch * default_thread_architecture (struct target_ops *ops, ptid_t ptid) { - return target_gdbarch (); + inferior *inf = find_inferior_ptid (ptid); + gdb_assert (inf != NULL); + return inf->gdbarch; } static int diff --git a/gdb/testsuite/gdb.multi/hangout.c b/gdb/testsuite/gdb.multi/hangout.c index 44dfba6..7266a18 100644 --- a/gdb/testsuite/gdb.multi/hangout.c +++ b/gdb/testsuite/gdb.multi/hangout.c @@ -16,14 +16,28 @@ along with this program. If not, see . */ #include +#include + +static void +hangout_loop (void) +{ +} int main(int argc, char *argv[]) { int i; + alarm (30); + for (i = 0; i < argc; ++i) { printf("Arg %d is %s\n", i, argv[i]); } + + while (1) + { + hangout_loop (); + usleep (20); + } } diff --git a/gdb/testsuite/gdb.multi/hello.c b/gdb/testsuite/gdb.multi/hello.c index 2c5bee9..7cc5f3c 100644 --- a/gdb/testsuite/gdb.multi/hello.c +++ b/gdb/testsuite/gdb.multi/hello.c @@ -16,6 +16,7 @@ along with this program. If not, see . */ #include +#include short hglob = 1; @@ -37,15 +38,27 @@ hello(int x) return x + 45; } +static void +hello_loop (void) +{ +} + int main() { int tmpx; + alarm (30); + bar(); tmpx = hello(glob); commonfun(); glob = tmpx; commonfun(); -} + while (1) + { + hello_loop (); + usleep (20); + } +} diff --git a/gdb/testsuite/gdb.multi/multi-arch.exp b/gdb/testsuite/gdb.multi/multi-arch.exp index d73b4f7..733afa0 100644 --- a/gdb/testsuite/gdb.multi/multi-arch.exp +++ b/gdb/testsuite/gdb.multi/multi-arch.exp @@ -96,3 +96,27 @@ if ![runto_main] then { gdb_test "info inferiors" \ "Executable.*${exec1}.*${exec2}.*" + +# Now select inferior 2, and trigger an event in inferior 1. This +# tries to check that GDB doesn't incorrectly uses the architecture of +# inferior 2 when parsing the expedited registers in a stop reply for +# inferior 1 (when remote debugging). + +gdb_test_no_output "set schedule-multiple on" + +with_test_prefix "inf1 event with inf2 selected" { + gdb_test "inferior 2" "Switching to inferior 2.*thread 2\.1.*main.*${srcfile2}.*" + gdb_test "b hello_loop" "Breakpoint .* at .*${srcfile1}.*" + gdb_test "c" " hello_loop.*" "continue to hello_loop" +} + +delete_breakpoints + +# Same, but the other way around: select inferior 1 and trigger an +# event in inferior 2. + +with_test_prefix "inf2 event with inf1 selected" { + gdb_test "inferior 1" "Switching to inferior 1.*thread 1\.1.*hello_loop.*${srcfile1}.*" + gdb_test "b hangout_loop" "Breakpoint .* at .*${srcfile2}.*" + gdb_test "c" " hangout_loop.*" "continue to hangout_loop" +} -- 2.5.5