[PATCH 1/2] GDB: S12Z: Add assertion

[PATCH 1/2] GDB: S12Z: Add assertion

[John Darrington]

---
 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).  */
-- 
2.11.0

[PATCH 2/2] GDB: S12Z: new function s12z_extract_return_value

[John Darrington]

Make gdb aware of the return values of functions which
return in registers.

gdb/ChangeLog:
* s12z-tdep.c (s12z_extract_return_value): New function.
  (inv_reg_perm) New array.
  (s12z_return_value): Populate readbuf if non-null.
---
 gdb/s12z-tdep.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 1 deletion(-)

diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
index bd0bd7c001..3da88523f7 100644
--- a/gdb/s12z-tdep.c
+++ b/gdb/s12z-tdep.c
@@ -37,7 +37,9 @@
 #define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2)
 
 
-/*  A permutation of all the physical registers.  */
+/*  A permutation of all the physical registers.   Indexing this array
+    with an integer from gdb's internal representation will return the
+    register enum.  */
 static const int reg_perm[N_PHYSICAL_REGISTERS] =
   {
    REG_D0,
@@ -55,6 +57,16 @@ static const int reg_perm[N_PHYSICAL_REGISTERS] =
    REG_CCW
   };
 
+/*  The inverse of the above permutation.  Indexing this
+    array with a register enum (e.g. REG_D2) will return the register
+    number in gdb's internal representation.  */
+static const int inv_reg_perm[N_PHYSICAL_REGISTERS] =
+  {
+   2, 3, 4, 5,      /* d2, d3, d4, d5 */
+   0, 1,            /* d0, d1 */
+   6, 7,            /* d6, d7 */
+   8, 9, 10, 11, 12 /* x, y, s, p, ccw */
+  };
 
 /*  Return the name of the register REGNUM.  */
 static const char *
