Message ID | 20180307224121.19941-1-andrew.burgess@embecosm.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 101910 invoked by alias); 7 Mar 2018 22:41:41 -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 99705 invoked by uid 89); 7 Mar 2018 22:41:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:1853 X-HELO: mail-wr0-f193.google.com Received: from mail-wr0-f193.google.com (HELO mail-wr0-f193.google.com) (209.85.128.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 07 Mar 2018 22:41:33 +0000 Received: by mail-wr0-f193.google.com with SMTP id k9so3776543wre.9 for <gdb-patches@sourceware.org>; Wed, 07 Mar 2018 14:41:27 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=a4PvG4iFmMDrB5vxGHl6MnZKvIVTSUNPSWKCePMhnYU=; b=D9Z+M1YVDiGbA1c1fcZ2Ub5/hsBrj1GN54214ovXMIQChW0AOvC/KnG9Tpu4gv70IZ lVhwt+tpqS8dGNudhyWb/laNH2tBvt3oV31q4tfZsqeZsJ4YIPJw7R7HwR9yYylX3L0E xR/ypIaIk6mkcWjGmLgAa1EgJD4YqC6QK0EFMmLe0QXOPzvKVFv3i37HKF55exo7FDBx h1VPIKmnRm9j47sUeLP9f++/Fq6wQEARjMZRX/Ivrwa/j6KZ5cUQdSbamqo3Kzr9ufLF evfmEcsFViSxSMPLyODCCPtbo6DL2XrsEa9hECC7h9h4U80f4I9BL0pzi+n07vA2AUTy hJ/Q== X-Gm-Message-State: APf1xPC7xXJNzGMsRNHgVeW5KH1RNI3gbtIeox6qTNMkG5R930qcRT3a chaix6TgNmU4xoXVl7HGcKeHFw9G X-Google-Smtp-Source: AG47ELt3k3WmZ6fs1RbkvMLw0jg+zoKxi/Dy0ReFLk7uBJN0Uxny5NKbGLx/qPQDJ/WLBWe2ZQLCfQ== X-Received: by 10.223.158.208 with SMTP id b16mr19465409wrf.274.1520462484967; Wed, 07 Mar 2018 14:41:24 -0800 (PST) Received: from localhost ([176.123.41.197]) by smtp.gmail.com with ESMTPSA id a67sm33992047wrc.13.2018.03.07.14.41.23 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 07 Mar 2018 14:41:24 -0800 (PST) From: Andrew Burgess <andrew.burgess@embecosm.com> To: gdb-patches@sourceware.org Cc: Andrew Burgess <andrew.burgess@embecosm.com> Subject: [PATCH] gdb: Fix riscv ARI issues Date: Wed, 7 Mar 2018 22:41:21 +0000 Message-Id: <20180307224121.19941-1-andrew.burgess@embecosm.com> X-IsSubscribed: yes |
Commit Message
Andrew Burgess
March 7, 2018, 10:41 p.m. UTC
Fixes some ARI issues in recently added riscv code, the ARI email is: https://sourceware.org/ml/gdb-patches/2018-03/msg00156.html 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(-)
Comments
On Wednesday, March 07 2018, Andrew Burgess wrote: > Fixes some ARI issues in recently added riscv code, the ARI email is: > > https://sourceware.org/ml/gdb-patches/2018-03/msg00156.html Thanks for the patch, Andrew. Comment below. > 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 regnum) > { > static char buf[20]; > > - sprintf (buf, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); > + xsnprintf (buf, 20, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); > return buf; > } > > @@ -1049,12 +1049,10 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch, > > /* If we need more, grab it now. */ > instlen = riscv_insn_length (buf[0]); > + gdb_assert (instlen <= sizeof (buf)); > *len = 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 = 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; > > default: > - error ("unknown argument location type"); > + gdb_assert_not_reached ("unknown argument location type"); Strings must be marked for localization, with _(): gdb_assert_not_reached (_("unknown argument location type")); I also think it's a good thing to start the sentence with a capital letter. > } > } > > @@ -2149,7 +2147,7 @@ riscv_push_dummy_call (struct gdbarch *gdbarch, > break; > > default: > - error ("unknown argument location type"); > + gdb_assert_not_reached ("unknown argument location type"); Likewise. > } > > if (second_arg_length > 0) > -- > 2.14.3 Thanks,
Hi Andrew, On top of what Sergio said: > 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 regnum) > { > static char buf[20]; > > - sprintf (buf, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); > + xsnprintf (buf, 20, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); sizeof (buf) instead of hard-coding 20? Thanks, Simon
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 <sergiodj@redhat.com> [2018-03-08 12:38:06 -0500]: > On Wednesday, March 07 2018, Andrew Burgess wrote: > > > Fixes some ARI issues in recently added riscv code, the ARI email is: > > > > https://sourceware.org/ml/gdb-patches/2018-03/msg00156.html > > Thanks for the patch, Andrew. Comment below. > > > 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 regnum) > > { > > static char buf[20]; > > > > - sprintf (buf, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); > > + xsnprintf (buf, 20, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); > > return buf; > > } > > > > @@ -1049,12 +1049,10 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch, > > > > /* If we need more, grab it now. */ > > instlen = riscv_insn_length (buf[0]); > > + gdb_assert (instlen <= sizeof (buf)); > > *len = 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 = 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; > > > > default: > > - error ("unknown argument location type"); > > + gdb_assert_not_reached ("unknown argument location type"); > > Strings must be marked for localization, with _(): > > gdb_assert_not_reached (_("unknown argument location type")); > > 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’t 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 > > > } > > } > > > > @@ -2149,7 +2147,7 @@ riscv_push_dummy_call (struct gdbarch *gdbarch, > > break; > > > > default: > > - error ("unknown argument location type"); > > + gdb_assert_not_reached ("unknown argument location type"); > > Likewise. > > > } > > > > if (second_arg_length > 0) > > -- > > 2.14.3 > > Thanks, > > -- > Sergio > GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 > Please send encrypted e-mail if possible > http://sergiodj.net/
On Friday, March 09 2018, Andrew Burgess wrote: > 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. Hi Andrew, > * Sergio Durigan Junior <sergiodj@redhat.com> [2018-03-08 12:38:06 -0500]: > >> On Wednesday, March 07 2018, Andrew Burgess wrote: >> >> > Fixes some ARI issues in recently added riscv code, the ARI email is: >> > >> > https://sourceware.org/ml/gdb-patches/2018-03/msg00156.html >> >> Thanks for the patch, Andrew. Comment below. >> >> > 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 regnum) >> > { >> > static char buf[20]; >> > >> > - sprintf (buf, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); >> > + xsnprintf (buf, 20, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); >> > return buf; >> > } >> > >> > @@ -1049,12 +1049,10 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch, >> > >> > /* If we need more, grab it now. */ >> > instlen = riscv_insn_length (buf[0]); >> > + gdb_assert (instlen <= sizeof (buf)); >> > *len = 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 = 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; >> > >> > default: >> > - error ("unknown argument location type"); >> > + gdb_assert_not_reached ("unknown argument location type"); >> >> Strings must be marked for localization, with _(): >> >> gdb_assert_not_reached (_("unknown argument location type")); >> >> 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’t 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? Oh, I guess I stand corrected, then. > I'll fix the internationalisation as requested. Thanks,
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 regnum) { static char buf[20]; - sprintf (buf, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); + xsnprintf (buf, 20, "csr%d", regnum - RISCV_FIRST_CSR_REGNUM); return buf; } @@ -1049,12 +1049,10 @@ riscv_insn::fetch_instruction (struct gdbarch *gdbarch, /* If we need more, grab it now. */ instlen = riscv_insn_length (buf[0]); + gdb_assert (instlen <= sizeof (buf)); *len = 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 = 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; default: - error ("unknown argument location type"); + gdb_assert_not_reached ("unknown argument location type"); } } @@ -2149,7 +2147,7 @@ riscv_push_dummy_call (struct gdbarch *gdbarch, break; default: - error ("unknown argument location type"); + gdb_assert_not_reached ("unknown argument location type"); } if (second_arg_length > 0)