[1/3] gdb: Added builtin types for 24 bit integers.
Commit Message
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
Comments
On Fri, Aug 24, 2018 at 11:09:40AM -0400, Simon Marchi wrote:
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?
Yes, I see no reason why that shouldn't work. Like you say, C99 et. al.
doesn't require uint24_t but doesn't prohibit it either. I just
thought, that it makes sense to keep all these type definitions in the
same place so as to avoid the next 24 bit arch having to copy this
definition. But I don't have a strong opinion either way.
J'
On 2018-08-24 11:29, John Darrington wrote:
> On Fri, Aug 24, 2018 at 11:09:40AM -0400, Simon Marchi wrote:
>
> 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?
>
>
> Yes, I see no reason why that shouldn't work. Like you say, C99 et.
> al.
> doesn't require uint24_t but doesn't prohibit it either. I just
> thought, that it makes sense to keep all these type definitions in the
> same place so as to avoid the next 24 bit arch having to copy this
> definition. But I don't have a strong opinion either way.
I see, then that's fine with mean to have it in gdbarch.
Simon
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(-)
@@ -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
@@ -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;
@@ -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