@@ -467,11 +479,59 @@ s12z_print_registers_info (struct gdbarch *gdbarch,
 
 \f
 
+
+static void
+s12z_extract_return_value (struct type *type, struct regcache *regcache,
+                              void *valbuf)
+{
+  int reg = -1;
+
+  gdb_byte buf[4];
+
+  switch (TYPE_LENGTH (type))
+    {
+    case 0:   /* Nothing to do */
+      return;
+
+    case 1:
+      reg = REG_D0;
+      break;
+
+    case 2:
+      reg = REG_D2;
+      break;
+
+    case 3:
+      reg = REG_X;
+      break;
+
+    case 4:
+      reg = REG_D6;
+      break;
+
+    default:
+      error (_("bad size for return value"));
+      return;
+    }
+
+  regcache->cooked_read (inv_reg_perm[reg], buf);
+  memcpy (valbuf, buf, TYPE_LENGTH (type));
+}
+
 static enum return_value_convention
 s12z_return_value (struct gdbarch *gdbarch, struct value *function,
                    struct type *type, struct regcache *regcache,
                    gdb_byte *readbuf, const gdb_byte *writebuf)
 {
+  if (TYPE_CODE (type) == TYPE_CODE_STRUCT
+      || TYPE_CODE (type) == TYPE_CODE_UNION
+      || TYPE_CODE (type) == TYPE_CODE_ARRAY
+      || TYPE_LENGTH (type) > 4)
+    return RETURN_VALUE_STRUCT_CONVENTION;
+
+  if (readbuf)
+    s12z_extract_return_value (type, regcache, readbuf);
+
   return RETURN_VALUE_REGISTER_CONVENTION;
 }
 
-- 
2.11.0

Re: [PATCH 2/2] GDB: S12Z: new function s12z_extract_return_value

[Kevin Buettner]

Hi John,

See my comments inline with your patch, below.

On Mon, 12 Nov 2018 10:17:21 +0100
John Darrington <john@darrington.wattle.id.au> wrote:

> Make gdb aware of the return values of functions which
> return in registers.
> 
> gdb/ChangeLog:
> * s12z-tdep.c (s12z_extract_return_value): New function.
>   (inv_reg_perm) New array.
>   (s12z_return_value): Populate readbuf if non-null.

Make sure that this is indented correctly when it eventually goes
in the ChangeLog file.

> diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
> index bd0bd7c001..3da88523f7 100644
> --- a/gdb/s12z-tdep.c
> +++ b/gdb/s12z-tdep.c
> @@ -37,7 +37,9 @@
>  #define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2)
>  
>  
> -/*  A permutation of all the physical registers.  */
> +/*  A permutation of all the physical registers.   Indexing this array
> +    with an integer from gdb's internal representation will return the
> +    register enum.  */
>  static const int reg_perm[N_PHYSICAL_REGISTERS] =
>    {
>     REG_D0,
> @@ -55,6 +57,16 @@ static const int reg_perm[N_PHYSICAL_REGISTERS] =
>     REG_CCW
>    };
>  
> +/*  The inverse of the above permutation.  Indexing this
> +    array with a register enum (e.g. REG_D2) will return the register
> +    number in gdb's internal representation.  */
> +static const int inv_reg_perm[N_PHYSICAL_REGISTERS] =
> +  {
> +   2, 3, 4, 5,      /* d2, d3, d4, d5 */
> +   0, 1,            /* d0, d1 */
> +   6, 7,            /* d6, d7 */
> +   8, 9, 10, 11, 12 /* x, y, s, p, ccw */
> +  };

My two cents on all of the above...

I think you'll have a lot less grief with this architecture port if
you don't try to use the numbering defined in include/opcode/s12z.h. 
Create a new numbering with new constants for GDB's purposes ordered
as shown in the reg_perm array.  Then use these constants in place of
the various REG_ constants that are currently in s12z-tdep.c.  If you
still want to be able to access registers[], it may make sense to
have an array which maps GDB's constants to those in include/opcode.

Also, if you want CCH and CCL to be show in "info registers" and/or
allow the user to display and set them, these can be implemented via
the use of pseudo-registers.

>  /*  Return the name of the register REGNUM.  */
>  static const char *
> @@ -467,11 +479,59 @@ s12z_print_registers_info (struct gdbarch *gdbarch,
>  
>  \f
>  
> +
> +static void
> +s12z_extract_return_value (struct type *type, struct regcache *regcache,
> +                              void *valbuf)
> +{
> +  int reg = -1;
> +
> +  gdb_byte buf[4];
> +
> +  switch (TYPE_LENGTH (type))
> +    {
> +    case 0:   /* Nothing to do */
> +      return;
> +
> +    case 1:
> +      reg = REG_D0;
> +      break;
> +
> +    case 2:
> +      reg = REG_D2;
> +      break;
> +
> +    case 3:
> +      reg = REG_X;
> +      break;
> +
> +    case 4:
> +      reg = REG_D6;
> +      break;
> +
> +    default:
> +      error (_("bad size for return value"));
> +      return;
> +    }
> +
> +  regcache->cooked_read (inv_reg_perm[reg], buf);
> +  memcpy (valbuf, buf, TYPE_LENGTH (type));

Is there any reason not to just pass valbuf in place of buf to
cooked_read?  Doing so will get rid of the memcpy.

Kevin

Re: [PATCH 2/2] GDB: S12Z: new function s12z_extract_return_value

[John Darrington]

Hi Kevin,

Thanks for the review.

On Sat, Nov 17, 2018 at 11:35:50PM -0700, Kevin Buettner wrote:
     > 
     > gdb/ChangeLog:
     > * s12z-tdep.c (s12z_extract_return_value): New function.
     >   (inv_reg_perm) New array.
     >   (s12z_return_value): Populate readbuf if non-null.
     
     Make sure that this is indented correctly when it eventually goes
     in the ChangeLog file.

Is it documented anywhere what "correctly" means?
     
     My two cents on all of the above...
     
     I think you'll have a lot less grief with this architecture port if
     you don't try to use the numbering defined in include/opcode/s12z.h. 
     Create a new numbering with new constants for GDB's purposes ordered
     as shown in the reg_perm array.  Then use these constants in place of
     the various REG_ constants that are currently in s12z-tdep.c.  If you
     still want to be able to access registers[], it may make sense to
     have an array which maps GDB's constants to those in include/opcode.

I'm beginning to think that you're right here.  I may change it in the
way you propose in a future patch.
     
     Also, if you want CCH and CCL to be show in "info registers" and/or
     allow the user to display and set them, these can be implemented via
     the use of pseudo-registers.

A the moment, I don't think it's worth bothering about.  CCH and CCL are
merely the high and low bytes of a 16bit register CCW.   The names CCH
and CCL only exist because of two (rarely used) instructions in the ISA.
     
     >  /*  Return the name of the register REGNUM.  */
     >  static const char *
     > @@ -467,11 +479,59 @@ s12z_print_registers_info (struct gdbarch *gdbarch,
     >  
     >  \f
     >  
     > +
     > +static void
     > +s12z_extract_return_value (struct type *type, struct regcache *regcache,
     > +                              void *valbuf)
     > +{
     > +  int reg = -1;
     > +
     > +  gdb_byte buf[4];
     > +
     > +  switch (TYPE_LENGTH (type))
     > +    {
     > +    case 0:   /* Nothing to do */
     > +      return;
     > +
     > +    case 1:
     > +      reg = REG_D0;
     > +      break;
     > +
     > +    case 2:
     > +      reg = REG_D2;
     > +      break;
     > +
     > +    case 3:
     > +      reg = REG_X;
     > +      break;
     > +
     > +    case 4:
     > +      reg = REG_D6;
     > +      break;
     > +
     > +    default:
     > +      error (_("bad size for return value"));
     > +      return;
     > +    }
     > +
     > +  regcache->cooked_read (inv_reg_perm[reg], buf);
     > +  memcpy (valbuf, buf, TYPE_LENGTH (type));
     
     Is there any reason not to just pass valbuf in place of buf to
     cooked_read?  Doing so will get rid of the memcpy.

Probably not.  Thanks for noticing this.
     
J'

-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH 2/2] GDB: S12Z: new function s12z_extract_return_value

[Kevin Buettner]

On Sun, 18 Nov 2018 10:13:37 +0100
John Darrington <john@darrington.wattle.id.au> wrote:

> Hi Kevin,
> 
> Thanks for the review.
> 
> On Sat, Nov 17, 2018 at 11:35:50PM -0700, Kevin Buettner wrote:
>      > 
>      > gdb/ChangeLog:
>      > * s12z-tdep.c (s12z_extract_return_value): New function.
>      >   (inv_reg_perm) New array.
>      >   (s12z_return_value): Populate readbuf if non-null.  
>      
>      Make sure that this is indented correctly when it eventually goes
>      in the ChangeLog file.
> 
> Is it documented anywhere what "correctly" means?

https://www.gnu.org/prep/standards/standards.html#Change-Logs
https://www.gnu.org/software/emacs/manual/html_node/emacs/Format-of-ChangeLog.html

Though with regards to indentation, neither of those links adequately
describe the ChangeLog entries that we've been using for many years
now.

When you do the commit / push, your ChangeLog entry should look something
like this:

YYYY-MM-DD  John Darrington  <john@darrington.wattle.id.au>

	* s12z-tdep.c (s12z_extract_return_value): New function.
	(inv_reg_perm) New array.
	(s12z_return_value): Populate readbuf if non-null.  

In particular, a single tab precedes the non-header (and non-empty)
portions of the ChangeLog entry.

Kevin

[PATCH] GDB: S12Z: new function s12z_extract_return_value

[John Darrington]

Make gdb aware of the return values of functions which
return in registers.

gdb/ChangeLog:
* s12z-tdep.c (s12z_extract_return_value): New function.
  (inv_reg_perm) New array.
  (s12z_return_value): Populate readbuf if non-null.
---
 gdb/s12z-tdep.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
index bd0bd7c001..b358b2fbe9 100644
--- a/gdb/s12z-tdep.c
+++ b/gdb/s12z-tdep.c
@@ -37,7 +37,9 @@
 #define N_PHYSICAL_REGISTERS (S12Z_N_REGISTERS - 2)
 
 
-/*  A permutation of all the physical registers.  */
+/*  A permutation of all the physical registers.   Indexing this array
+    with an integer from gdb's internal representation will return the
+    register enum.  */
 static const int reg_perm[N_PHYSICAL_REGISTERS] =
   {
    REG_D0,
@@ -55,6 +57,16 @@ static const int reg_perm[N_PHYSICAL_REGISTERS] =
    REG_CCW
   };
 
+/*  The inverse of the above permutation.  Indexing this
+    array with a register enum (e.g. REG_D2) will return the register
+    number in gdb's internal representation.  */
+static const int inv_reg_perm[N_PHYSICAL_REGISTERS] =
+  {
+   2, 3, 4, 5,      /* d2, d3, d4, d5 */
+   0, 1,            /* d0, d1 */
+   6, 7,            /* d6, d7 */
+   8, 9, 10, 11, 12 /* x, y, s, p, ccw */
+  };
 
 /*  Return the name of the register REGNUM.  */
 static const char *
@@ -467,11 +479,56 @@ s12z_print_registers_info (struct gdbarch *gdbarch,
 
 \f
 
+
+static void
+s12z_extract_return_value (struct type *type, struct regcache *regcache,
+                              void *valbuf)
+{
+  int reg = -1;
+
+  switch (TYPE_LENGTH (type))
+    {
+    case 0:   /* Nothing to do */
+      return;
+
+    case 1:
+      reg = REG_D0;
+      break;
+
+    case 2:
+      reg = REG_D2;
+      break;
+
+    case 3:
+      reg = REG_X;
+      break;
+
+    case 4:
+      reg = REG_D6;
+      break;
+
+    default:
+      error (_("bad size for return value"));
+      return;
+    }
+
+  regcache->cooked_read (inv_reg_perm[reg], (gdb_byte *) valbuf);
+}
+
 static enum return_value_convention
 s12z_return_value (struct gdbarch *gdbarch, struct value *function,
                    struct type *type, struct regcache *regcache,
                    gdb_byte *readbuf, const gdb_byte *writebuf)
 {
+  if (TYPE_CODE (type) == TYPE_CODE_STRUCT
+      || TYPE_CODE (type) == TYPE_CODE_UNION
+      || TYPE_CODE (type) == TYPE_CODE_ARRAY
+      || TYPE_LENGTH (type) > 4)
+    return RETURN_VALUE_STRUCT_CONVENTION;
+
+  if (readbuf)
+    s12z_extract_return_value (type, regcache, readbuf);
+
   return RETURN_VALUE_REGISTER_CONVENTION;
 }
 
-- 
2.11.0

Re: [PATCH] GDB: S12Z: new function s12z_extract_return_value

[Kevin Buettner]

On Tue, 20 Nov 2018 08:56:26 +0100
John Darrington <john@darrington.wattle.id.au> wrote:

> Make gdb aware of the return values of functions which
> return in registers.
> 
> gdb/ChangeLog:
> * s12z-tdep.c (s12z_extract_return_value): New function.
>   (inv_reg_perm) New array.
>   (s12z_return_value): Populate readbuf if non-null.

Okay.  (But make sure that the leading spaces get changed to a single
tab when you place your ChangeLog entry in the ChangeLog file.)

Kevin

Re: [PATCH] GDB: S12Z: new function s12z_extract_return_value

[John Darrington]

On Tue, Nov 20, 2018 at 09:40:31AM -0700, Kevin Buettner wrote:
     On Tue, 20 Nov 2018 08:56:26 +0100
     John Darrington <john@darrington.wattle.id.au> wrote:
     
     > Make gdb aware of the return values of functions which
     > return in registers.
     > 
     > gdb/ChangeLog:
     > * s12z-tdep.c (s12z_extract_return_value): New function.
     >   (inv_reg_perm) New array.
     >   (s12z_return_value): Populate readbuf if non-null.
     
     Okay.  (But make sure that the leading spaces get changed to a single
     tab when you place your ChangeLog entry in the ChangeLog file.)
     

Thanks.  I will do that.

(If I remember rightly however, the link you posted earlier explicitly
stated that the leading whitespace should be 8 spaces.  But as all other
entries have used tabs it seems best to be consistent.)


J'

Re: [PATCH 1/2] GDB: S12Z: Add assertion

[Kevin Buettner]

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

Re: [PATCH 1/2] GDB: S12Z: Add assertion

[John Darrington]

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'
     
-- 
Avoid eavesdropping.  Send strong encrypted email.
PGP Public key ID: 1024D/2DE827B3 
fingerprint = 8797 A26D 0854 2EAB 0285  A290 8A67 719C 2DE8 27B3
See http://sks-keyservers.net or any PGP keyserver for public key.

Re: [PATCH 1/2] GDB: S12Z: Add assertion

[Kevin Buettner]

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

Re: [PATCH 1/2] GDB: S12Z: Add assertion

[Joel Brobecker]

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).

-- 
Joel

[PATCH] GDB: S12Z: Add assertion

[John Darrington]

gdb/ChangeLog:

	* s12z-tdep.c (s12z_frame_cache): Add an assertion.
---
 gdb/ChangeLog   | 8 ++++++++
 gdb/s12z-tdep.c | 1 +
 2 files changed, 9 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4fa0b42657..990cfc3f9d 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2018-11-19  John Darrington <john@darrington.wattle.id.au>
+
+	*s12z-tdep.c (s12z_frame_cache): Add an assertion.
+
 2018-11-18  Tom Tromey  <tom@tromey.com>
 
 	PR build/23814:
@@ -216,6 +220,10 @@
 	frame cache, leave the frame id as the default, which is the outer
 	frame id.
 
+2018-11-14  John Darrington <john@darrington.wattle.id.au>
+
+	* s12z-tdep.c (s12z_frame_cache): Add an assertion.
+
 2018-11-07  Joel Brobecker  <brobecker@adacore.com>
 
 	* ada-lang.c (read_atcb): Only set task_info->called_task if
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).  */
-- 
2.11.0

Re: [PATCH] GDB: S12Z: Add assertion

[Kevin Buettner]

Hi John,

This is okay.  Go ahead and push it.

You leave the diff for ChangeLog out of your patches.  Just make
sure that the ChangeLog entries are placed at the top of the ChangeLog
file with  suitable header (date, name, and email) at the time that the
change is pushed.

Kevin

On Mon, 19 Nov 2018 17:47:34 +0100
John Darrington <john@darrington.wattle.id.au> wrote:

> gdb/ChangeLog:
> 
> 	* s12z-tdep.c (s12z_frame_cache): Add an assertion.
> ---
>  gdb/ChangeLog   | 8 ++++++++
>  gdb/s12z-tdep.c | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index 4fa0b42657..990cfc3f9d 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,7 @@
> +2018-11-19  John Darrington <john@darrington.wattle.id.au>
> +
> +	*s12z-tdep.c (s12z_frame_cache): Add an assertion.
> +
>  2018-11-18  Tom Tromey  <tom@tromey.com>
>  
>  	PR build/23814:
> @@ -216,6 +220,10 @@
>  	frame cache, leave the frame id as the default, which is the outer
>  	frame id.
>  
> +2018-11-14  John Darrington <john@darrington.wattle.id.au>
> +
> +	* s12z-tdep.c (s12z_frame_cache): Add an assertion.
> +
>  2018-11-07  Joel Brobecker  <brobecker@adacore.com>
>  
>  	* ada-lang.c (read_atcb): Only set task_info->called_task if
> 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).  */
> -- 
> 2.11.0
>