[PATCH] Return bytes for tdesc_register_size()

[PATCH] Return bytes for tdesc_register_size()

[Alan Hayward]

tdesc_register_size returns number of bits.
Change this to use bytes, the same as regcache::register_size, memcpy and sizeof.

This fixes a bug in aarch64_get_tdesc_vq which assumed bytes.

Update all other calls to tdesc_register_size.

I originally planned to fix aarch64_get_tdesc_vq and push as OBV.
However, this seemed the better fix.
Required for 8.2
Tested with make check on a target all build.
I don't have a rs6000 machine, however change is simple enough.

2018-07-10  Alan Hayward  <alan.hayward@arm.com>

	* target-descriptions.c (tdesc_register_size): Return bytes.
	* target-descriptions.h (tdesc_register_size): Update comment.
	* rs6000-tdep.c (rs6000_gdbarch_init): Assume bytes.
---
 gdb/rs6000-tdep.c         | 4 ++--
 gdb/target-descriptions.c | 4 ++--
 gdb/target-descriptions.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 4eeb62ac52..9801665723 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -5953,7 +5953,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       have_mq = tdesc_numbered_register (feature, tdesc_data, PPC_MQ_REGNUM,
 					 "mq");
 
-      tdesc_wordsize = tdesc_register_size (feature, "pc") / 8;
+      tdesc_wordsize = tdesc_register_size (feature, "pc");
       if (wordsize == -1)
 	wordsize = tdesc_wordsize;
 
@@ -5984,7 +5984,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 	  /* The fpscr register was expanded in isa 2.05 to 64 bits
 	     along with the addition of the decimal floating point
 	     facility.  */
-	  if (tdesc_register_size (feature, "fpscr") > 32)
+	  if (tdesc_register_size (feature, "fpscr") > 4)
 	    have_dfp = 1;
 	}
       else
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 3d7aa2582e..8c987fb88d 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -813,7 +813,7 @@ tdesc_numbered_register_choices (const struct tdesc_feature *feature,
 }
 
 /* Search FEATURE for a register named NAME, and return its size in
-   bits.  The register must exist.  */
+   bytes.  The register must exist.  */
 
 int
 tdesc_register_size (const struct tdesc_feature *feature,
@@ -822,7 +822,7 @@ tdesc_register_size (const struct tdesc_feature *feature,
   struct tdesc_reg *reg = tdesc_find_register_early (feature, name);
 
   gdb_assert (reg != NULL);
-  return reg->bitsize;
+  return reg->bitsize / 8;
 }
 
 /* Look up a register by its GDB internal register number.  */
diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h
index 3ba71b1add..4284f23fe0 100644
--- a/gdb/target-descriptions.h
+++ b/gdb/target-descriptions.h
@@ -123,7 +123,7 @@ int tdesc_unnumbered_register (const struct tdesc_feature *feature,
 			       const char *name);
 
 /* Search FEATURE for a register named NAME, and return its size in
-   bits.  The register must exist.  */
+   bytes.  The register must exist.  */
 
 int tdesc_register_size (const struct tdesc_feature *feature,
 			 const char *name);
-- 
2.15.2 (Apple Git-101.1)

Re: [PATCH] Return bytes for tdesc_register_size()

[Simon Marchi]

On 2018-07-10 11:05 AM, Alan Hayward wrote:
> tdesc_register_size returns number of bits.
> Change this to use bytes, the same as regcache::register_size, memcpy and sizeof.
> 
> This fixes a bug in aarch64_get_tdesc_vq which assumed bytes.
> 
> Update all other calls to tdesc_register_size.
> 
> I originally planned to fix aarch64_get_tdesc_vq and push as OBV.
> However, this seemed the better fix.
> Required for 8.2
> Tested with make check on a target all build.
> I don't have a rs6000 machine, however change is simple enough.

Hi Alan,

Since I work with processors that have 16-bit bytes, I always prefer
expressing sizes in bits, otherwise it's ambiguous.  Are we talking
about 8-bit bytes or target-sized bytes?  So I would have perhaps
chosen to rename tdesc_register_size to tdesc_register_size_bits
(for clarity) and fix up the caller.

Would you be ok with that?  Other than that, the patch looks fine.

Simon

Re: [PATCH] Return bytes for tdesc_register_size()

[Alan Hayward]

> On 10 Jul 2018, at 19:44, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> On 2018-07-10 11:05 AM, Alan Hayward wrote:
>> tdesc_register_size returns number of bits.
>> Change this to use bytes, the same as regcache::register_size, memcpy and sizeof.
>> 
>> This fixes a bug in aarch64_get_tdesc_vq which assumed bytes.
>> 
>> Update all other calls to tdesc_register_size.
>> 
>> I originally planned to fix aarch64_get_tdesc_vq and push as OBV.
>> However, this seemed the better fix.
>> Required for 8.2
>> Tested with make check on a target all build.
>> I don't have a rs6000 machine, however change is simple enough.
> 
> Hi Alan,
> 
> Since I work with processors that have 16-bit bytes, I always prefer
> expressing sizes in bits, otherwise it's ambiguous.  Are we talking
> about 8-bit bytes or target-sized bytes?  So I would have perhaps
> chosen to rename tdesc_register_size to tdesc_register_size_bits
> (for clarity) and fix up the caller.
> 
> Would you be ok with that?  Other than that, the patch looks fine.
> 

Right, that makes sense. This means regcache::register_size is at odds
with that

Happy with just renaming the function as it clears up the ambiguities.

Pushed the following as it's obvious:


diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 5120fffe776ada9fc5670ff4759c8e53378a8193..5c6eb985451c43909cc929b32eade09f778d0018 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2922,8 +2922,8 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
   if (feature_sve == nullptr)
     return 0;

-  uint64_t vl = tdesc_register_size (feature_sve,
-				     aarch64_sve_register_names[0]);
+  uint64_t vl = tdesc_register_bitsize (feature_sve,
+					aarch64_sve_register_names[0]) / 8;
   return sve_vq_from_vl (vl);
 }

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 4eeb62ac52fcfe0c0a3648e3e68b0bb23516c73f..45bf98267a99a1c8990f7b0b0f0788dd7b0e03d3 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -5953,7 +5953,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
       have_mq = tdesc_numbered_register (feature, tdesc_data, PPC_MQ_REGNUM,
 					 "mq");

