From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38471 invoked by alias); 9 Mar 2018 10:43:12 -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 38452 invoked by uid 89); 9 Mar 2018 10:43:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=HX-Received:sk:l13mr17 X-HELO: mail-wm0-f65.google.com Received: from mail-wm0-f65.google.com (HELO mail-wm0-f65.google.com) (74.125.82.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 09 Mar 2018 10:43:09 +0000 Received: by mail-wm0-f65.google.com with SMTP id w128so3101114wmw.0 for ; Fri, 09 Mar 2018 02:43:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=JN9PdLRPlzD/gZWKRCM3A6BXbp5v5tRq7Wb51UWqqsU=; b=VEfeVbAQisQkgAazm/yMyzZSHgBKp2+3icA/L/2j5CiT1HavwoAGH5avzJZMs+oqie WCT36j+Q3ecbE8lAqQQD3gzW3iH6uyQYeGOC18KgsrixmA6IrAuQJx13aVvndMnEWcr4 t4Pdw6rHMw212fLlk+/y7PwW92V/XDVZD8VWyUoFqMialAPf1FWbDVXb9PzW6eyqwWhc Lxp19fn5fXOWNmpje+7Mh4G2blzUFLNjh6O2TDhgsf4npGn16Kb10eIt8E+96rmoMxub AaIFLDyqd8zjG3q3qgv68gQoN4P1fdokM5OHd3ECidnSfCfgd/erNu2y381x32w69aRB mIVQ== X-Gm-Message-State: AElRT7GaLXMpwqjmmFr4TEIXf5lALszaKguI7DGOyojf8q1yFwu3DbYt UOAVuqj1v881DGKuMvBiFv6naXs4 X-Google-Smtp-Source: AG47ELsbYKbfvSudLIJOZB6WbfthXvLdaNI8bzbylA2Nq4hf0c9Vk1vCuAS6ZXZcyt+1bbo8YX4OXQ== X-Received: by 10.28.212.13 with SMTP id l13mr1757131wmg.123.1520592186751; Fri, 09 Mar 2018 02:43:06 -0800 (PST) Received: from localhost (host86-164-103-156.range86-164.btcentralplus.com. [86.164.103.156]) by smtp.gmail.com with ESMTPSA id y8sm763742wmb.48.2018.03.09.02.43.05 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 09 Mar 2018 02:43:06 -0800 (PST) Date: Fri, 09 Mar 2018 10:43:00 -0000 From: Andrew Burgess To: Sergio Durigan Junior Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: Fix riscv ARI issues Message-ID: <20180309104304.GA2671@embecosm.com> References: <20180307224121.19941-1-andrew.burgess@embecosm.com> <871sgutqm9.fsf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <871sgutqm9.fsf@redhat.com> X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes X-SW-Source: 2018-03/txt/msg00191.txt.bz2 Sergio, Thanks for taking the time to review my patch. I just wanted to follow up on your feedback to help clarify the coding standard. * Sergio Durigan Junior [2018-03-08 12:38:06 -0500]: > On Wednesday, March 07 2018, Andrew Burgess wrote: >=20 > > Fixes some ARI issues in recently added riscv code, the ARI email is: > > > > https://sourceware.org/ml/gdb-patches/2018-03/msg00156.html >=20 > Thanks for the patch, Andrew. Comment below. >=20 > > gdb/ChangeLog: > > > > * riscv-tdep.c (riscv_register_name): Use xsnprintf instead of > > sprintf. > > (riscv_insn::fetch_instruction): Use gdb_assert instead of > > internal_error. > > (riscv_print_arg_location): Use gdb_assert_not_reached instead of > > error. > > (riscv_push_dummy_call): Likewise. > > --- > > gdb/ChangeLog | 10 ++++++++++ > > gdb/riscv-tdep.c | 14 ++++++-------- > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > > index 11b12279321..d84e7aba76a 100644 > > --- a/gdb/riscv-tdep.c > > +++ b/gdb/riscv-tdep.c > > @@ -481,7 +481,7 @@ riscv_register_name (struct gdbarch *gdbarch, int r= egnum) > > { > > static char buf[20]; > >=20=20 > > - sprintf (buf, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); > > + xsnprintf (buf, 20, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); > > return buf; > > } > >=20=20 > > @@ -1049,12 +1049,10 @@ riscv_insn::fetch_instruction (struct gdbarch *= gdbarch, > >=20=20 > > /* If we need more, grab it now. */ > > instlen =3D riscv_insn_length (buf[0]); > > + gdb_assert (instlen <=3D sizeof (buf)); > > *len =3D instlen; > > - if (instlen > sizeof (buf)) > > - internal_error (__FILE__, __LINE__, > > - _("%s: riscv_insn_length returned %i"), > > - __func__, instlen); > > - else if (instlen > 2) > > + > > + if (instlen > 2) > > { > > status =3D target_read_memory (addr + 2, buf + 2, instlen - 2); > > if (status) > > @@ -2009,7 +2007,7 @@ riscv_print_arg_location (ui_file *stream, struct= gdbarch *gdbarch, > > break; > >=20=20 > > default: > > - error ("unknown argument location type"); > > + gdb_assert_not_reached ("unknown argument location type"); >=20 > Strings must be marked for localization, with _(): >=20 > gdb_assert_not_reached (_("unknown argument location type")); >=20 > I also think it's a good thing to start the sentence with a capital > letter. I referenced the GNU Standard here: https://www.gnu.org/prep/standards/standards.html#Errors Specifically these two paragraphs are I think relevant: "The string message should not begin with a capital letter when it follows a program name and/or file name, because that isn=E2=80=99t the beginning of a sentence. (The sentence conceptually starts at the beginning of the line.) Also, it should not end with a period. Error messages from interactive programs, and other messages such as usage messages, should start with a capital letter. But they should not end with a period." Though GDB is clearly an interactive program and so the second paragraph should apply, the 'gdb_assert_not_reached' does prefix the message with some text like this: filename:line: internal-error: function: message So, to me, it feels like the first paragraph should apply and the message should start with no capital letter. Further, a survey of the 157 uses of gdb_assert_not_reached show: 142 start with a lower case letter 1 starts '...' 7 start with a capital letter 6 start with LOC_COMPUTED 1 is the empty string So, should message strings that are going to be prefixed be capitalised? I'll fix the internationalisation as requested. Thanks, Andrew >=20 > > } > > } > >=20=20 > > @@ -2149,7 +2147,7 @@ riscv_push_dummy_call (struct gdbarch *gdbarch, > > break; > >=20=20 > > default: > > - error ("unknown argument location type"); > > + gdb_assert_not_reached ("unknown argument location type"); >=20 > Likewise. >=20 > > } > >=20=20 > > if (second_arg_length > 0) > > --=20 > > 2.14.3 >=20 > Thanks, >=20 > --=20 > Sergio > GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 > Please send encrypted e-mail if possible > http://sergiodj.net/