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

Discussion:

John Darrington

2018-11-12 09:17:20 UTC

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

John Darrington

2018-11-12 09:17:21 UTC

Permalink

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,

+
+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

Kevin Buettner

2018-11-18 06:35:50 UTC

Permalink

Hi John,

See my comments inline with your patch, below.

On Mon, 12 Nov 2018 10:17:21 +0100

Post by John Darrington
Make gdb aware of the return values of functions which
return in registers.
* 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.

Post by John Darrington
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.

Post by John Darrington
/* Return the name of the register REGNUM. */
static const char *
@@ -467,11 +479,59 @@ s12z_print_registers_info (struct gdbarch *gdbarch,
+
+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;
+
+ reg = REG_D0;
+ break;
+
+ reg = REG_D2;
+ break;
+
+ reg = REG_X;
+ break;
+
+ reg = REG_D6;
+ break;
+
+ 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

John Darrington

2018-11-18 09:13:37 UTC

Permalink

Hi Kevin,

Thanks for the review.

Post by John Darrington
* 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.

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.

Kevin Buettner

2018-11-18 18:28:46 UTC

Permalink

On Sun, 18 Nov 2018 10:13:37 +0100

Post by John Darrington
Hi Kevin,
Thanks for the review.

Post by John Darrington
* 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 <***@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

John Darrington

2018-11-20 07:56:26 UTC

Permalink

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,

+
+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

Kevin Buettner

2018-11-20 16:40:31 UTC

Permalink

On Tue, 20 Nov 2018 08:56:26 +0100

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

John Darrington

2018-11-20 17:01:25 UTC

Permalink

On Tue, Nov 20, 2018 at 09:40:31AM -0700, Kevin Buettner wrote:
On Tue, 20 Nov 2018 08:56:26 +0100

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'

John Darrington

2018-11-15 19:02:25 UTC

Permalink

--
2.11.0

Kevin Buettner

2018-11-17 21:59:28 UTC

Permalink

On Mon, 12 Nov 2018 10:17:20 +0100

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

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

John Darrington

2018-11-18 09:22:58 UTC

Permalink

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'

Kevin Buettner

2018-11-18 18:42:42 UTC

Permalink

On Sun, 18 Nov 2018 10:22:58 +0100

Post by Kevin Buettner

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

Joel Brobecker

2018-11-19 16:09:41 UTC

Permalink

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

John Darrington

2018-11-19 16:47:34 UTC

Permalink

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 <***@darrington.wattle.id.au>
+
+ *s12z-tdep.c (s12z_frame_cache): Add an assertion.
+
2018-11-18 Tom Tromey <***@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 <***@darrington.wattle.id.au>
+
+ * s12z-tdep.c (s12z_frame_cache): Add an assertion.
+
2018-11-07 Joel 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

Kevin Buettner

2018-11-20 05:16:34 UTC

Permalink

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

Post by John Darrington
* 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 @@
+
+ *s12z-tdep.c (s12z_frame_cache): Add an assertion.
+
@@ -216,6 +220,10 @@
frame cache, leave the frame id as the default, which is the outer
frame id.
+
+ * s12z-tdep.c (s12z_frame_cache): Add an assertion.
+
* 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