Re: [PATCH 1/3] gdb: Added builtin types for 24 bit integers.

[Simon Marchi <simon.marchi@polymtl.ca>]

[-- Attachment #1: Type: text/plain, Size: 2804 bytes --]

On 2018-08-24 02:11, John Darrington wrote:
> On Thu, Aug 23, 2018 at 04:35:25PM -0400, Simon Marchi wrote:
> 
>      It is clear now, but somebody doing a git blame to know why 24-bit
>      integer types were added would only find the patch that adds them 
> by
>      itself and wonder who uses that.  A little message like
> 
>        This patch adds 24-bit integer types, used when debugging on the 
> S12Z
>      architecture (added by a later patch in this series).
> 
>      clears that up.  That might looks a bit silly, but I think it 
> helps in
>      the long run.
> 
> I fully agree with you.   I've worked on other projects however had a
> different opinion - they insisted that the checkin comment NOT contain
> any rationale for the change, instead it should just summarize what
> changed.  I find that rather pointless but anyway ....

Well, if you look at our commit history, you'll see we like to be 
verbose :).

>      > It seems that up till now there has been no 24 bit targets, so 
> the
>      > other
>      > two patches as some necessary things to make that possible.
> 
>      Thanks.  Coming back to the code of the patch, I was wondering if 
> these
>      24-bit types are useful or even relevant for any other 
> architecture.
> 
> There most certainly are plenty of 24 bit architectures  especially in 
> the
> embedded world  - just apparently none that gdb currently supports :(
> 
>      Would it work if you only defined the types for s12z 
> architectures,
>      storing the reference in the gdbarch_tdep object?
> 
> My first reaction is that it probably *could* be made to work, but not
> in an elegant fashion.   Somehow I'd have to avoid that gdb ever calls 
> the
> read_encoded_value function.

I'm not sure I understand.  I was only talking about the definition of 
the int24_t and uint24_t types, not the handling of DW_EH_PE_udata3.  
 From what I read, the C99 standard mandates that the 8, 16, 32 and 64 
variants of the intX_t/uintX_t types exist.  Other types (with other 
values of X) would be extensions.  That's why I thought it would make 
sense to define that in the s12z-specific gdbarches only.  In the end I 
don't really mind, but it just looks like the "clean" way to do it and 
doesn't seem really more difficult.  Can you see if the attached diff 
(applied on top of your series) work for you?

And as far as I understand, this is disconnected from the handling of 
DW_EH_PE_udata3.

> I do concede that  adding DW_EH_PE_udata3 might be problematic since
> it's not part of the dwarf standard.  An alternative might be to rework
> the read_encoded_value function to not rely on the dwarf enums (all it
> really cares about is the size of the target's address space.

I'll take a look at that patch (2/3) separately and reply to it.

Simon

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Move-builtin_uint24_t-type-to-s12z-tdep.c.patch --]
[-- Type: text/x-diff; name=0001-Move-builtin_uint24_t-type-to-s12z-tdep.c.patch, Size: 2813 bytes --]

From e0902733a29726ba107c41980bdbd8500261c852 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 24 Aug 2018 11:03:45 -0400
Subject: [PATCH] Move builtin_uint24_t type to s12z-tdep.c

---
 gdb/gdbtypes.c  |  4 ----
 gdb/gdbtypes.h  |  2 --
 gdb/s12z-tdep.c | 16 +++++++++++-----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 05bf7b1..65b1211 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -5402,10 +5402,6 @@ gdbtypes_post_init (struct gdbarch *gdbarch)
     = arch_integer_type (gdbarch, 16, 0, "int16_t");
   builtin_type->builtin_uint16
     = arch_integer_type (gdbarch, 16, 1, "uint16_t");
-  builtin_type->builtin_int24
-    = arch_integer_type (gdbarch, 24, 0, "int24_t");
-  builtin_type->builtin_uint24
-    = arch_integer_type (gdbarch, 24, 1, "uint24_t");
   builtin_type->builtin_int32
     = arch_integer_type (gdbarch, 32, 0, "int32_t");
   builtin_type->builtin_uint32
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index eb7c365..14059ab 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -1611,8 +1611,6 @@ struct builtin_type
   struct type *builtin_uint8;
   struct type *builtin_int16;
   struct type *builtin_uint16;
-  struct type *builtin_int24;
-  struct type *builtin_uint24;
   struct type *builtin_int32;
   struct type *builtin_uint32;
   struct type *builtin_int64;
diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
index 5169025..48af422 100644
--- a/gdb/s12z-tdep.c
+++ b/gdb/s12z-tdep.c
@@ -76,6 +76,11 @@ static const int reg_perm[N_PHYSICAL_REGISTERS] =
    REG_CCW
   };
 
+struct gdbarch_tdep
+{
+  type *builtin_uint24;
+};
+
 static const char *
 s12z_register_name (struct gdbarch *gdbarch, int regnum)
 {
@@ -138,7 +143,10 @@ s12z_register_type (struct gdbarch *gdbarch, int reg_nr)
     case 2:
       return builtin_type (gdbarch)->builtin_uint16;
     case 3:
-      return builtin_type (gdbarch)->builtin_uint24;
+      {
+	struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+	return tdep->builtin_uint24;
+      }
     case 4:
       return builtin_type (gdbarch)->builtin_uint32;
     default:
@@ -347,10 +355,6 @@ constexpr gdb_byte s12z_break_insn[] = {0x00};
 
 typedef BP_MANIPULATION (s12z_break_insn) s12z_breakpoint;
 
-struct gdbarch_tdep
-{
-};
-
 static struct gdbarch *
 s12z_gdbarch_init (struct gdbarch_info info,
                     struct gdbarch_list *arches)
@@ -358,6 +362,8 @@ s12z_gdbarch_init (struct gdbarch_info info,
   struct gdbarch_tdep *tdep = (struct gdbarch_tdep *) xmalloc (sizeof *tdep);
   struct gdbarch *gdbarch = gdbarch_alloc (&info, tdep);
 
+  tdep->builtin_uint24 = arch_integer_type (gdbarch, 24, 1, "uint24_t");
+
   /* Target data types.  */
   set_gdbarch_short_bit (gdbarch, 16);
   set_gdbarch_int_bit (gdbarch, 16);
-- 
2.7.4