-      tdesc_wordsize = tdesc_register_size (feature, "pc") / 8;
+      tdesc_wordsize = tdesc_register_bitsize (feature, "pc") / 8;
       if (wordsize == -1)
 	wordsize = tdesc_wordsize;

@@ -5984,7 +5984,7 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 	  /* The fpscr register was expanded in isa 2.05 to 64 bits
 	     along with the addition of the decimal floating point
 	     facility.  */
-	  if (tdesc_register_size (feature, "fpscr") > 32)
+	  if (tdesc_register_bitsize (feature, "fpscr") > 32)
 	    have_dfp = 1;
 	}
       else
diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h
index 3ba71b1add638de35c3d5bc5b6f7943a32169369..87403acc0d4d3dc7da4f2ca12c4778d5b9a5d353 100644
--- a/gdb/target-descriptions.h
+++ b/gdb/target-descriptions.h
@@ -125,8 +125,8 @@ int tdesc_unnumbered_register (const struct tdesc_feature *feature,
 /* Search FEATURE for a register named NAME, and return its size in
    bits.  The register must exist.  */

-int tdesc_register_size (const struct tdesc_feature *feature,
-			 const char *name);
+int tdesc_register_bitsize (const struct tdesc_feature *feature,
+			    const char *name);

 /* Search FEATURE for a register with any of the names from NAMES
    (NULL-terminated).  Record REGNO and the register in DATA; when
diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
index 3d7aa2582e72815f98e77681fdec15acfb54ec43..a96416cd3cc94faf213cf576e9eac31524baa3ab 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -816,8 +816,7 @@ tdesc_numbered_register_choices (const struct tdesc_feature *feature,
    bits.  The register must exist.  */

 int
-tdesc_register_size (const struct tdesc_feature *feature,
-		     const char *name)
+tdesc_register_bitsize (const struct tdesc_feature *feature, const char *name)
 {
   struct tdesc_reg *reg = tdesc_find_register_early (feature, name);