From patchwork Fri Mar 24 14:49:11 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alan Hayward X-Patchwork-Id: 19721 Received: (qmail 100753 invoked by alias); 24 Mar 2017 14:49:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 100743 invoked by uid 89); 24 Mar 2017 14:49:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, MIME_BASE64_BLANKS, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 spammy=1116, 6286 X-HELO: EUR01-HE1-obe.outbound.protection.outlook.com Received: from mail-he1eur01on0050.outbound.protection.outlook.com (HELO EUR01-HE1-obe.outbound.protection.outlook.com) (104.47.0.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Mar 2017 14:49:14 +0000 Received: from AM3PR08MB0101.eurprd08.prod.outlook.com (10.160.211.19) by AM3PR08MB0103.eurprd08.prod.outlook.com (10.160.211.21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.991.14; Fri, 24 Mar 2017 14:49:11 +0000 Received: from AM3PR08MB0101.eurprd08.prod.outlook.com ([fe80::a542:8c73:7479:1ad3]) by AM3PR08MB0101.eurprd08.prod.outlook.com ([fe80::a542:8c73:7479:1ad3%15]) with mapi id 15.01.0991.017; Fri, 24 Mar 2017 14:49:11 +0000 From: Alan Hayward To: Yao Qi CC: "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c Date: Fri, 24 Mar 2017 14:49:11 +0000 Message-ID: References: <86lgspqisk.fsf@gmail.com> In-Reply-To: <86lgspqisk.fsf@gmail.com> authentication-results: gmail.com; dkim=none (message not signed) header.d=none; gmail.com; dmarc=none action=none header.from=arm.com; x-ms-exchange-messagesentrepresentingtype: 1 x-microsoft-exchange-diagnostics: 1; AM3PR08MB0103; 7:F+I3PeQO5ZYOPlxuK8daqACbDxTuT+iuMzl9+hOalFqwDBgNnhG8s8EPljYi4dHV9h7Pd7NDiWEZYmr61p7UZIvvu1woDKB0E/3Sh9tGfy538ExPwCucaf6qO+niDwMGYq9M3psaQObJ6l8XC/g0cxo7oAvw6OLT2sZsL+JmhQCW8ZXmZr47EMFSEZMUqvptB9AxQ93mGzQjbLlIT8DjFO3ra+U5N8eW8tnW3CjwQfET4FXuMDTfLNKbWNjW5RePVRZyQBGRnoBGFyTJioVN69SVPrInU5wBnK58SSE2F62aUPd9edFfliYqr/Cw+P35rMgAGNzXvkYg0XcCVmgMuA== x-ms-office365-filtering-correlation-id: 0cf61050-f9c8-4816-cab5-08d472c4ef2d x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(2017030254075)(48565401081); SRVR:AM3PR08MB0103; nodisclaimer: True x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(180628864354917); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040375)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026)(6041248)(20161123564025)(20161123562025)(20161123560025)(20161123555025)(20161123558025)(6072148); SRVR:AM3PR08MB0103; BCL:0; PCL:0; RULEID:; SRVR:AM3PR08MB0103; x-forefront-prvs: 0256C18696 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(39850400002)(39410400002)(39860400002)(39450400003)(39840400002)(377424004)(24454002)(3660700001)(1411001)(6916009)(8676002)(81166006)(6116002)(2900100001)(86362001)(575784001)(2906002)(2950100002)(102836003)(3846002)(305945005)(33656002)(7736002)(53546009)(25786009)(110136004)(38730400002)(36756003)(6246003)(4326008)(39060400002)(3280700002)(66066001)(6436002)(189998001)(5660300001)(6486002)(53936002)(50986999)(76176999)(54356999)(6506006)(5250100002)(83716003)(99286003)(54906002)(229853002)(6512007)(8936002)(82746002); DIR:OUT; SFP:1101; SCL:1; SRVR:AM3PR08MB0103; H:AM3PR08MB0101.eurprd08.prod.outlook.com; FPR:; SPF:None; MLV:ovrnspm; PTR:InfoNoRecords; LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-ID: <06C2CECF457CAE4CB3E8836AF3AFF50E@eurprd08.prod.outlook.com> MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Mar 2017 14:49:11.5883 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM3PR08MB0103 Patch update as per comments below. Tested using make check with board files unix and native-gdbserver. Ok to commit? I'm happy with Yao's patch below too. Alan. 2017-03-24 Alan Hayward * frame.c (get_frame_register_bytes): Unwind using value. (put_frame_register_bytes): Likewise. > On 1 Mar 2017, at 12:32, Yao Qi wrote: > > Alan Hayward writes: > >> @@ -1252,7 +1252,11 @@ frame_unwind_register_signed (struct frame_info *frame, int regnum) >> struct gdbarch *gdbarch = frame_unwind_arch (frame); >> enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); >> int size = register_size (gdbarch, regnum); >> - gdb_byte buf[MAX_REGISTER_SIZE]; >> + gdb_byte buf[sizeof (LONGEST)]; >> + >> + if (size > (int) sizeof (LONGEST)) >> + error (_("Cannot unwind integers more than %d bytes."), >> + (int) sizeof (LONGEST)); >> > > We apply the restriction of extract_signed_integer to its caller here. > People will wonder why do we have such check until he/she digs into > extract_signed_integer. My first reaction is to add some comments to > explain why do we do so, but the recent gdb::function_view reminds me > that we can do something differently (and better, IMO). > > Current pattern of using extract_unsigned_integer is > > 1) allocate an array on stack, > 2) read data from regcache or frame into the array, > 3) pass the array to extract_unsigned_integer > > we can pass a callable function_view as a content provider to > extract_unsigned_integer, so that we don't need step 1). The code > becomes, > > return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size) > { > frame_unwind_register (frame, regnum, buf); > }, size, byte_order); > > We can remove some uses of MAX_REGISTER_SIZE in this way. Do you (Alan > and others) like the patch below? > >> @@ -1460,11 +1473,15 @@ put_frame_register_bytes (struct frame_info *frame, int regnum, >> } >> else >> { >> - gdb_byte buf[MAX_REGISTER_SIZE]; >> - >> - deprecated_frame_register_read (frame, regnum, buf); >> - memcpy (buf + offset, myaddr, curr_len); >> - put_frame_register (frame, regnum, buf); >> + struct value *value = frame_unwind_register_value (frame->next, >> + regnum); >> + gdb_assert (value != NULL); >> + >> + memcpy ((char *) value_contents_all (value) + offset, myaddr, >> + curr_len); > > Use value_contents_writeable, > >> + put_frame_register (frame, regnum, value_contents_all (value)); > > s/value_contents_all/value_contents_raw/ > > because value_contents_all requires value is available but > deprecated_frame_register_read doesn't. > >> + release_value (value); >> + value_free (value); > > -- > Yao (齐尧) > From 2a76b39ffa87ed015f3637ee4a9d083f682863a0 Mon Sep 17 00:00:00 2001 > From: Yao Qi > Date: Wed, 1 Mar 2017 10:43:29 +0000 > Subject: [PATCH] Use content provider in extract_unsigned_integer > > --- > gdb/ada-lang.c | 9 ++++++--- > gdb/defs.h | 4 ++++ > gdb/findvar.c | 38 ++++++++++++++++++++++++++++++-------- > gdb/frame.c | 7 ++++--- > gdb/ia64-tdep.c | 19 +++++++++++++++---- > 5 files changed, 59 insertions(+), 18 deletions(-) > > diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c > index 753409c..07ce04a 100644 > --- a/gdb/ada-lang.c > +++ b/gdb/ada-lang.c > @@ -4555,12 +4555,15 @@ value_pointer (struct value *value, struct type *type) > { > struct gdbarch *gdbarch = get_type_arch (type); > unsigned len = TYPE_LENGTH (type); > - gdb_byte *buf = (gdb_byte *) alloca (len); > CORE_ADDR addr; > > addr = value_address (value); > - gdbarch_address_to_pointer (gdbarch, type, buf, addr); > - addr = extract_unsigned_integer (buf, len, gdbarch_byte_order (gdbarch)); > + addr = extract_unsigned_integer ([&] (gdb_byte *buf, size_t size) > + { > + gdbarch_address_to_pointer (gdbarch, > + type, buf, > + addr); > + }, len, gdbarch_byte_order (gdbarch)); > return addr; > } > > diff --git a/gdb/defs.h b/gdb/defs.h > index aa58605..a2f8fce 100644 > --- a/gdb/defs.h > +++ b/gdb/defs.h > @@ -628,6 +628,10 @@ extern LONGEST extract_signed_integer (const gdb_byte *, int, > extern ULONGEST extract_unsigned_integer (const gdb_byte *, int, > enum bfd_endian); > > +extern ULONGEST > + extract_unsigned_integer (gdb::function_view content_provider, > + int len, enum bfd_endian byte_order); > + > extern int extract_long_unsigned_integer (const gdb_byte *, int, > enum bfd_endian, LONGEST *); > > diff --git a/gdb/findvar.c b/gdb/findvar.c > index 80c709a..6d7d0de 100644 > --- a/gdb/findvar.c > +++ b/gdb/findvar.c > @@ -81,20 +81,15 @@ That operation is not available on integers of more than %d bytes."), > return retval; > } > > -ULONGEST > -extract_unsigned_integer (const gdb_byte *addr, int len, > - enum bfd_endian byte_order) > +static ULONGEST > +extract_unsigned_integer_1 (const gdb_byte *addr, int len, > + enum bfd_endian byte_order) > { > ULONGEST retval; > const unsigned char *p; > const unsigned char *startaddr = addr; > const unsigned char *endaddr = startaddr + len; > > - if (len > (int) sizeof (ULONGEST)) > - error (_("\ > -That operation is not available on integers of more than %d bytes."), > - (int) sizeof (ULONGEST)); > - > /* Start at the most significant end of the integer, and work towards > the least significant. */ > retval = 0; > @@ -111,6 +106,33 @@ That operation is not available on integers of more than %d bytes."), > return retval; > } > > +ULONGEST > +extract_unsigned_integer (const gdb_byte *addr, int len, > + enum bfd_endian byte_order) > +{ > + if (len > (int) sizeof (ULONGEST)) > + error (_("\ > +That operation is not available on integers of more than %d bytes."), > + (int) sizeof (ULONGEST)); > + > + return extract_unsigned_integer_1 (addr, len, byte_order); > +} > + > +ULONGEST > +extract_unsigned_integer (gdb::function_view content_provider, > + int len, enum bfd_endian byte_order) > +{ > + if (len > (int) sizeof (ULONGEST)) > + error (_("\ > +That operation is not available on integers of more than %d bytes."), > + (int) sizeof (ULONGEST)); > + > + gdb_byte buf[sizeof (ULONGEST)]; > + > + content_provider (buf, len); > + return extract_unsigned_integer_1 (buf, len, byte_order); > +} > + > /* Sometimes a long long unsigned integer can be extracted as a > LONGEST value. This is done so that we can print these values > better. If this integer can be converted to a LONGEST, this > diff --git a/gdb/frame.c b/gdb/frame.c > index d98003d..57f82f1 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -1270,10 +1270,11 @@ frame_unwind_register_unsigned (struct frame_info *frame, int regnum) > struct gdbarch *gdbarch = frame_unwind_arch (frame); > enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > int size = register_size (gdbarch, regnum); > - gdb_byte buf[MAX_REGISTER_SIZE]; > > - frame_unwind_register (frame, regnum, buf); > - return extract_unsigned_integer (buf, size, byte_order); > + return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size) > + { > + frame_unwind_register (frame, regnum, buf); > + }, size, byte_order); > } > > ULONGEST > diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c > index 4c53bc6..2748dac 100644 > --- a/gdb/ia64-tdep.c > +++ b/gdb/ia64-tdep.c > @@ -1516,7 +1516,6 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc, > else if (qp == 0 && rN == 2 > && ((rM == fp_reg && fp_reg != 0) || rM == 12)) > { > - gdb_byte buf[MAX_REGISTER_SIZE]; > CORE_ADDR saved_sp = 0; > /* adds r2, spilloffset, rFramePointer > or > @@ -1534,8 +1533,15 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc, > { > struct gdbarch *gdbarch = get_frame_arch (this_frame); > enum bfd_endian byte_order = gdbarch_byte_order (gdbarch); > - get_frame_register (this_frame, sp_regnum, buf); > - saved_sp = extract_unsigned_integer (buf, 8, byte_order); > + > + saved_sp > + = extract_unsigned_integer ([&] (gdb_byte * buf, > + size_t size) > + { > + get_frame_register (this_frame, > + sp_regnum, > + buf); > + }, 8, byte_order); > } > spill_addr = saved_sp > + (rM == 12 ? 0 : mem_stack_frame_size) > @@ -1782,7 +1788,12 @@ examine_prologue (CORE_ADDR pc, CORE_ADDR lim_pc, > else if (cfm_reg != 0) > { > get_frame_register (this_frame, cfm_reg, buf); > - cfm = extract_unsigned_integer (buf, 8, byte_order); > + cfm = extract_unsigned_integer ([&] (gdb_byte * buf, size_t size) > + { > + get_frame_register (this_frame, > + cfm_reg, > + buf); > + }, 8, byte_order); > } > cache->prev_cfm = cfm; > > -- > 1.9.1 diff --git a/gdb/frame.c b/gdb/frame.c index d98003dee7c34a63bd25356e6674721664a4b2f3..05a3be400c35ada0de48fd529e9cc83e0eaa941d 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1410,16 +1410,21 @@ get_frame_register_bytes (struct frame_info *frame, int regnum, } else { - gdb_byte buf[MAX_REGISTER_SIZE]; - enum lval_type lval; - CORE_ADDR addr; - int realnum; + struct value *value = frame_unwind_register_value (frame->next, + regnum); + gdb_assert (value != NULL); + *optimizedp = value_optimized_out (value); + *unavailablep = !value_entirely_available (value); - frame_register (frame, regnum, optimizedp, unavailablep, - &lval, &addr, &realnum, buf); if (*optimizedp || *unavailablep) - return 0; - memcpy (myaddr, buf + offset, curr_len); + { + release_value (value); + value_free (value); + return 0; + } + memcpy (myaddr, value_contents_all (value) + offset, curr_len); + release_value (value); + value_free (value); } myaddr += curr_len; @@ -1460,11 +1465,15 @@ put_frame_register_bytes (struct frame_info *frame, int regnum, } else { - gdb_byte buf[MAX_REGISTER_SIZE]; - - deprecated_frame_register_read (frame, regnum, buf); - memcpy (buf + offset, myaddr, curr_len); - put_frame_register (frame, regnum, buf); + struct value *value = frame_unwind_register_value (frame->next, + regnum); + gdb_assert (value != NULL); + + memcpy ((char *) value_contents_writeable (value) + offset, myaddr, + curr_len); + put_frame_register (frame, regnum, value_contents_raw (value)); + release_value (value); + value_free (value); } myaddr += curr_len;