From patchwork Fri Feb 3 09:59:13 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alan Hayward X-Patchwork-Id: 19107 Received: (qmail 108703 invoked by alias); 3 Feb 2017 09:59:28 -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 108623 invoked by uid 89); 3 Feb 2017 09:59:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.9 required=5.0 tests=AWL, BAYES_50, KAM_LOTSOFHASH, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=no version=3.3.2 spammy=2028, 1876, U*brobecker, sk:brobeck X-HELO: EUR02-AM5-obe.outbound.protection.outlook.com Received: from mail-eopbgr00061.outbound.protection.outlook.com (HELO EUR02-AM5-obe.outbound.protection.outlook.com) (40.107.0.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 03 Feb 2017 09:59:17 +0000 Received: from AM5PR0801MB1809.eurprd08.prod.outlook.com (10.169.247.138) by AM5PR0801MB1811.eurprd08.prod.outlook.com (10.169.247.140) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.888.16; Fri, 3 Feb 2017 09:59:14 +0000 Received: from AM5PR0801MB1809.eurprd08.prod.outlook.com ([10.169.247.138]) by AM5PR0801MB1809.eurprd08.prod.outlook.com ([10.169.247.138]) with mapi id 15.01.0888.018; Fri, 3 Feb 2017 09:59:14 +0000 From: Alan Hayward To: Joel Brobecker CC: Pedro Alves , Yao Qi , "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH] Removal of uses of MAX_REGISTER_SIZE Date: Fri, 3 Feb 2017 09:59:13 +0000 Message-ID: References: <7CF07197-4FED-4970-BB4B-2FE828E29A63@arm.com> <45e3a5e1-a9aa-1bc0-5d08-526b89fc458e@redhat.com> <20170201124123.GA27498@E107787-LIN> <20170202094012.dge4r6rsl2skdrii@adacore.com> In-Reply-To: <20170202094012.dge4r6rsl2skdrii@adacore.com> authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan.Hayward@arm.com; x-ms-exchange-messagesentrepresentingtype: 1 x-ms-office365-filtering-correlation-id: 7e2414b9-285a-4b29-727b-08d44c1b4f12 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(48565401081); SRVR:AM5PR0801MB1811; x-microsoft-exchange-diagnostics: 1; AM5PR0801MB1811; 7:Bm5scGvs6lfpwsmXPaqmrl05zy2yQ7w6pIrB7RzKX8G0fVFwP3hU0a36OOJSzMR0QqfAzKiGZ1H33g/zp+gvTEo95lU3z8Pmyl+al5faFdqprzspgxJsTd7rRsI/5fNeyy4iVeFxtixb0Zv3OPWzAn8Bx5pG4Ab0Fr32nWM13tFnGruPXH19CN+ZzUH/vNiopM40LQYxNz42ogsHCCwUQemWNEJ+csIb7rLGBFAOUwHRz+RFEWYtovbZxogOO6XvJ/32PbfGNTdtCo23j5PEDlv7v06EoFpRwyDoOZ+8b+KfXXqwiwTCFzItuiVvxwcxXjml05bVZprIES2r41IDBw8lj5F417DrvCs/F3KaMYpTGx+5e7oThpYmxTNaI+phLfH0edEz6RN22Oh8IyP6DPogpYE1GxuspZT03wJ8ih6k4bRNWiZrftUEFdzF1DYsT6AkQVdnIsSiD/lj68/aNKOVkPrzGf2GVX72jJpp35p1WM8cOYbxlAYIDKRgEkRMCED7525DpyN/jZutL86a9w== 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)(5005006)(8121501046)(10201501046)(3002001)(6055026)(6041248)(20161123555025)(20161123560025)(20161123564025)(20161123558025)(20161123562025)(6072148); SRVR:AM5PR0801MB1811; BCL:0; PCL:0; RULEID:; SRVR:AM5PR0801MB1811; x-forefront-prvs: 02070414A1 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(39840400002)(39410400002)(39850400002)(39450400003)(199003)(377424004)(24454002)(189002)(6506006)(53546003)(38730400001)(6436002)(106356001)(106116001)(39060400001)(97736004)(77096006)(229853002)(81156014)(189998001)(6486002)(8936002)(81166006)(8676002)(68736007)(36756003)(101416001)(76176999)(99286003)(54356999)(3660700001)(6246003)(93886004)(25786008)(105586002)(54906002)(6512007)(50986999)(33656002)(53936002)(2900100001)(575784001)(7736002)(66066001)(122556002)(2906002)(92566002)(5660300001)(4326007)(305945005)(3280700002)(110136003)(2950100002)(6916009)(6116002)(102836003)(3846002)(83716003)(86362001)(82746002)(104396002); DIR:OUT; SFP:1101; SCL:1; SRVR:AM5PR0801MB1811; H:AM5PR0801MB1809.eurprd08.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-ID: MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Feb 2017 09:59:13.8187 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0801MB1811 > On 2 Feb 2017, at 09:40, Joel Brobecker wrote: > >> #2 - Switch to heap allocation >> >> Or std::vector or something like that that hides it. It's not clear >> whether that would have a noticeable performance impact. > > I would try that. I think using one of the standard C++ classes > looks a little more attractive to me, and would only consider > the lambda functions if we can show a noticeable performance > impact. Those two are not exclusive, by the way, but in the past, > we've always frowned on calls to alloca in a loop, and using > a xmalloc+cleanup combination has never been an issue in my cases. > I'd imagine that a standard C++ memory management class would be > fast enough for those same situations. > > -- > Joel We're not allocating much space, so I'd hope std::vector didn't cause much of a slowdown. Also, the allocas with lamdba's approach feels a little overcomplicated. Reworking my patch that adds max_register_size (), I've replaced the allocas with std::vector. This patch ok? If people are happy, I'll then rework the larger patch. Please let me know if there's a particular test you want me to run to test performance (although I suspect it would need the larger patch to get meaningful results). Thanks, Alan. 2017-02-03 Alan Hayward * regcache.c (struct regcache_descr): Add max_register_size. (max_register_size): New. (init_regcache_descr): Find max register size. (regcache_save): Use std::vector. (regcache_restore): Likewise. (regcache_dump): Likewise. * regcache.h (max_register_size): New. * remote.c (remote_prepare_to_store): Use std::vector. * frame.c (frame_unwind_register_signed): Likewise. (frame_unwind_register_unsigned): Likewise. (get_frame_register_bytes): Likewise. (put_frame_register_bytes): Likewise. diff --git a/gdb/frame.c b/gdb/frame.c index d98003dee7c34a63bd25356e6674721664a4b2f3..135ca2b753cf4d64d0322331199d008548839c8d 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1252,10 +1252,10 @@ 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]; + std::vector buf (size); - frame_unwind_register (frame, regnum, buf); - return extract_signed_integer (buf, size, byte_order); + frame_unwind_register (frame, regnum, buf.data ()); + return extract_signed_integer (buf.data (), size, byte_order); } LONGEST @@ -1270,10 +1270,10 @@ 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]; + std::vector buf (size); - frame_unwind_register (frame, regnum, buf); - return extract_unsigned_integer (buf, size, byte_order); + frame_unwind_register (frame, regnum, buf.data ()); + return extract_unsigned_integer (buf.data (), size, byte_order); } ULONGEST @@ -1410,16 +1410,16 @@ get_frame_register_bytes (struct frame_info *frame, int regnum, } else { - gdb_byte buf[MAX_REGISTER_SIZE]; + std::vector buf (max_register_size (gdbarch)); enum lval_type lval; CORE_ADDR addr; int realnum; frame_register (frame, regnum, optimizedp, unavailablep, - &lval, &addr, &realnum, buf); + &lval, &addr, &realnum, buf.data ()); if (*optimizedp || *unavailablep) return 0; - memcpy (myaddr, buf + offset, curr_len); + memcpy (myaddr, buf.data () + offset, curr_len); } myaddr += curr_len; @@ -1460,11 +1460,11 @@ put_frame_register_bytes (struct frame_info *frame, int regnum, } else { - gdb_byte buf[MAX_REGISTER_SIZE]; + std::vector buf (max_register_size (gdbarch)); - deprecated_frame_register_read (frame, regnum, buf); - memcpy (buf + offset, myaddr, curr_len); - put_frame_register (frame, regnum, buf); + deprecated_frame_register_read (frame, regnum, buf.data ()); + memcpy (buf.data () + offset, myaddr, curr_len); + put_frame_register (frame, regnum, buf.data ()); } myaddr += curr_len; diff --git a/gdb/regcache.h b/gdb/regcache.h index e5a7cf553279b8cc0d546ec1b8274cbf97e246d5..5bc99f5c1ef87318edf4e934ec60c7f1225e7561 100644 --- a/gdb/regcache.h +++ b/gdb/regcache.h @@ -202,6 +202,8 @@ extern struct type *register_type (struct gdbarch *gdbarch, int regnum); extern int register_size (struct gdbarch *gdbarch, int regnum); +/* Return the size of the largest register. */ +extern long max_register_size (struct gdbarch *gdbarch); /* Save/restore a register cache. The set of registers saved / restored into the DST regcache determined by the save_reggroup / diff --git a/gdb/regcache.c b/gdb/regcache.c index 9d28aa2c2114e0f1c52758bb2fbe9669a329c13e..522633ae0fdf6d80508d725bc1d68d05567fd9ff 100644 --- a/gdb/regcache.c +++ b/gdb/regcache.c @@ -73,6 +73,9 @@ struct regcache_descr /* Cached table containing the type of each register. */ struct type **register_type; + + /* Size of the largest register. */ + long max_register_size; }; static void * @@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch) descr->register_offset[i] = offset; offset += descr->sizeof_register[i]; gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]); + descr->max_register_size = std::max (descr->max_register_size, + descr->sizeof_register[i]); } /* Set the real size of the raw register cache buffer. */ descr->sizeof_raw_registers = offset; @@ -136,6 +141,8 @@ init_regcache_descr (struct gdbarch *gdbarch) descr->register_offset[i] = offset; offset += descr->sizeof_register[i]; gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]); + descr->max_register_size = std::max (descr->max_register_size, + descr->sizeof_register[i]); } /* Set the real size of the readonly register cache buffer. */ descr->sizeof_cooked_registers = offset; @@ -187,6 +194,13 @@ regcache_register_size (const struct regcache *regcache, int n) return register_size (get_regcache_arch (regcache), n); } +long +max_register_size (struct gdbarch *gdbarch) +{ + struct regcache_descr *descr = regcache_descr (gdbarch); + return descr->max_register_size; +} + /* The register cache for storing raw register values. */ struct regcache @@ -327,7 +341,7 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read, void *src) { struct gdbarch *gdbarch = dst->descr->gdbarch; - gdb_byte buf[MAX_REGISTER_SIZE]; + std::vector buf (max_register_size (gdbarch)); int regnum; /* The DST should be `read-only', if it wasn't then the save would @@ -346,10 +360,10 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read, { if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup)) { - enum register_status status = cooked_read (src, regnum, buf); + enum register_status status = cooked_read (src, regnum, buf.data ()); if (status == REG_VALID) - memcpy (register_buffer (dst, regnum), buf, + memcpy (register_buffer (dst, regnum), buf.data (), register_size (gdbarch, regnum)); else { @@ -369,7 +383,7 @@ regcache_restore (struct regcache *dst, void *cooked_read_context) { struct gdbarch *gdbarch = dst->descr->gdbarch; - gdb_byte buf[MAX_REGISTER_SIZE]; + std::vector buf (max_register_size (gdbarch)); int regnum; /* The dst had better not be read-only. If it is, the `restore' @@ -385,9 +399,9 @@ regcache_restore (struct regcache *dst, { enum register_status status; - status = cooked_read (cooked_read_context, regnum, buf); + status = cooked_read (cooked_read_context, regnum, buf.data ()); if (status == REG_VALID) - regcache_cooked_write (dst, regnum, buf); + regcache_cooked_write (dst, regnum, buf.data ()); } } } @@ -1279,7 +1293,7 @@ regcache_dump (struct regcache *regcache, struct ui_file *file, int footnote_register_offset = 0; int footnote_register_type_name_null = 0; long register_offset = 0; - gdb_byte buf[MAX_REGISTER_SIZE]; + std::vector buf (max_register_size (gdbarch)); #if 0 fprintf_unfiltered (file, "nr_raw_registers %d\n", @@ -1406,8 +1420,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file, fprintf_unfiltered (file, ""); else { - regcache_raw_read (regcache, regnum, buf); - print_hex_chars (file, buf, + regcache_raw_read (regcache, regnum, buf.data ()); + print_hex_chars (file, buf.data (), regcache->descr->sizeof_register[regnum], gdbarch_byte_order (gdbarch)); } @@ -1422,13 +1436,13 @@ regcache_dump (struct regcache *regcache, struct ui_file *file, { enum register_status status; - status = regcache_cooked_read (regcache, regnum, buf); + status = regcache_cooked_read (regcache, regnum, buf.data ()); if (status == REG_UNKNOWN) fprintf_unfiltered (file, ""); else if (status == REG_UNAVAILABLE) fprintf_unfiltered (file, ""); else - print_hex_chars (file, buf, + print_hex_chars (file, buf.data (), regcache->descr->sizeof_register[regnum], gdbarch_byte_order (gdbarch)); } diff --git a/gdb/remote.c b/gdb/remote.c index c4cec910c44cf91cc7f36b7f2d87cde3f46de41e..157a1b1789d2a248c11dcc4efebd8ce54da82045 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -7757,9 +7757,10 @@ remote_fetch_registers (struct target_ops *ops, static void remote_prepare_to_store (struct target_ops *self, struct regcache *regcache) { + struct gdbarch *gdbarch = get_regcache_arch (regcache); struct remote_arch_state *rsa = get_remote_arch_state (); int i; - gdb_byte buf[MAX_REGISTER_SIZE]; + std::vector buf (max_register_size (gdbarch)); /* Make sure the entire registers array is valid. */ switch (packet_support (PACKET_P)) @@ -7769,7 +7770,7 @@ remote_prepare_to_store (struct target_ops *self, struct regcache *regcache) /* Make sure all the necessary registers are cached. */ for (i = 0; i < gdbarch_num_regs (get_regcache_arch (regcache)); i++) if (rsa->regs[i].in_g_packet) - regcache_raw_read (regcache, rsa->regs[i].regnum, buf); + regcache_raw_read (regcache, rsa->regs[i].regnum, buf.data ()); break; case PACKET_ENABLE: break;