Message ID | 434A7317-C19A-4B53-8CB1-C7B4ACEC7D17@arm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 122395 invoked by alias); 7 Jun 2017 08:31:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 122379 invoked by uid 89); 7 Jun 2017 08:31:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 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= X-HELO: EUR01-VE1-obe.outbound.protection.outlook.com Received: from mail-ve1eur01on0089.outbound.protection.outlook.com (HELO EUR01-VE1-obe.outbound.protection.outlook.com) (104.47.1.89) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 07 Jun 2017 08:31:51 +0000 Received: from AM3PR08MB0101.eurprd08.prod.outlook.com (10.160.211.19) by AM3PR08MB0102.eurprd08.prod.outlook.com (10.160.211.20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.1157.12; Wed, 7 Jun 2017 08:31:52 +0000 Received: from AM3PR08MB0101.eurprd08.prod.outlook.com ([fe80::f0a8:fd0f:69e1:e280]) by AM3PR08MB0101.eurprd08.prod.outlook.com ([fe80::f0a8:fd0f:69e1:e280%17]) with mapi id 15.01.1157.012; Wed, 7 Jun 2017 08:31:52 +0000 From: Alan Hayward <Alan.Hayward@arm.com> To: Yao Qi <qiyaoltc@gmail.com> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, nd <nd@arm.com> Subject: Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4) Date: Wed, 7 Jun 2017 08:31:52 +0000 Message-ID: <434A7317-C19A-4B53-8CB1-C7B4ACEC7D17@arm.com> References: <3C00280E-37C9-4C0A-9DA6-F3B9DB1A6E8F@arm.com> <86y3v7uf9j.fsf@gmail.com> <0150DDF9-6204-4F4F-99E9-D757C1DBD512@arm.com> In-Reply-To: <0150DDF9-6204-4F4F-99E9-D757C1DBD512@arm.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-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM3PR08MB0102; 7:HMZeJaTKUzv1ZY/+Tcyp1qvYm3XNH3tYtj6x4v4Hc3dudB/N1L2jerGeZiQtFJuo1M7oGY3+GmPNHR067tdn/rt/FDQwgMfkXn7Y1rzjIo7LidVUo/p7HSuRD9VMDoZ+DdI793n5W9u/c0GoQUhlPR0by738amZUJ5I6ya+IFG8UjrjkZdPtcL8sC7v1OViYtvEXxQp8ZNrxBLYIdm4JgBG49WTWpyQh1gMsu4ndnTrBr+SnrytHzcZJNL8NfSakQg+Ys8br5Gz4ECL7TvFg2vHRE5Mlm09EQ4kyy+imrWc7TJyrMd3Yu6oCk1HP+/5b2UzOGJZYJmj3N8h4m9hg0Q== x-ms-traffictypediagnostic: AM3PR08MB0102: x-ms-office365-filtering-correlation-id: de37937a-f447-4c66-01ca-08d4ad7fa5fd x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001)(2017030254075)(48565401081)(201703131423075)(201703031133081)(201702281549075); SRVR:AM3PR08MB0102; nodisclaimer: True x-microsoft-antispam-prvs: <AM3PR08MB0102C08CE19717B95B6EEC3697C80@AM3PR08MB0102.eurprd08.prod.outlook.com> x-exchange-antispam-report-test: UriScan:(180628864354917); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(601004)(2401047)(8121501046)(5005006)(93006095)(93001095)(3002001)(10201501046)(100000703101)(100105400095)(6055026)(6041248)(201703131423075)(201703011903075)(201702281528075)(201703061421075)(20161123558100)(20161123564025)(20161123555025)(20161123560025)(20161123562025)(6072148)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:AM3PR08MB0102; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:AM3PR08MB0102; x-forefront-prvs: 03319F6FEF x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(39850400002)(39840400002)(39400400002)(39860400002)(39450400003)(39410400002)(24454002)(377424004)(7736002)(54356999)(4326008)(508600001)(72206003)(6246003)(6916009)(305945005)(6486002)(39060400002)(6506006)(53546009)(3280700002)(25786009)(229853002)(3660700001)(76176999)(6436002)(99286003)(36756003)(54906002)(6512007)(38730400002)(189998001)(110136004)(2950100002)(81166006)(50986999)(53936002)(8936002)(2900100001)(66066001)(6116002)(1411001)(102836003)(3846002)(106356001)(33656002)(8676002)(2906002)(5250100002)(5660300001)(575784001)(83716003)(82746002)(86362001)(14454004); DIR:OUT; SFP:1101; SCL:1; SRVR:AM3PR08MB0102; H:AM3PR08MB0101.eurprd08.prod.outlook.com; FPR:; SPF:None; MLV:ovrnspm; PTR:InfoNoRecords; LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="utf-8" Content-ID: <9C51195F99CC694791C7B718E494C047@eurprd08.prod.outlook.com> Content-Transfer-Encoding: base64 MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Jun 2017 08:31:52.2262 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM3PR08MB0102 |
Commit Message
Alan Hayward
June 7, 2017, 8:31 a.m. UTC
> On 5 May 2017, at 09:04, Alan Hayward <Alan.Hayward@arm.com> wrote: > > >> On 11 Apr 2017, at 16:37, Yao Qi <qiyaoltc@gmail.com> wrote: >> >> Alan Hayward <Alan.Hayward@arm.com> writes: >> I’ve rebased this patch due to Yao’s unit test changes. I don't have a MIPS machine to test on. Tested on a --enable-targets=all and asan build using make check with board files unix, native-gdbserver and unittest Ok to commit? Alan. 2017-05-30 Alan Hayward <alan.hayward@arm.com> * mips-tdep.c (mips_eabi_push_dummy_call): Hard code buffer size.
Comments
On Wed, 7 Jun 2017, Alan Hayward wrote: > I don't have a MIPS machine to test on. I could schedule a test run over the coming weekend, however your change applies to EABI support, which I believe is long dead (as in: nobody uses it). Consequently I don't have a way to test with an EABI target either. > diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c > index 82f91ba2cd950c5f48f8f408f645ea49e952ef29..52d2ca134f8d14f54c6f4e450c84597c4d6a0e0e 100644 > --- a/gdb/mips-tdep.c > +++ b/gdb/mips-tdep.c > @@ -4528,7 +4528,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function, > for (argnum = 0; argnum < nargs; argnum++) > { > const gdb_byte *val; > - gdb_byte valbuf[MAX_REGISTER_SIZE]; > + gdb_byte valbuf[8]; > struct value *arg = args[argnum]; > struct type *arg_type = check_typedef (value_type (arg)); > int len = TYPE_LENGTH (arg_type); If you need to get rid of MAX_REGISTER_SIZE here (what was reason again?), then why not simply use `regsize' instead as the array size? AFAICS `valbuf' is only written once, with the size of data requested specified exactly as `regsize'. The only explanation I have been able to come up with as to why MAX_REGISTER_SIZE has been chosen for `valbuf' allocation is it was made before we could assume variable-length automatic array support. > @@ -4544,6 +4544,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function, > if (len > regsize > && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION)) > { > + gdb_assert (regsize <= 8); > store_unsigned_integer (valbuf, regsize, byte_order, > value_address (arg)); > typecode = TYPE_CODE_PTR; You obviously don't need the assertion then. Maciej
> On 8 Jun 2017, at 21:26, Maciej W. Rozycki <macro@imgtec.com> wrote: > > On Wed, 7 Jun 2017, Alan Hayward wrote: > >> I don't have a MIPS machine to test on. > > I could schedule a test run over the coming weekend, however your change > applies to EABI support, which I believe is long dead (as in: nobody uses > it). Consequently I don't have a way to test with an EABI target either. Thanks for the offer, but sounds like the only real option then is to just ensure the change is safe enough to not break. > >> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c >> index 82f91ba2cd950c5f48f8f408f645ea49e952ef29..52d2ca134f8d14f54c6f4e450c84597c4d6a0e0e 100644 >> --- a/gdb/mips-tdep.c >> +++ b/gdb/mips-tdep.c >> @@ -4528,7 +4528,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function, >> for (argnum = 0; argnum < nargs; argnum++) >> { >> const gdb_byte *val; >> - gdb_byte valbuf[MAX_REGISTER_SIZE]; >> + gdb_byte valbuf[8]; >> struct value *arg = args[argnum]; >> struct type *arg_type = check_typedef (value_type (arg)); >> int len = TYPE_LENGTH (arg_type); > > If you need to get rid of MAX_REGISTER_SIZE here (what was reason > again?), then why not simply use `regsize' instead as the array size? > > AFAICS `valbuf' is only written once, with the size of data requested > specified exactly as `regsize'. The only explanation I have been able to > come up with as to why MAX_REGISTER_SIZE has been chosen for `valbuf' > allocation is it was made before we could assume variable-length automatic > array support. The reason for getting rid of MAX_REGISTER_SIZE is that Aarch64 SVE will require increasing the size massively, with potential for future increases. Hence the large number of patches that are gradually removing MAX_REGISTER_SIZE. I’ve avoided using variable-length arrays because it has the potential to break the stack. However, here we know that we aren’t going to get a value >8, so maybe in this case a VLA would be ok? Anyone else have an opinion here? Alan.
On 06/09/2017 11:31 AM, Alan Hayward wrote: > I’ve avoided using variable-length arrays because it has the potential to > break the stack. > However, here we know that we aren’t going to get a value >8, so maybe in > this case a VLA would be ok? > > Anyone else have an opinion here? VLAs are not standard C++, unfortunately. Do we know whether all compilers we care about support them? It doesn't seem worth it to me to rely on compiler extensions when we know we're always going to see a size <=8. Thanks, Pedro Alves
On Fri, 9 Jun 2017, Pedro Alves wrote: > > I’ve avoided using variable-length arrays because it has the potential to > > break the stack. > > However, here we know that we aren’t going to get a value >8, so maybe in > > this case a VLA would be ok? > > > > Anyone else have an opinion here? > > VLAs are not standard C++, unfortunately. Do we know whether all compilers > we care about support them? It doesn't seem worth it to me to rely > on compiler extensions when we know we're always going to see a size <=8. Hmm, `alloca' then? It used to be used here actually, up till commit d9d9c31f3130 ("MAX_REGISTER_RAW_SIZE -> MAX_REGISTER_SIZE"), <https://sourceware.org/ml/gdb-patches/2003-05/msg00127.html>. Maciej
On 06/09/2017 12:48 PM, Maciej W. Rozycki wrote: > On Fri, 9 Jun 2017, Pedro Alves wrote: > >>> I’ve avoided using variable-length arrays because it has the potential to >>> break the stack. >>> However, here we know that we aren’t going to get a value >8, so maybe in >>> this case a VLA would be ok? >>> >>> Anyone else have an opinion here? >> >> VLAs are not standard C++, unfortunately. Do we know whether all compilers >> we care about support them? It doesn't seem worth it to me to rely >> on compiler extensions when we know we're always going to see a size <=8. > > Hmm, `alloca' then? It used to be used here actually, up till commit > d9d9c31f3130 ("MAX_REGISTER_RAW_SIZE -> MAX_REGISTER_SIZE"), > <https://sourceware.org/ml/gdb-patches/2003-05/msg00127.html>. IMO, alloca calls are a red flag, which force you to reason about whether you're dangerously calling it in a loop such as in this case, because alloca memory is unwound on function exit, not scope exit. This is both a concern when an alloca call is introduced, and later when code is moved around/refactored. If we know the hard upper bound, and it's just 8 bytes, I don't see the point of making it variable. Alignment and padding for following variables plus the magic needed to handle variable length frames end up negating the saving anyway. Also, the function already hardcodes "8" in several places. IMO, here it'd be better to keep it simple and use a static upper bound. Thanks, Pedro Alves
On Fri, 9 Jun 2017, Pedro Alves wrote: > > Hmm, `alloca' then? It used to be used here actually, up till commit > > d9d9c31f3130 ("MAX_REGISTER_RAW_SIZE -> MAX_REGISTER_SIZE"), > > <https://sourceware.org/ml/gdb-patches/2003-05/msg00127.html>. > > IMO, alloca calls are a red flag, which force you to reason about > whether you're dangerously calling it in a loop such as in > this case, because alloca memory is unwound on function exit, not > scope exit. This is both a concern when an alloca call is > introduced, and later when code is moved around/refactored. If we know > the hard upper bound, and it's just 8 bytes, I don't see the point > of making it variable. Alignment and padding for following > variables plus the magic needed to handle variable length frames end up > negating the saving anyway. Also, the function already hardcodes "8" > in several places. IMO, here it'd be better to keep it simple > and use a static upper bound. Contrariwise `mips_read_fp_register_single' already uses `alloca' for a similar purpose. Good point about the loop though; moving the declaration and allocation outside the loop will easily solve the problem however. Hardcoding things, and especially with literals, causes a maintenance pain when things change. Bad code elsewhere is not an excuse; besides, the other places where `8' is hardcoded are not (buffer) limits, but just handle specific register sizes, which are not going to change, and which are a completely different matter. So if you find `alloca' unacceptable, then the limit has to be a macro, and an assertion check is also due (as already proposed), preferably using `sizeof', so that a future change does not break it by accident. NB the MSA ASE expands FPRs to 16 bytes and we'll soon have to accomodate that; patches have already been posted and are being worked on. Maciej
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c index 82f91ba2cd950c5f48f8f408f645ea49e952ef29..52d2ca134f8d14f54c6f4e450c84597c4d6a0e0e 100644 --- a/gdb/mips-tdep.c +++ b/gdb/mips-tdep.c @@ -4528,7 +4528,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function, for (argnum = 0; argnum < nargs; argnum++) { const gdb_byte *val; - gdb_byte valbuf[MAX_REGISTER_SIZE]; + gdb_byte valbuf[8]; struct value *arg = args[argnum]; struct type *arg_type = check_typedef (value_type (arg)); int len = TYPE_LENGTH (arg_type); @@ -4544,6 +4544,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function, if (len > regsize && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION)) { + gdb_assert (regsize <= 8); store_unsigned_integer (valbuf, regsize, byte_order, value_address (arg)); typecode = TYPE_CODE_PTR;