Message ID | 20181112091721.28040-1-john@darrington.wattle.id.au |
---|---|
State | New, archived |
Headers |
Received: (qmail 94128 invoked by alias); 12 Nov 2018 09:18:07 -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 94108 invoked by uid 89); 12 Nov 2018 09:18:06 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.2 spammy=H*MI:john X-HELO: jocasta.intra Received: from de.cellform.com (HELO jocasta.intra) (88.217.224.109) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 12 Nov 2018 09:18:06 +0000 Received: from jocasta.intra (localhost [127.0.0.1]) by jocasta.intra (8.15.2/8.15.2/Debian-8) with ESMTPS id wAC9I3j4028092 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Nov 2018 10:18:03 +0100 Received: (from john@localhost) by jocasta.intra (8.15.2/8.15.2/Submit) id wAC9I3u2028091; Mon, 12 Nov 2018 10:18:03 +0100 From: John Darrington <john@darrington.wattle.id.au> To: gdb-patches@sourceware.org Cc: John Darrington <john@darrington.wattle.id.au> Subject: [PATCH 1/2] GDB: S12Z: Add assertion Date: Mon, 12 Nov 2018 10:17:20 +0100 Message-Id: <20181112091721.28040-1-john@darrington.wattle.id.au> |
Commit Message
John Darrington
Nov. 12, 2018, 9:17 a.m. UTC
--- gdb/s12z-tdep.c | 1 + 1 file changed, 1 insertion(+)
Comments
On Mon, 12 Nov 2018 10:17:20 +0100 John Darrington <john@darrington.wattle.id.au> wrote: > --- > gdb/s12z-tdep.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c > index 79f5772035..bd0bd7c001 100644 > --- a/gdb/s12z-tdep.c > +++ b/gdb/s12z-tdep.c > @@ -320,6 +320,7 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache) > } > else > { > + gdb_assert (this_sp == this_sp_for_id); > /* The stack pointer of the prev frame is frame_size greater > than the stack pointer of this frame plus one address > size (caused by the JSR or BSR). */ > -- Could you please add a ChangeLog entry? Also, on the same topic, what happened to the ChangeLog entries for the initial commit / push of the s12z port? (I don't see them in the ChangeLog file.) As for adding the assert, I suppose that you're just reminding the reader of this earlier assignment? ... this_sp_for_id = this_sp; (I didn't see anything which might change either this_sp or this_sp_for_id along the code path leading to the assert.) Kevin
On Sat, Nov 17, 2018 at 02:59:28PM -0700, Kevin Buettner wrote: > --- > gdb/s12z-tdep.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c > index 79f5772035..bd0bd7c001 100644 > --- a/gdb/s12z-tdep.c > +++ b/gdb/s12z-tdep.c > @@ -320,6 +320,7 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache) > } > else > { > + gdb_assert (this_sp == this_sp_for_id); > /* The stack pointer of the prev frame is frame_size greater > than the stack pointer of this frame plus one address > size (caused by the JSR or BSR). */ > -- Could you please add a ChangeLog entry? Also, on the same topic, what happened to the ChangeLog entries for the initial commit / push of the s12z port? (I don't see them in the ChangeLog file.) Is it really necessary in 2018 to have ChangeLogs? After all, the same information is available by typing git log and/or git diff. If the answer is "yes", then how about generating it automatically using the git-log-to-changelog script which is available in gnulib ? Many other projects have gone over to this method. ChangeLogs are a dreaded nuisance. They are almost guaranteed to cause merge conflicts. The information they contain is redundant. Their formatting requirements are onerous. And in thirty years of software development, I don't think I've ever experienced a case where I've needed to refer to one. As for adding the assert, I suppose that you're just reminding the reader of this earlier assignment? ... this_sp_for_id = this_sp; Indeed. (I didn't see anything which might change either this_sp or this_sp_for_id along the code path leading to the assert.) I have some plans for future changes in this part of the code. J'
On Sun, 18 Nov 2018 10:22:58 +0100 John Darrington <john@darrington.wattle.id.au> wrote: > On Sat, Nov 17, 2018 at 02:59:28PM -0700, Kevin Buettner wrote: > > > --- > > gdb/s12z-tdep.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c > > index 79f5772035..bd0bd7c001 100644 > > --- a/gdb/s12z-tdep.c > > +++ b/gdb/s12z-tdep.c > > @@ -320,6 +320,7 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache) > > } > > else > > { > > + gdb_assert (this_sp == this_sp_for_id); > > /* The stack pointer of the prev frame is frame_size greater > > than the stack pointer of this frame plus one address > > size (caused by the JSR or BSR). */ > > -- > > Could you please add a ChangeLog entry? > > Also, on the same topic, what happened to the ChangeLog entries > for the initial commit / push of the s12z port? (I don't see them > in the ChangeLog file.) > > Is it really necessary in 2018 to have ChangeLogs? After all, the same > information is available by typing git log and/or git diff. If the answer is > "yes", then how about generating it automatically using the git-log-to-changelog > script which is available in gnulib ? Many other projects have gone over to > this method. > > ChangeLogs are a dreaded nuisance. They are almost guaranteed to cause merge > conflicts. The information they contain is redundant. Their formatting > requirements are onerous. And in thirty years of software development, I don't > think I've ever experienced a case where I've needed to refer to one. While I agree with some of your arguments against the use of ChangeLogs in 2018, the GDB project still requires the use of ChangeLogs. See: https://sourceware.org/gdb/wiki/ContributionChecklist#ChangeLog and: https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog And, FWIW, the first thing that I did while reviewing your patches was to look at the current ChangeLog to get a brief history of changes to s12z-tdep.c. I was surprised when I did not find any ChangeLog entries, though I did figure it out by consulting the git log. Kevin
Hey John, To add to what Kevin said, we've been discussing this topic many times. I am pretty sure most people would be very happy to be getting rid of ChangeLog entries. I think the main issue, now, is to automate their generation when producing the source packages and releases; this is because we are a GNU project, and we are expected to allow people without the git repository to have a sense of the changes that were made. I think that if we have a process in place that automates this, we should be OK to replace the ChangeLog files by data extracted from the revision logs. What we need, at this point, is someone sufficiently motivated to put in place this automatic generation. We might also need a bit of development work on the hooks to validate the revision log's format and confirm that it always contains a ChangeLog entry in a format we can understand so we can always extract it (I can take care of that part once we have an agreement on the process).
diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c index 79f5772035..bd0bd7c001 100644 --- a/gdb/s12z-tdep.c +++ b/gdb/s12z-tdep.c @@ -320,6 +320,7 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache) } else { + gdb_assert (this_sp == this_sp_for_id); /* The stack pointer of the prev frame is frame_size greater than the stack pointer of this frame plus one address size (caused by the JSR or BSR). */