* [PATCH] Use visitors for make_gdb_type @ 2018-01-26 15:14 Alan Hayward 2018-01-26 15:30 ` Alan Hayward 0 siblings, 1 reply; 9+ messages in thread From: Alan Hayward @ 2018-01-26 15:14 UTC (permalink / raw) To: gdb-patches; +Cc: nd This patch implements the suggestion in the review for [PATCH v2 6/8] Create xml from target descriptions. Remove the make_gdb_type functions from the tdesc_type_ classes. Replace with a static make_gdb_type function that uses a element visitor called gdb_type_creator. I've defined gdb_type_creator inside make_gdb_type because it shouldn't be needed outside the function. The method of creating the types has not changed. This patch will allow a future patch to commonise the tdesc_types with gdbserver without having to move any gdb_type functionality. Alan. 2018-01-26 Alan Hayward <alan.hayward@arm.com>    * target-descriptions.c (tdesc_type): Move make_gdb_type from here.    (tdesc_type_builtin): Likewise.    (tdesc_type_vector): Likewise.    (tdesc_type_with_fields): Move make_gdb_type_ functions from here.    (make_gdb_type_struct): Move from tdesc_type_with_fields.    (make_gdb_type_union): Likewise.    (make_gdb_type_flags): Likewise.    (make_gdb_type_enum): Likewise.    (make_gdb_type): New function.    (tdesc_register_type): Use static make_gdb_type. ---  gdb/target-descriptions.c | 460 +++++++++++++++++++++++++---------------------  1 file changed, 254 insertions(+), 206 deletions(-) diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 1b20a12d76..55ac1d7a80 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -38,6 +38,8 @@  #include "completer.h"  #include "readline/tilde.h" /* tilde_expand */ +static type *make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype); +  /* The interface to visit different elements of target description. */  class tdesc_element_visitor @@ -223,11 +225,6 @@ struct tdesc_type : tdesc_element   {     return !(*this == other);   } - - /* Construct, if necessary, and return the GDB type implementing this -    target type for architecture GDBARCH. */ - - virtual type *make_gdb_type (struct gdbarch *gdbarch) const = 0;  };  typedef std::unique_ptr<tdesc_type> tdesc_type_up; @@ -242,81 +239,6 @@ struct tdesc_type_builtin : tdesc_type   {     v.visit (this);   } - - type *make_gdb_type (struct gdbarch *gdbarch) const override - { -   switch (this->kind) -     { -     /* Predefined types. */ -     case TDESC_TYPE_BOOL: -       return builtin_type (gdbarch)->builtin_bool; - -     case TDESC_TYPE_INT8: -       return builtin_type (gdbarch)->builtin_int8; - -     case TDESC_TYPE_INT16: -       return builtin_type (gdbarch)->builtin_int16; - -     case TDESC_TYPE_INT32: -       return builtin_type (gdbarch)->builtin_int32; - -     case TDESC_TYPE_INT64: -       return builtin_type (gdbarch)->builtin_int64; - -     case TDESC_TYPE_INT128: -       return builtin_type (gdbarch)->builtin_int128; - -     case TDESC_TYPE_UINT8: -       return builtin_type (gdbarch)->builtin_uint8; - -     case TDESC_TYPE_UINT16: -       return builtin_type (gdbarch)->builtin_uint16; - -     case TDESC_TYPE_UINT32: -       return builtin_type (gdbarch)->builtin_uint32; - -     case TDESC_TYPE_UINT64: -       return builtin_type (gdbarch)->builtin_uint64; - -     case TDESC_TYPE_UINT128: -       return builtin_type (gdbarch)->builtin_uint128; - -     case TDESC_TYPE_CODE_PTR: -       return builtin_type (gdbarch)->builtin_func_ptr; - -     case TDESC_TYPE_DATA_PTR: -       return builtin_type (gdbarch)->builtin_data_ptr; -     } - -   type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); -   if (gdb_type != NULL) -     return gdb_type; - -   switch (this->kind) -     { -     case TDESC_TYPE_IEEE_SINGLE: -       return arch_float_type (gdbarch, -1, "builtin_type_ieee_single", -            floatformats_ieee_single); - -     case TDESC_TYPE_IEEE_DOUBLE: -       return arch_float_type (gdbarch, -1, "builtin_type_ieee_double", -            floatformats_ieee_double); - -     case TDESC_TYPE_ARM_FPA_EXT: -       return arch_float_type (gdbarch, -1, "builtin_type_arm_ext", -            floatformats_arm_ext); - -     case TDESC_TYPE_I387_EXT: -       return arch_float_type (gdbarch, -1, "builtin_type_i387_ext", -            floatformats_i387_ext); -     } - -   internal_error (__FILE__, __LINE__, -         "Type \"%s\" has an unknown kind %d", -         this->name.c_str (), this->kind); - -   return NULL; - }  };  /* tdesc_type for vector types. */ @@ -333,19 +255,6 @@ struct tdesc_type_vector : tdesc_type     v.visit (this);   } - type *make_gdb_type (struct gdbarch *gdbarch) const override - { -   type *vector_gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); -   if (vector_gdb_type != NULL) -     return vector_gdb_type; - -   type *element_gdb_type = this->element_type->make_gdb_type (gdbarch); -   vector_gdb_type = init_vector_type (element_gdb_type, this->count); -   TYPE_NAME (vector_gdb_type) = xstrdup (this->name.c_str ()); - -   return vector_gdb_type; - } -   struct tdesc_type *element_type;   int count;  }; @@ -364,151 +273,290 @@ struct tdesc_type_with_fields : tdesc_type     v.visit (this);   } - type *make_gdb_type_struct (struct gdbarch *gdbarch) const - { -   type *struct_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); -   TYPE_NAME (struct_gdb_type) = xstrdup (this->name.c_str ()); -   TYPE_TAG_NAME (struct_gdb_type) = TYPE_NAME (struct_gdb_type); + std::vector<tdesc_type_field> fields; + int size; +}; -   for (const tdesc_type_field &f : this->fields) -     { -   if (f.start != -1 && f.end != -1) -    { -      /* Bitfield. */ -      struct field *fld; -      struct type *field_gdb_type; -      int bitsize, total_size; - -      /* This invariant should be preserved while creating types. */ -      gdb_assert (this->size != 0); -      if (f.type != NULL) -        field_gdb_type = f.type->make_gdb_type (gdbarch); -      else if (this->size > 4) -        field_gdb_type = builtin_type (gdbarch)->builtin_uint64; -      else -        field_gdb_type = builtin_type (gdbarch)->builtin_uint32; - -      fld = append_composite_type_field_raw -        (struct_gdb_type, xstrdup (f.name.c_str ()), field_gdb_type); - -      /* For little-endian, BITPOS counts from the LSB of -         the structure and marks the LSB of the field. For -         big-endian, BITPOS counts from the MSB of the -         structure and marks the MSB of the field. Either -         way, it is the number of bits to the "left" of the -         field. To calculate this in big-endian, we need -         the total size of the structure. */ -      bitsize = f.end - f.start + 1; -      total_size = this->size * TARGET_CHAR_BIT; -      if (gdbarch_bits_big_endian (gdbarch)) -        SET_FIELD_BITPOS (fld[0], total_size - f.start - bitsize); -      else -        SET_FIELD_BITPOS (fld[0], f.start); -      FIELD_BITSIZE (fld[0]) = bitsize; -    } -   else -    { -      gdb_assert (f.start == -1 && f.end == -1); -      type *field_gdb_type = f.type->make_gdb_type (gdbarch); -      append_composite_type_field (struct_gdb_type, -                xstrdup (f.name.c_str ()), -                field_gdb_type); -    } -     } +static type * +make_gdb_type_struct (struct gdbarch *gdbarch, const tdesc_type_with_fields *e) +{ + type *struct_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); + TYPE_NAME (struct_gdb_type) = xstrdup (e->name.c_str ()); + TYPE_TAG_NAME (struct_gdb_type) = TYPE_NAME (struct_gdb_type); -   if (this->size != 0) -     TYPE_LENGTH (struct_gdb_type) = this->size; + for (const tdesc_type_field &f : e->fields) +   { +     if (f.start != -1 && f.end != -1) +   { +    /* Bitfield. */ +    struct field *fld; +    struct type *field_gdb_type; +    int bitsize, total_size; + +    /* This invariant should be preserved while creating types. */ +    gdb_assert (e->size != 0); +    if (f.type != NULL) +      field_gdb_type = make_gdb_type (gdbarch, f.type); +    else if (e->size > 4) +      field_gdb_type = builtin_type (gdbarch)->builtin_uint64; +    else +      field_gdb_type = builtin_type (gdbarch)->builtin_uint32; + +    fld = append_composite_type_field_raw +      (struct_gdb_type, xstrdup (f.name.c_str ()), field_gdb_type); + +    /* For little-endian, BITPOS counts from the LSB of +       the structure and marks the LSB of the field. For +       big-endian, BITPOS counts from the MSB of the +       structure and marks the MSB of the field. Either +       way, it is the number of bits to the "left" of the +       field. To calculate this in big-endian, we need +       the total size of the structure. */ +    bitsize = f.end - f.start + 1; +    total_size = e->size * TARGET_CHAR_BIT; +    if (gdbarch_bits_big_endian (gdbarch)) +      SET_FIELD_BITPOS (fld[0], total_size - f.start - bitsize); +    else +      SET_FIELD_BITPOS (fld[0], f.start); +    FIELD_BITSIZE (fld[0]) = bitsize; +   } +     else +   { +    gdb_assert (f.start == -1 && f.end == -1); +    type *field_gdb_type = make_gdb_type (gdbarch, f.type); +    append_composite_type_field (struct_gdb_type, +                  xstrdup (f.name.c_str ()), +                  field_gdb_type); +   } +   } -   return struct_gdb_type; - } + if (e->size != 0) +   TYPE_LENGTH (struct_gdb_type) = e->size; - type *make_gdb_type_union (struct gdbarch *gdbarch) const - { -   type *union_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION); -   TYPE_NAME (union_gdb_type) = xstrdup (this->name.c_str ()); + return struct_gdb_type; +} -   for (const tdesc_type_field &f : this->fields) -     { -   type* field_gdb_type = f.type->make_gdb_type (gdbarch); -   append_composite_type_field (union_gdb_type, xstrdup (f.name.c_str ()), +static type * +make_gdb_type_union (struct gdbarch *gdbarch, const tdesc_type_with_fields *e) +{ + type *union_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION); + TYPE_NAME (union_gdb_type) = xstrdup (e->name.c_str ()); + + for (const tdesc_type_field &f : e->fields) +   { +     type* field_gdb_type = make_gdb_type (gdbarch, f.type); +     append_composite_type_field (union_gdb_type, xstrdup (f.name.c_str ()),                  field_gdb_type); -   /* If any of the children of a union are vectors, flag the -     union as a vector also. This allows e.g. a union of two -     vector types to show up automatically in "info vector". */ -   if (TYPE_VECTOR (field_gdb_type)) -    TYPE_VECTOR (union_gdb_type) = 1; -     } +     /* If any of the children of a union are vectors, flag the +    union as a vector also. This allows e.g. a union of two +    vector types to show up automatically in "info vector". */ +     if (TYPE_VECTOR (field_gdb_type)) +   TYPE_VECTOR (union_gdb_type) = 1; +   } -   return union_gdb_type; - } + return union_gdb_type; +} - type *make_gdb_type_flags (struct gdbarch *gdbarch) const - { -   type *flags_gdb_type = arch_flags_type (gdbarch, this->name.c_str (), -                this->size * TARGET_CHAR_BIT); +static type * +make_gdb_type_flags (struct gdbarch *gdbarch, const tdesc_type_with_fields *e) +{ + type *flags_gdb_type = arch_flags_type (gdbarch, e->name.c_str (), +                e->size * TARGET_CHAR_BIT); -   for (const tdesc_type_field &f : this->fields) -     { + for (const tdesc_type_field &f : e->fields) +   {       int bitsize = f.end - f.start + 1;       gdb_assert (f.type != NULL); -     type *field_gdb_type = f.type->make_gdb_type (gdbarch); +     type *field_gdb_type = make_gdb_type (gdbarch, f.type);       append_flags_type_field (flags_gdb_type, f.start, bitsize,                 field_gdb_type, f.name.c_str ()); -     } +   } -   return flags_gdb_type; - } + return flags_gdb_type; +} - type *make_gdb_type_enum (struct gdbarch *gdbarch) const - { -   type *enum_gdb_type = arch_type (gdbarch, TYPE_CODE_ENUM, -              this->size * TARGET_CHAR_BIT, -              this->name.c_str ()); +static type * +make_gdb_type_enum (struct gdbarch *gdbarch, const tdesc_type_with_fields *e) +{ + type *enum_gdb_type = arch_type (gdbarch, TYPE_CODE_ENUM, +              e->size * TARGET_CHAR_BIT, +              e->name.c_str ()); -   TYPE_UNSIGNED (enum_gdb_type) = 1; -   for (const tdesc_type_field &f : this->fields) -     { + TYPE_UNSIGNED (enum_gdb_type) = 1; + for (const tdesc_type_field &f : e->fields) +   {       struct field *fld     = append_composite_type_field_raw (enum_gdb_type,                   xstrdup (f.name.c_str ()),                   NULL);       SET_FIELD_BITPOS (fld[0], f.start); -     } +   } -   return enum_gdb_type; - } + return enum_gdb_type; +} - type *make_gdb_type (struct gdbarch *gdbarch) const override +static type * +make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype) +{ + class gdb_type_creator : public tdesc_element_visitor   { -   type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); -   if (gdb_type != NULL) -     return gdb_type; + public: +   gdb_type_creator (struct gdbarch *gdbarch) +     : m_gdbarch (gdbarch) +   {} -   switch (this->kind) +   type *get_type ()     { -     case TDESC_TYPE_STRUCT: -   return make_gdb_type_struct (gdbarch); -     case TDESC_TYPE_UNION: -   return make_gdb_type_union (gdbarch); -     case TDESC_TYPE_FLAGS: -   return make_gdb_type_flags (gdbarch); -     case TDESC_TYPE_ENUM: -   return make_gdb_type_enum (gdbarch); +     return m_type;     } -   internal_error (__FILE__, __LINE__, -         "Type \"%s\" has an unknown kind %d", -         this->name.c_str (), this->kind); +   void visit_pre (const target_desc *e) +   {} -   return NULL; - } +   void visit_post (const target_desc *e) +   {} - std::vector<tdesc_type_field> fields; - int size; -}; +   void visit_pre (const tdesc_feature *e) +   {} + +   void visit_post (const tdesc_feature *e) +   {} + +   void visit (const tdesc_type_builtin *e) override +   { +     switch (e->kind) +   { +    /* Predefined types. */ +   case TDESC_TYPE_BOOL: +    m_type = builtin_type (m_gdbarch)->builtin_bool; +    return; +   case TDESC_TYPE_INT8: +    m_type = builtin_type (m_gdbarch)->builtin_int8; +    return; +   case TDESC_TYPE_INT16: +    m_type = builtin_type (m_gdbarch)->builtin_int16; +    return; +   case TDESC_TYPE_INT32: +    m_type = builtin_type (m_gdbarch)->builtin_int32; +    return; +   case TDESC_TYPE_INT64: +    m_type = builtin_type (m_gdbarch)->builtin_int64; +    return; +   case TDESC_TYPE_INT128: +    m_type = builtin_type (m_gdbarch)->builtin_int128; +    return; +   case TDESC_TYPE_UINT8: +    m_type = builtin_type (m_gdbarch)->builtin_uint8; +    return; +   case TDESC_TYPE_UINT16: +    m_type = builtin_type (m_gdbarch)->builtin_uint16; +    return; +   case TDESC_TYPE_UINT32: +    m_type = builtin_type (m_gdbarch)->builtin_uint32; +    return; +   case TDESC_TYPE_UINT64: +    m_type = builtin_type (m_gdbarch)->builtin_uint64; +    return; +   case TDESC_TYPE_UINT128: +    m_type = builtin_type (m_gdbarch)->builtin_uint128; +    return; +   case TDESC_TYPE_CODE_PTR: +    m_type = builtin_type (m_gdbarch)->builtin_func_ptr; +    return; +   case TDESC_TYPE_DATA_PTR: +    m_type = builtin_type (m_gdbarch)->builtin_data_ptr; +    return; +   } + +     m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); +     if (m_type != NULL) +   return; + +     switch (e->kind) +   { +   case TDESC_TYPE_IEEE_SINGLE: +    m_type = arch_float_type (m_gdbarch, -1, "builtin_type_ieee_single", +               floatformats_ieee_single); +    return; + +   case TDESC_TYPE_IEEE_DOUBLE: +    m_type = arch_float_type (m_gdbarch, -1, "builtin_type_ieee_double", +               floatformats_ieee_double); +    return; +   case TDESC_TYPE_ARM_FPA_EXT: +    m_type = arch_float_type (m_gdbarch, -1, "builtin_type_arm_ext", +               floatformats_arm_ext); +    return; + +   case TDESC_TYPE_I387_EXT: +    m_type = arch_float_type (m_gdbarch, -1, "builtin_type_i387_ext", +               floatformats_i387_ext); +    return; +   } + +     internal_error (__FILE__, __LINE__, +           "Type \"%s\" has an unknown kind %d", +           e->name.c_str (), e->kind); +   } + +   void visit (const tdesc_type_vector *e) override +   { +     m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); +     if (m_type != NULL) +   return; + +     type *element_gdb_type = make_gdb_type (m_gdbarch, e->element_type); +     m_type = init_vector_type (element_gdb_type, e->count); +     TYPE_NAME (m_type) = xstrdup (e->name.c_str ()); +     return; +   } + +   void visit (const tdesc_type_with_fields *e) override +   { +     m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); +     if (m_type != NULL) +   return; + +     switch (e->kind) +   { +   case TDESC_TYPE_STRUCT: +    m_type = make_gdb_type_struct (m_gdbarch, e); +    return; +   case TDESC_TYPE_UNION: +    m_type = make_gdb_type_union (m_gdbarch, e); +    return; +   case TDESC_TYPE_FLAGS: +    m_type = make_gdb_type_flags (m_gdbarch, e); +    return; +   case TDESC_TYPE_ENUM: +    m_type = make_gdb_type_enum (m_gdbarch, e); +    return; +   } + +     internal_error (__FILE__, __LINE__, +           "Type \"%s\" has an unknown kind %d", +           e->name.c_str (), e->kind); +   } + +   void visit (const tdesc_reg *reg) +   {} + + private: + +   /* The gdbarch used. */ +   struct gdbarch *m_gdbarch; + +   /* The type created. */ +   type *m_type; + }; + + gdb_type_creator gdb_type (gdbarch); + ttype->accept (gdb_type); + return gdb_type.get_type (); +}  /* A feature from a target description. Each feature is a collection    of other elements, e.g. registers and types. */ @@ -1216,7 +1264,7 @@ tdesc_register_type (struct gdbarch *gdbarch, int regno)     {       /* First check for a predefined or target defined type. */       if (reg->tdesc_type) -       arch_reg->type = reg->tdesc_type->make_gdb_type (gdbarch); +   arch_reg->type = make_gdb_type (gdbarch, reg->tdesc_type);       /* Next try size-sensitive type shortcuts. */       else if (reg->type == "float") -- 2.14.3 (Apple Git-98) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Use visitors for make_gdb_type 2018-01-26 15:14 [PATCH] Use visitors for make_gdb_type Alan Hayward @ 2018-01-26 15:30 ` Alan Hayward 2018-01-29 2:24 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Alan Hayward @ 2018-01-26 15:30 UTC (permalink / raw) To: gdb-patches; +Cc: nd I appear to still have email issues - previous post had the tabs stripped out. Hoping this version is ok. Apologies. This patch implements the suggestion in the review for [PATCH v2 6/8] Create xml from target descriptions. Remove the make_gdb_type functions from the tdesc_type_ classes. Replace with a static make_gdb_type function that uses a element visitor called gdb_type_creator. I've defined gdb_type_creator inside make_gdb_type because it shouldn't be needed outside the function. The method of creating the types has not changed. This patch will allow a future patch to commonise the tdesc_types with gdbserver without having to move any gdb_type functionality. Alan. 2018-01-26 Alan Hayward <alan.hayward@arm.com> * target-descriptions.c (tdesc_type): Move make_gdb_type from here. (tdesc_type_builtin): Likewise. (tdesc_type_vector): Likewise. (tdesc_type_with_fields): Move make_gdb_type_ functions from here. (make_gdb_type_struct): Move from tdesc_type_with_fields. (make_gdb_type_union): Likewise. (make_gdb_type_flags): Likewise. (make_gdb_type_enum): Likewise. (make_gdb_type): New function. (tdesc_register_type): Use static make_gdb_type. --- gdb/target-descriptions.c | 460 +++++++++++++++++++++++++--------------------- 1 file changed, 254 insertions(+), 206 deletions(-) diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 1b20a12d76..55ac1d7a80 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -38,6 +38,8 @@ #include "completer.h" #include "readline/tilde.h" /* tilde_expand */ +static type *make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype); + /* The interface to visit different elements of target description. */ class tdesc_element_visitor @@ -223,11 +225,6 @@ struct tdesc_type : tdesc_element { return !(*this == other); } - - /* Construct, if necessary, and return the GDB type implementing this - target type for architecture GDBARCH. */ - - virtual type *make_gdb_type (struct gdbarch *gdbarch) const = 0; }; typedef std::unique_ptr<tdesc_type> tdesc_type_up; @@ -242,81 +239,6 @@ struct tdesc_type_builtin : tdesc_type { v.visit (this); } - - type *make_gdb_type (struct gdbarch *gdbarch) const override - { - switch (this->kind) - { - /* Predefined types. */ - case TDESC_TYPE_BOOL: - return builtin_type (gdbarch)->builtin_bool; - - case TDESC_TYPE_INT8: - return builtin_type (gdbarch)->builtin_int8; - - case TDESC_TYPE_INT16: - return builtin_type (gdbarch)->builtin_int16; - - case TDESC_TYPE_INT32: - return builtin_type (gdbarch)->builtin_int32; - - case TDESC_TYPE_INT64: - return builtin_type (gdbarch)->builtin_int64; - - case TDESC_TYPE_INT128: - return builtin_type (gdbarch)->builtin_int128; - - case TDESC_TYPE_UINT8: - return builtin_type (gdbarch)->builtin_uint8; - - case TDESC_TYPE_UINT16: - return builtin_type (gdbarch)->builtin_uint16; - - case TDESC_TYPE_UINT32: - return builtin_type (gdbarch)->builtin_uint32; - - case TDESC_TYPE_UINT64: - return builtin_type (gdbarch)->builtin_uint64; - - case TDESC_TYPE_UINT128: - return builtin_type (gdbarch)->builtin_uint128; - - case TDESC_TYPE_CODE_PTR: - return builtin_type (gdbarch)->builtin_func_ptr; - - case TDESC_TYPE_DATA_PTR: - return builtin_type (gdbarch)->builtin_data_ptr; - } - - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (gdb_type != NULL) - return gdb_type; - - switch (this->kind) - { - case TDESC_TYPE_IEEE_SINGLE: - return arch_float_type (gdbarch, -1, "builtin_type_ieee_single", - floatformats_ieee_single); - - case TDESC_TYPE_IEEE_DOUBLE: - return arch_float_type (gdbarch, -1, "builtin_type_ieee_double", - floatformats_ieee_double); - - case TDESC_TYPE_ARM_FPA_EXT: - return arch_float_type (gdbarch, -1, "builtin_type_arm_ext", - floatformats_arm_ext); - - case TDESC_TYPE_I387_EXT: - return arch_float_type (gdbarch, -1, "builtin_type_i387_ext", - floatformats_i387_ext); - } - - internal_error (__FILE__, __LINE__, - "Type \"%s\" has an unknown kind %d", - this->name.c_str (), this->kind); - - return NULL; - } }; /* tdesc_type for vector types. */ @@ -333,19 +255,6 @@ struct tdesc_type_vector : tdesc_type v.visit (this); } - type *make_gdb_type (struct gdbarch *gdbarch) const override - { - type *vector_gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (vector_gdb_type != NULL) - return vector_gdb_type; - - type *element_gdb_type = this->element_type->make_gdb_type (gdbarch); - vector_gdb_type = init_vector_type (element_gdb_type, this->count); - TYPE_NAME (vector_gdb_type) = xstrdup (this->name.c_str ()); - - return vector_gdb_type; - } - struct tdesc_type *element_type; int count; }; @@ -364,151 +273,290 @@ struct tdesc_type_with_fields : tdesc_type v.visit (this); } - type *make_gdb_type_struct (struct gdbarch *gdbarch) const - { - type *struct_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); - TYPE_NAME (struct_gdb_type) = xstrdup (this->name.c_str ()); - TYPE_TAG_NAME (struct_gdb_type) = TYPE_NAME (struct_gdb_type); + std::vector<tdesc_type_field> fields; + int size; +}; - for (const tdesc_type_field &f : this->fields) - { - if (f.start != -1 && f.end != -1) - { - /* Bitfield. */ - struct field *fld; - struct type *field_gdb_type; - int bitsize, total_size; - - /* This invariant should be preserved while creating types. */ - gdb_assert (this->size != 0); - if (f.type != NULL) - field_gdb_type = f.type->make_gdb_type (gdbarch); - else if (this->size > 4) - field_gdb_type = builtin_type (gdbarch)->builtin_uint64; - else - field_gdb_type = builtin_type (gdbarch)->builtin_uint32; - - fld = append_composite_type_field_raw - (struct_gdb_type, xstrdup (f.name.c_str ()), field_gdb_type); - - /* For little-endian, BITPOS counts from the LSB of - the structure and marks the LSB of the field. For - big-endian, BITPOS counts from the MSB of the - structure and marks the MSB of the field. Either - way, it is the number of bits to the "left" of the - field. To calculate this in big-endian, we need - the total size of the structure. */ - bitsize = f.end - f.start + 1; - total_size = this->size * TARGET_CHAR_BIT; - if (gdbarch_bits_big_endian (gdbarch)) - SET_FIELD_BITPOS (fld[0], total_size - f.start - bitsize); - else - SET_FIELD_BITPOS (fld[0], f.start); - FIELD_BITSIZE (fld[0]) = bitsize; - } - else - { - gdb_assert (f.start == -1 && f.end == -1); - type *field_gdb_type = f.type->make_gdb_type (gdbarch); - append_composite_type_field (struct_gdb_type, - xstrdup (f.name.c_str ()), - field_gdb_type); - } - } +static type * +make_gdb_type_struct (struct gdbarch *gdbarch, const tdesc_type_with_fields *e) +{ + type *struct_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); + TYPE_NAME (struct_gdb_type) = xstrdup (e->name.c_str ()); + TYPE_TAG_NAME (struct_gdb_type) = TYPE_NAME (struct_gdb_type); - if (this->size != 0) - TYPE_LENGTH (struct_gdb_type) = this->size; + for (const tdesc_type_field &f : e->fields) + { + if (f.start != -1 && f.end != -1) + { + /* Bitfield. */ + struct field *fld; + struct type *field_gdb_type; + int bitsize, total_size; + + /* This invariant should be preserved while creating types. */ + gdb_assert (e->size != 0); + if (f.type != NULL) + field_gdb_type = make_gdb_type (gdbarch, f.type); + else if (e->size > 4) + field_gdb_type = builtin_type (gdbarch)->builtin_uint64; + else + field_gdb_type = builtin_type (gdbarch)->builtin_uint32; + + fld = append_composite_type_field_raw + (struct_gdb_type, xstrdup (f.name.c_str ()), field_gdb_type); + + /* For little-endian, BITPOS counts from the LSB of + the structure and marks the LSB of the field. For + big-endian, BITPOS counts from the MSB of the + structure and marks the MSB of the field. Either + way, it is the number of bits to the "left" of the + field. To calculate this in big-endian, we need + the total size of the structure. */ + bitsize = f.end - f.start + 1; + total_size = e->size * TARGET_CHAR_BIT; + if (gdbarch_bits_big_endian (gdbarch)) + SET_FIELD_BITPOS (fld[0], total_size - f.start - bitsize); + else + SET_FIELD_BITPOS (fld[0], f.start); + FIELD_BITSIZE (fld[0]) = bitsize; + } + else + { + gdb_assert (f.start == -1 && f.end == -1); + type *field_gdb_type = make_gdb_type (gdbarch, f.type); + append_composite_type_field (struct_gdb_type, + xstrdup (f.name.c_str ()), + field_gdb_type); + } + } - return struct_gdb_type; - } + if (e->size != 0) + TYPE_LENGTH (struct_gdb_type) = e->size; - type *make_gdb_type_union (struct gdbarch *gdbarch) const - { - type *union_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION); - TYPE_NAME (union_gdb_type) = xstrdup (this->name.c_str ()); + return struct_gdb_type; +} - for (const tdesc_type_field &f : this->fields) - { - type* field_gdb_type = f.type->make_gdb_type (gdbarch); - append_composite_type_field (union_gdb_type, xstrdup (f.name.c_str ()), +static type * +make_gdb_type_union (struct gdbarch *gdbarch, const tdesc_type_with_fields *e) +{ + type *union_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION); + TYPE_NAME (union_gdb_type) = xstrdup (e->name.c_str ()); + + for (const tdesc_type_field &f : e->fields) + { + type* field_gdb_type = make_gdb_type (gdbarch, f.type); + append_composite_type_field (union_gdb_type, xstrdup (f.name.c_str ()), field_gdb_type); - /* If any of the children of a union are vectors, flag the - union as a vector also. This allows e.g. a union of two - vector types to show up automatically in "info vector". */ - if (TYPE_VECTOR (field_gdb_type)) - TYPE_VECTOR (union_gdb_type) = 1; - } + /* If any of the children of a union are vectors, flag the + union as a vector also. This allows e.g. a union of two + vector types to show up automatically in "info vector". */ + if (TYPE_VECTOR (field_gdb_type)) + TYPE_VECTOR (union_gdb_type) = 1; + } - return union_gdb_type; - } + return union_gdb_type; +} - type *make_gdb_type_flags (struct gdbarch *gdbarch) const - { - type *flags_gdb_type = arch_flags_type (gdbarch, this->name.c_str (), - this->size * TARGET_CHAR_BIT); +static type * +make_gdb_type_flags (struct gdbarch *gdbarch, const tdesc_type_with_fields *e) +{ + type *flags_gdb_type = arch_flags_type (gdbarch, e->name.c_str (), + e->size * TARGET_CHAR_BIT); - for (const tdesc_type_field &f : this->fields) - { + for (const tdesc_type_field &f : e->fields) + { int bitsize = f.end - f.start + 1; gdb_assert (f.type != NULL); - type *field_gdb_type = f.type->make_gdb_type (gdbarch); + type *field_gdb_type = make_gdb_type (gdbarch, f.type); append_flags_type_field (flags_gdb_type, f.start, bitsize, field_gdb_type, f.name.c_str ()); - } + } - return flags_gdb_type; - } + return flags_gdb_type; +} - type *make_gdb_type_enum (struct gdbarch *gdbarch) const - { - type *enum_gdb_type = arch_type (gdbarch, TYPE_CODE_ENUM, - this->size * TARGET_CHAR_BIT, - this->name.c_str ()); +static type * +make_gdb_type_enum (struct gdbarch *gdbarch, const tdesc_type_with_fields *e) +{ + type *enum_gdb_type = arch_type (gdbarch, TYPE_CODE_ENUM, + e->size * TARGET_CHAR_BIT, + e->name.c_str ()); - TYPE_UNSIGNED (enum_gdb_type) = 1; - for (const tdesc_type_field &f : this->fields) - { + TYPE_UNSIGNED (enum_gdb_type) = 1; + for (const tdesc_type_field &f : e->fields) + { struct field *fld = append_composite_type_field_raw (enum_gdb_type, xstrdup (f.name.c_str ()), NULL); SET_FIELD_BITPOS (fld[0], f.start); - } + } - return enum_gdb_type; - } + return enum_gdb_type; +} - type *make_gdb_type (struct gdbarch *gdbarch) const override +static type * +make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype) +{ + class gdb_type_creator : public tdesc_element_visitor { - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (gdb_type != NULL) - return gdb_type; + public: + gdb_type_creator (struct gdbarch *gdbarch) + : m_gdbarch (gdbarch) + {} - switch (this->kind) + type *get_type () { - case TDESC_TYPE_STRUCT: - return make_gdb_type_struct (gdbarch); - case TDESC_TYPE_UNION: - return make_gdb_type_union (gdbarch); - case TDESC_TYPE_FLAGS: - return make_gdb_type_flags (gdbarch); - case TDESC_TYPE_ENUM: - return make_gdb_type_enum (gdbarch); + return m_type; } - internal_error (__FILE__, __LINE__, - "Type \"%s\" has an unknown kind %d", - this->name.c_str (), this->kind); + void visit_pre (const target_desc *e) + {} - return NULL; - } + void visit_post (const target_desc *e) + {} - std::vector<tdesc_type_field> fields; - int size; -}; + void visit_pre (const tdesc_feature *e) + {} + + void visit_post (const tdesc_feature *e) + {} + + void visit (const tdesc_type_builtin *e) override + { + switch (e->kind) + { + /* Predefined types. */ + case TDESC_TYPE_BOOL: + m_type = builtin_type (m_gdbarch)->builtin_bool; + return; + case TDESC_TYPE_INT8: + m_type = builtin_type (m_gdbarch)->builtin_int8; + return; + case TDESC_TYPE_INT16: + m_type = builtin_type (m_gdbarch)->builtin_int16; + return; + case TDESC_TYPE_INT32: + m_type = builtin_type (m_gdbarch)->builtin_int32; + return; + case TDESC_TYPE_INT64: + m_type = builtin_type (m_gdbarch)->builtin_int64; + return; + case TDESC_TYPE_INT128: + m_type = builtin_type (m_gdbarch)->builtin_int128; + return; + case TDESC_TYPE_UINT8: + m_type = builtin_type (m_gdbarch)->builtin_uint8; + return; + case TDESC_TYPE_UINT16: + m_type = builtin_type (m_gdbarch)->builtin_uint16; + return; + case TDESC_TYPE_UINT32: + m_type = builtin_type (m_gdbarch)->builtin_uint32; + return; + case TDESC_TYPE_UINT64: + m_type = builtin_type (m_gdbarch)->builtin_uint64; + return; + case TDESC_TYPE_UINT128: + m_type = builtin_type (m_gdbarch)->builtin_uint128; + return; + case TDESC_TYPE_CODE_PTR: + m_type = builtin_type (m_gdbarch)->builtin_func_ptr; + return; + case TDESC_TYPE_DATA_PTR: + m_type = builtin_type (m_gdbarch)->builtin_data_ptr; + return; + } + + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; + + switch (e->kind) + { + case TDESC_TYPE_IEEE_SINGLE: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_ieee_single", + floatformats_ieee_single); + return; + + case TDESC_TYPE_IEEE_DOUBLE: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_ieee_double", + floatformats_ieee_double); + return; + case TDESC_TYPE_ARM_FPA_EXT: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_arm_ext", + floatformats_arm_ext); + return; + + case TDESC_TYPE_I387_EXT: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_i387_ext", + floatformats_i387_ext); + return; + } + + internal_error (__FILE__, __LINE__, + "Type \"%s\" has an unknown kind %d", + e->name.c_str (), e->kind); + } + + void visit (const tdesc_type_vector *e) override + { + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; + + type *element_gdb_type = make_gdb_type (m_gdbarch, e->element_type); + m_type = init_vector_type (element_gdb_type, e->count); + TYPE_NAME (m_type) = xstrdup (e->name.c_str ()); + return; + } + + void visit (const tdesc_type_with_fields *e) override + { + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; + + switch (e->kind) + { + case TDESC_TYPE_STRUCT: + m_type = make_gdb_type_struct (m_gdbarch, e); + return; + case TDESC_TYPE_UNION: + m_type = make_gdb_type_union (m_gdbarch, e); + return; + case TDESC_TYPE_FLAGS: + m_type = make_gdb_type_flags (m_gdbarch, e); + return; + case TDESC_TYPE_ENUM: + m_type = make_gdb_type_enum (m_gdbarch, e); + return; + } + + internal_error (__FILE__, __LINE__, + "Type \"%s\" has an unknown kind %d", + e->name.c_str (), e->kind); + } + + void visit (const tdesc_reg *reg) + {} + + private: + + /* The gdbarch used. */ + struct gdbarch *m_gdbarch; + + /* The type created. */ + type *m_type; + }; + + gdb_type_creator gdb_type (gdbarch); + ttype->accept (gdb_type); + return gdb_type.get_type (); +} /* A feature from a target description. Each feature is a collection of other elements, e.g. registers and types. */ @@ -1216,7 +1264,7 @@ tdesc_register_type (struct gdbarch *gdbarch, int regno) { /* First check for a predefined or target defined type. */ if (reg->tdesc_type) - arch_reg->type = reg->tdesc_type->make_gdb_type (gdbarch); + arch_reg->type = make_gdb_type (gdbarch, reg->tdesc_type); /* Next try size-sensitive type shortcuts. */ else if (reg->type == "float") -- 2.14.3 (Apple Git-98) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Use visitors for make_gdb_type 2018-01-26 15:30 ` Alan Hayward @ 2018-01-29 2:24 ` Simon Marchi 2018-01-29 9:30 ` Philipp Rudo 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2018-01-29 2:24 UTC (permalink / raw) To: Alan Hayward, gdb-patches; +Cc: nd On 2018-01-26 10:30 AM, Alan Hayward wrote: > I appear to still have email issues - previous post had the tabs stripped out. > Hoping this version is ok. Apologies. Hi Alan, I was able to apply it correctly. > This patch implements the suggestion in the review for > [PATCH v2 6/8] Create xml from target descriptions. > > Remove the make_gdb_type functions from the tdesc_type_ classes. > Replace with a static make_gdb_type function that uses a element > visitor called gdb_type_creator. > > I've defined gdb_type_creator inside make_gdb_type because it shouldn't > be needed outside the function. > > The method of creating the types has not changed. > > This patch will allow a future patch to commonise the tdesc_types with > gdbserver without having to move any gdb_type functionality. > > Alan. make_gdb_type_union & co could be methods of gdb_type_creator and access gdbarch directly. I think it would make sense to have all the knowledge of the tdesc type to gdb type conversion inside that class. > - type *make_gdb_type (struct gdbarch *gdbarch) const override > +static type * > +make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype) > +{ > + class gdb_type_creator : public tdesc_element_visitor > { > - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); > - if (gdb_type != NULL) > - return gdb_type; > + public: > + gdb_type_creator (struct gdbarch *gdbarch) > + : m_gdbarch (gdbarch) > + {} > > - switch (this->kind) > + type *get_type () > { > - case TDESC_TYPE_STRUCT: > - return make_gdb_type_struct (gdbarch); > - case TDESC_TYPE_UNION: > - return make_gdb_type_union (gdbarch); > - case TDESC_TYPE_FLAGS: > - return make_gdb_type_flags (gdbarch); > - case TDESC_TYPE_ENUM: > - return make_gdb_type_enum (gdbarch); > + return m_type; > } > > - internal_error (__FILE__, __LINE__, > - "Type \"%s\" has an unknown kind %d", > - this->name.c_str (), this->kind); > + void visit_pre (const target_desc *e) > + {} I think we should have empty default implementations of these visit functions in the base class, instead of having to declare them empty when unnecessary. Maybe Yao had a reason not to do this initially? In any case, make sure to mark the overriden methods with the "override" keyword. Using clang: /home/simark/src/binutils-gdb/gdb/target-descriptions.c:416:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit_pre (const target_desc *e) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:48:16: note: overridden virtual function is here virtual void visit_pre (const target_desc *e) = 0; ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:419:10: error: 'visit_post' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit_post (const target_desc *e) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:49:16: note: overridden virtual function is here virtual void visit_post (const target_desc *e) = 0; ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:422:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit_pre (const tdesc_feature *e) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:51:16: note: overridden virtual function is here virtual void visit_pre (const tdesc_feature *e) = 0; ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:425:10: error: 'visit_post' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit_post (const tdesc_feature *e) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:52:16: note: overridden virtual function is here virtual void visit_post (const tdesc_feature *e) = 0; ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:544:10: error: 'visit' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] void visit (const tdesc_reg *reg) ^ /home/simark/src/binutils-gdb/gdb/target-descriptions.c:58:16: note: overridden virtual function is here virtual void visit (const tdesc_reg *e) = 0; ^ Otherwise, LGTM. Thanks, Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Use visitors for make_gdb_type 2018-01-29 2:24 ` Simon Marchi @ 2018-01-29 9:30 ` Philipp Rudo 2018-01-29 15:31 ` Alan Hayward 0 siblings, 1 reply; 9+ messages in thread From: Philipp Rudo @ 2018-01-29 9:30 UTC (permalink / raw) To: Simon Marchi; +Cc: Alan Hayward, gdb-patches, nd Hi Alan, besides the comments Simon already made, the patch looks fine to me. Thanks Philipp On Sun, 28 Jan 2018 21:24:19 -0500 Simon Marchi <simark@simark.ca> wrote: > On 2018-01-26 10:30 AM, Alan Hayward wrote: > > I appear to still have email issues - previous post had the tabs stripped out. > > Hoping this version is ok. Apologies. > > Hi Alan, > > I was able to apply it correctly. > > This patch implements the suggestion in the review for > > [PATCH v2 6/8] Create xml from target descriptions. > > > > Remove the make_gdb_type functions from the tdesc_type_ classes. > > Replace with a static make_gdb_type function that uses a element > > visitor called gdb_type_creator. > > > > I've defined gdb_type_creator inside make_gdb_type because it shouldn't > > be needed outside the function. > > > > The method of creating the types has not changed. > > > > This patch will allow a future patch to commonise the tdesc_types with > > gdbserver without having to move any gdb_type functionality. > > > > Alan. > > make_gdb_type_union & co could be methods of gdb_type_creator and access gdbarch > directly. I think it would make sense to have all the knowledge of the tdesc type > to gdb type conversion inside that class. > > > - type *make_gdb_type (struct gdbarch *gdbarch) const override > > +static type * > > +make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype) > > +{ > > + class gdb_type_creator : public tdesc_element_visitor > > { > > - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); > > - if (gdb_type != NULL) > > - return gdb_type; > > + public: > > + gdb_type_creator (struct gdbarch *gdbarch) > > + : m_gdbarch (gdbarch) > > + {} > > > > - switch (this->kind) > > + type *get_type () > > { > > - case TDESC_TYPE_STRUCT: > > - return make_gdb_type_struct (gdbarch); > > - case TDESC_TYPE_UNION: > > - return make_gdb_type_union (gdbarch); > > - case TDESC_TYPE_FLAGS: > > - return make_gdb_type_flags (gdbarch); > > - case TDESC_TYPE_ENUM: > > - return make_gdb_type_enum (gdbarch); > > + return m_type; > > } > > > > - internal_error (__FILE__, __LINE__, > > - "Type \"%s\" has an unknown kind %d", > > - this->name.c_str (), this->kind); > > + void visit_pre (const target_desc *e) > > + {} > > I think we should have empty default implementations of these visit functions in the > base class, instead of having to declare them empty when unnecessary. Maybe Yao had > a reason not to do this initially? > > In any case, make sure to mark the overriden methods with the "override" keyword. Using > clang: > > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:416:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] > void visit_pre (const target_desc *e) > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:48:16: note: overridden virtual function is here > virtual void visit_pre (const target_desc *e) = 0; > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:419:10: error: 'visit_post' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] > void visit_post (const target_desc *e) > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:49:16: note: overridden virtual function is here > virtual void visit_post (const target_desc *e) = 0; > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:422:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] > void visit_pre (const tdesc_feature *e) > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:51:16: note: overridden virtual function is here > virtual void visit_pre (const tdesc_feature *e) = 0; > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:425:10: error: 'visit_post' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] > void visit_post (const tdesc_feature *e) > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:52:16: note: overridden virtual function is here > virtual void visit_post (const tdesc_feature *e) = 0; > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:544:10: error: 'visit' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] > void visit (const tdesc_reg *reg) > ^ > /home/simark/src/binutils-gdb/gdb/target-descriptions.c:58:16: note: overridden virtual function is here > virtual void visit (const tdesc_reg *e) = 0; > ^ > > Otherwise, LGTM. > > Thanks, > > Simon > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Use visitors for make_gdb_type 2018-01-29 9:30 ` Philipp Rudo @ 2018-01-29 15:31 ` Alan Hayward 2018-01-29 16:13 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Alan Hayward @ 2018-01-29 15:31 UTC (permalink / raw) To: Philipp Rudo, Simon Marchi; +Cc: gdb-patches, nd Updated version of patch at the bottom. > On 29 Jan 2018, at 09:28, Philipp Rudo <prudo@linux.vnet.ibm.com> wrote: > > Hi Alan, > > besides the comments Simon already made, the patch looks fine to me. > Thanks! > > On Sun, 28 Jan 2018 21:24:19 -0500 > Simon Marchi <simark@simark.ca> wrote: > >> On 2018-01-26 10:30 AM, Alan Hayward wrote: >>> I appear to still have email issues - previous post had the tabs stripped out. >>> Hoping this version is ok. Apologies. >> >> Hi Alan, >> >> I was able to apply it correctly. >>> Very strange! Think I’m ok now. >> >> make_gdb_type_union & co could be methods of gdb_type_creator and access gdbarch >> directly. I think it would make sense to have all the knowledge of the tdesc type >> to gdb type conversion inside that class. Agreed and done. >>> >>> + void visit_pre (const target_desc *e) >>> + {} >> >> I think we should have empty default implementations of these visit functions in the >> base class, instead of having to declare them empty when unnecessary. Maybe Yao had >> a reason not to do this initially? >> I think Yao didn’t add the method because tdesc_element_visitor is meant to be an interface and remain abstract. I considered putting the types into a parent class of tdesc_element_visitor, but that breaks the accept() functions horribly. Instead, I’ve created a new subclass from tdesc_element_visitor called tdesc_element_type_visitor which provides null implementations for all the non type visit functions. gdb_type_creator can now inherit from tdesc_element_type_visitor and only has to provide the three visitors. Are you happy with that? >> In any case, make sure to mark the overriden methods with the "override" keyword. Using >> clang: >> >> /home/simark/src/binutils-gdb/gdb/target-descriptions.c:416:10: error: 'visit_pre' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override] >> void visit_pre (const target_desc *e) Done. And thanks for the review. New version implements the above changes. Didn’t mention previously, but patches have been tested on a make check on x86 targets=all build with target board unix native-gdbserver. Built for power (because it does not use new target descriptions), but am unable to test. 2018-01-29 Alan Hayward <alan.hayward@arm.com> * target-descriptions.c (tdesc_element_type_visitor) Add class. (tdesc_type): Move make_gdb_type from here. (tdesc_type_builtin): Likewise. (tdesc_type_vector): Likewise. (tdesc_type_with_fields): Move make_gdb_type_ functions from here. (make_gdb_type_struct): Move from tdesc_type_with_fields. (make_gdb_type_union): Likewise. (make_gdb_type_flags): Likewise. (make_gdb_type_enum): Likewise. (make_gdb_type): New function. (tdesc_register_type): Use static make_gdb_type. --- gdb/target-descriptions.c | 480 +++++++++++++++++++++++++--------------------- 1 file changed, 264 insertions(+), 216 deletions(-) diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 1b20a12d76..df0ad9241f 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -38,6 +38,8 @@ #include "completer.h" #include "readline/tilde.h" /* tilde_expand */ +static type *make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype); + /* The interface to visit different elements of target description. */ class tdesc_element_visitor @@ -56,6 +58,32 @@ public: virtual void visit (const tdesc_reg *e) = 0; }; +/* The interface to visit different elements of tdesc type. Restricts vistors + to only the ones that work on types. */ + +class tdesc_element_type_visitor : public tdesc_element_visitor +{ +public: + void visit_pre (const target_desc *e) override + {} + + void visit_post (const target_desc *e) override + {} + + void visit_pre (const tdesc_feature *e) override + {} + + void visit_post (const tdesc_feature *e) override + {} + + virtual void visit (const tdesc_type_builtin *e) = 0; + virtual void visit (const tdesc_type_vector *e) = 0; + virtual void visit (const tdesc_type_with_fields *e) = 0; + + void visit (const tdesc_reg *e) override + {} +}; + class tdesc_element { public: @@ -223,11 +251,6 @@ struct tdesc_type : tdesc_element { return !(*this == other); } - - /* Construct, if necessary, and return the GDB type implementing this - target type for architecture GDBARCH. */ - - virtual type *make_gdb_type (struct gdbarch *gdbarch) const = 0; }; typedef std::unique_ptr<tdesc_type> tdesc_type_up; @@ -242,81 +265,6 @@ struct tdesc_type_builtin : tdesc_type { v.visit (this); } - - type *make_gdb_type (struct gdbarch *gdbarch) const override - { - switch (this->kind) - { - /* Predefined types. */ - case TDESC_TYPE_BOOL: - return builtin_type (gdbarch)->builtin_bool; - - case TDESC_TYPE_INT8: - return builtin_type (gdbarch)->builtin_int8; - - case TDESC_TYPE_INT16: - return builtin_type (gdbarch)->builtin_int16; - - case TDESC_TYPE_INT32: - return builtin_type (gdbarch)->builtin_int32; - - case TDESC_TYPE_INT64: - return builtin_type (gdbarch)->builtin_int64; - - case TDESC_TYPE_INT128: - return builtin_type (gdbarch)->builtin_int128; - - case TDESC_TYPE_UINT8: - return builtin_type (gdbarch)->builtin_uint8; - - case TDESC_TYPE_UINT16: - return builtin_type (gdbarch)->builtin_uint16; - - case TDESC_TYPE_UINT32: - return builtin_type (gdbarch)->builtin_uint32; - - case TDESC_TYPE_UINT64: - return builtin_type (gdbarch)->builtin_uint64; - - case TDESC_TYPE_UINT128: - return builtin_type (gdbarch)->builtin_uint128; - - case TDESC_TYPE_CODE_PTR: - return builtin_type (gdbarch)->builtin_func_ptr; - - case TDESC_TYPE_DATA_PTR: - return builtin_type (gdbarch)->builtin_data_ptr; - } - - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (gdb_type != NULL) - return gdb_type; - - switch (this->kind) - { - case TDESC_TYPE_IEEE_SINGLE: - return arch_float_type (gdbarch, -1, "builtin_type_ieee_single", - floatformats_ieee_single); - - case TDESC_TYPE_IEEE_DOUBLE: - return arch_float_type (gdbarch, -1, "builtin_type_ieee_double", - floatformats_ieee_double); - - case TDESC_TYPE_ARM_FPA_EXT: - return arch_float_type (gdbarch, -1, "builtin_type_arm_ext", - floatformats_arm_ext); - - case TDESC_TYPE_I387_EXT: - return arch_float_type (gdbarch, -1, "builtin_type_i387_ext", - floatformats_i387_ext); - } - - internal_error (__FILE__, __LINE__, - "Type \"%s\" has an unknown kind %d", - this->name.c_str (), this->kind); - - return NULL; - } }; /* tdesc_type for vector types. */ @@ -333,19 +281,6 @@ struct tdesc_type_vector : tdesc_type v.visit (this); } - type *make_gdb_type (struct gdbarch *gdbarch) const override - { - type *vector_gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (vector_gdb_type != NULL) - return vector_gdb_type; - - type *element_gdb_type = this->element_type->make_gdb_type (gdbarch); - vector_gdb_type = init_vector_type (element_gdb_type, this->count); - TYPE_NAME (vector_gdb_type) = xstrdup (this->name.c_str ()); - - return vector_gdb_type; - } - struct tdesc_type *element_type; int count; }; @@ -364,151 +299,264 @@ struct tdesc_type_with_fields : tdesc_type v.visit (this); } - type *make_gdb_type_struct (struct gdbarch *gdbarch) const + std::vector<tdesc_type_field> fields; + int size; +}; + +/* Convert a tdesc_type to a gdb type. */ + +static type * +make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype) +{ + class gdb_type_creator : public tdesc_element_type_visitor { - type *struct_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); - TYPE_NAME (struct_gdb_type) = xstrdup (this->name.c_str ()); - TYPE_TAG_NAME (struct_gdb_type) = TYPE_NAME (struct_gdb_type); + public: + gdb_type_creator (struct gdbarch *gdbarch) + : m_gdbarch (gdbarch) + {} - for (const tdesc_type_field &f : this->fields) - { - if (f.start != -1 && f.end != -1) - { - /* Bitfield. */ - struct field *fld; - struct type *field_gdb_type; - int bitsize, total_size; - - /* This invariant should be preserved while creating types. */ - gdb_assert (this->size != 0); - if (f.type != NULL) - field_gdb_type = f.type->make_gdb_type (gdbarch); - else if (this->size > 4) - field_gdb_type = builtin_type (gdbarch)->builtin_uint64; - else - field_gdb_type = builtin_type (gdbarch)->builtin_uint32; - - fld = append_composite_type_field_raw - (struct_gdb_type, xstrdup (f.name.c_str ()), field_gdb_type); - - /* For little-endian, BITPOS counts from the LSB of - the structure and marks the LSB of the field. For - big-endian, BITPOS counts from the MSB of the - structure and marks the MSB of the field. Either - way, it is the number of bits to the "left" of the - field. To calculate this in big-endian, we need - the total size of the structure. */ - bitsize = f.end - f.start + 1; - total_size = this->size * TARGET_CHAR_BIT; - if (gdbarch_bits_big_endian (gdbarch)) - SET_FIELD_BITPOS (fld[0], total_size - f.start - bitsize); - else - SET_FIELD_BITPOS (fld[0], f.start); - FIELD_BITSIZE (fld[0]) = bitsize; - } - else - { - gdb_assert (f.start == -1 && f.end == -1); - type *field_gdb_type = f.type->make_gdb_type (gdbarch); - append_composite_type_field (struct_gdb_type, - xstrdup (f.name.c_str ()), - field_gdb_type); - } - } + type *get_type () + { + return m_type; + } - if (this->size != 0) - TYPE_LENGTH (struct_gdb_type) = this->size; + void visit (const tdesc_type_builtin *e) override + { + switch (e->kind) + { + /* Predefined types. */ + case TDESC_TYPE_BOOL: + m_type = builtin_type (m_gdbarch)->builtin_bool; + return; + case TDESC_TYPE_INT8: + m_type = builtin_type (m_gdbarch)->builtin_int8; + return; + case TDESC_TYPE_INT16: + m_type = builtin_type (m_gdbarch)->builtin_int16; + return; + case TDESC_TYPE_INT32: + m_type = builtin_type (m_gdbarch)->builtin_int32; + return; + case TDESC_TYPE_INT64: + m_type = builtin_type (m_gdbarch)->builtin_int64; + return; + case TDESC_TYPE_INT128: + m_type = builtin_type (m_gdbarch)->builtin_int128; + return; + case TDESC_TYPE_UINT8: + m_type = builtin_type (m_gdbarch)->builtin_uint8; + return; + case TDESC_TYPE_UINT16: + m_type = builtin_type (m_gdbarch)->builtin_uint16; + return; + case TDESC_TYPE_UINT32: + m_type = builtin_type (m_gdbarch)->builtin_uint32; + return; + case TDESC_TYPE_UINT64: + m_type = builtin_type (m_gdbarch)->builtin_uint64; + return; + case TDESC_TYPE_UINT128: + m_type = builtin_type (m_gdbarch)->builtin_uint128; + return; + case TDESC_TYPE_CODE_PTR: + m_type = builtin_type (m_gdbarch)->builtin_func_ptr; + return; + case TDESC_TYPE_DATA_PTR: + m_type = builtin_type (m_gdbarch)->builtin_data_ptr; + return; + } - return struct_gdb_type; - } + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; - type *make_gdb_type_union (struct gdbarch *gdbarch) const - { - type *union_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION); - TYPE_NAME (union_gdb_type) = xstrdup (this->name.c_str ()); + switch (e->kind) + { + case TDESC_TYPE_IEEE_SINGLE: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_ieee_single", + floatformats_ieee_single); + return; + + case TDESC_TYPE_IEEE_DOUBLE: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_ieee_double", + floatformats_ieee_double); + return; + case TDESC_TYPE_ARM_FPA_EXT: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_arm_ext", + floatformats_arm_ext); + return; + + case TDESC_TYPE_I387_EXT: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_i387_ext", + floatformats_i387_ext); + return; + } - for (const tdesc_type_field &f : this->fields) - { - type* field_gdb_type = f.type->make_gdb_type (gdbarch); - append_composite_type_field (union_gdb_type, xstrdup (f.name.c_str ()), - field_gdb_type); - - /* If any of the children of a union are vectors, flag the - union as a vector also. This allows e.g. a union of two - vector types to show up automatically in "info vector". */ - if (TYPE_VECTOR (field_gdb_type)) - TYPE_VECTOR (union_gdb_type) = 1; - } + internal_error (__FILE__, __LINE__, + "Type \"%s\" has an unknown kind %d", + e->name.c_str (), e->kind); + } - return union_gdb_type; - } + void visit (const tdesc_type_vector *e) override + { + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; + + type *element_gdb_type = make_gdb_type (m_gdbarch, e->element_type); + m_type = init_vector_type (element_gdb_type, e->count); + TYPE_NAME (m_type) = xstrdup (e->name.c_str ()); + return; + } - type *make_gdb_type_flags (struct gdbarch *gdbarch) const - { - type *flags_gdb_type = arch_flags_type (gdbarch, this->name.c_str (), - this->size * TARGET_CHAR_BIT); + void visit (const tdesc_type_with_fields *e) override + { + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; - for (const tdesc_type_field &f : this->fields) - { - int bitsize = f.end - f.start + 1; + switch (e->kind) + { + case TDESC_TYPE_STRUCT: + make_gdb_type_struct (e); + return; + case TDESC_TYPE_UNION: + make_gdb_type_union (e); + return; + case TDESC_TYPE_FLAGS: + make_gdb_type_flags (e); + return; + case TDESC_TYPE_ENUM: + make_gdb_type_enum (e); + return; + } - gdb_assert (f.type != NULL); - type *field_gdb_type = f.type->make_gdb_type (gdbarch); - append_flags_type_field (flags_gdb_type, f.start, bitsize, - field_gdb_type, f.name.c_str ()); - } + internal_error (__FILE__, __LINE__, + "Type \"%s\" has an unknown kind %d", + e->name.c_str (), e->kind); + } - return flags_gdb_type; - } + private: - type *make_gdb_type_enum (struct gdbarch *gdbarch) const - { - type *enum_gdb_type = arch_type (gdbarch, TYPE_CODE_ENUM, - this->size * TARGET_CHAR_BIT, - this->name.c_str ()); + void make_gdb_type_struct (const tdesc_type_with_fields *e) + { + m_type = arch_composite_type (m_gdbarch, NULL, TYPE_CODE_STRUCT); + TYPE_NAME (m_type) = xstrdup (e->name.c_str ()); + TYPE_TAG_NAME (m_type) = TYPE_NAME (m_type); - TYPE_UNSIGNED (enum_gdb_type) = 1; - for (const tdesc_type_field &f : this->fields) - { - struct field *fld - = append_composite_type_field_raw (enum_gdb_type, + for (const tdesc_type_field &f : e->fields) + { + if (f.start != -1 && f.end != -1) + { + /* Bitfield. */ + struct field *fld; + struct type *field_gdb_type; + int bitsize, total_size; + + /* This invariant should be preserved while creating types. */ + gdb_assert (e->size != 0); + if (f.type != NULL) + field_gdb_type = make_gdb_type (m_gdbarch, f.type); + else if (e->size > 4) + field_gdb_type = builtin_type (m_gdbarch)->builtin_uint64; + else + field_gdb_type = builtin_type (m_gdbarch)->builtin_uint32; + + fld = append_composite_type_field_raw + (m_type, xstrdup (f.name.c_str ()), field_gdb_type); + + /* For little-endian, BITPOS counts from the LSB of + the structure and marks the LSB of the field. For + big-endian, BITPOS counts from the MSB of the + structure and marks the MSB of the field. Either + way, it is the number of bits to the "left" of the + field. To calculate this in big-endian, we need + the total size of the structure. */ + bitsize = f.end - f.start + 1; + total_size = e->size * TARGET_CHAR_BIT; + if (gdbarch_bits_big_endian (m_gdbarch)) + SET_FIELD_BITPOS (fld[0], total_size - f.start - bitsize); + else + SET_FIELD_BITPOS (fld[0], f.start); + FIELD_BITSIZE (fld[0]) = bitsize; + } + else + { + gdb_assert (f.start == -1 && f.end == -1); + type *field_gdb_type = make_gdb_type (m_gdbarch, f.type); + append_composite_type_field (m_type, xstrdup (f.name.c_str ()), - NULL); + field_gdb_type); + } + } - SET_FIELD_BITPOS (fld[0], f.start); - } + if (e->size != 0) + TYPE_LENGTH (m_type) = e->size; + } - return enum_gdb_type; - } + void make_gdb_type_union (const tdesc_type_with_fields *e) + { + m_type = arch_composite_type (m_gdbarch, NULL, TYPE_CODE_UNION); + TYPE_NAME (m_type) = xstrdup (e->name.c_str ()); - type *make_gdb_type (struct gdbarch *gdbarch) const override - { - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (gdb_type != NULL) - return gdb_type; + for (const tdesc_type_field &f : e->fields) + { + type* field_gdb_type = make_gdb_type (m_gdbarch, f.type); + append_composite_type_field (m_type, xstrdup (f.name.c_str ()), + field_gdb_type); + + /* If any of the children of a union are vectors, flag the + union as a vector also. This allows e.g. a union of two + vector types to show up automatically in "info vector". */ + if (TYPE_VECTOR (field_gdb_type)) + TYPE_VECTOR (m_type) = 1; + } + } - switch (this->kind) + void make_gdb_type_flags (const tdesc_type_with_fields *e) { - case TDESC_TYPE_STRUCT: - return make_gdb_type_struct (gdbarch); - case TDESC_TYPE_UNION: - return make_gdb_type_union (gdbarch); - case TDESC_TYPE_FLAGS: - return make_gdb_type_flags (gdbarch); - case TDESC_TYPE_ENUM: - return make_gdb_type_enum (gdbarch); + m_type = arch_flags_type (m_gdbarch, e->name.c_str (), + e->size * TARGET_CHAR_BIT); + + for (const tdesc_type_field &f : e->fields) + { + int bitsize = f.end - f.start + 1; + + gdb_assert (f.type != NULL); + type *field_gdb_type = make_gdb_type (m_gdbarch, f.type); + append_flags_type_field (m_type, f.start, bitsize, + field_gdb_type, f.name.c_str ()); + } } - internal_error (__FILE__, __LINE__, - "Type \"%s\" has an unknown kind %d", - this->name.c_str (), this->kind); + void make_gdb_type_enum (const tdesc_type_with_fields *e) + { + m_type = arch_type (m_gdbarch, TYPE_CODE_ENUM, e->size * TARGET_CHAR_BIT, + e->name.c_str ()); - return NULL; - } + TYPE_UNSIGNED (m_type) = 1; + for (const tdesc_type_field &f : e->fields) + { + struct field *fld + = append_composite_type_field_raw (m_type, + xstrdup (f.name.c_str ()), + NULL); - std::vector<tdesc_type_field> fields; - int size; -}; + SET_FIELD_BITPOS (fld[0], f.start); + } + } + + /* The gdbarch used. */ + struct gdbarch *m_gdbarch; + + /* The type created. */ + type *m_type; + }; + + gdb_type_creator gdb_type (gdbarch); + ttype->accept (gdb_type); + return gdb_type.get_type (); +} /* A feature from a target description. Each feature is a collection of other elements, e.g. registers and types. */ @@ -1216,7 +1264,7 @@ tdesc_register_type (struct gdbarch *gdbarch, int regno) { /* First check for a predefined or target defined type. */ if (reg->tdesc_type) - arch_reg->type = reg->tdesc_type->make_gdb_type (gdbarch); + arch_reg->type = make_gdb_type (gdbarch, reg->tdesc_type); /* Next try size-sensitive type shortcuts. */ else if (reg->type == "float") -- 2.14.3 (Apple Git-98) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Use visitors for make_gdb_type 2018-01-29 15:31 ` Alan Hayward @ 2018-01-29 16:13 ` Simon Marchi 2018-01-29 16:54 ` Yao Qi 0 siblings, 1 reply; 9+ messages in thread From: Simon Marchi @ 2018-01-29 16:13 UTC (permalink / raw) To: Alan Hayward, Philipp Rudo, Simon Marchi; +Cc: gdb-patches, nd, Yao Qi On 2018-01-29 10:31 AM, Alan Hayward wrote: >> On Sun, 28 Jan 2018 21:24:19 -0500 >> Simon Marchi <simark@simark.ca> wrote: >> >>> On 2018-01-26 10:30 AM, Alan Hayward wrote: >>>> I appear to still have email issues - previous post had the tabs stripped out. >>>> Hoping this version is ok. Apologies. >>> >>> Hi Alan, >>> >>> I was able to apply it correctly. >>>> > > Very strange! Think Iâm ok now. Well, now I am unable to apply this one :(. The message body is encoded in base64, I tried to decode it and apply it with git-apply, but it doesn't apply, not sure why. git-send-email is really the safest way. >>>> + void visit_pre (const target_desc *e) >>>> + {} >>> >>> I think we should have empty default implementations of these visit functions in the >>> base class, instead of having to declare them empty when unnecessary. Maybe Yao had >>> a reason not to do this initially? >>> > > I think Yao didnât add the method because tdesc_element_visitor is meant to be an interface > and remain abstract. A type can be abstract but have implementations for some of its methods. To make a type abstract, it is common to make the destructor pure virtual, if no other method is pure virtual. > I considered putting the types into a parent class of tdesc_element_visitor, but that breaks > the accept() functions horribly. > Instead, Iâve created a new subclass from tdesc_element_visitor called tdesc_element_type_visitor > which provides null implementations for all the non type visit functions. gdb_type_creator can > now inherit from tdesc_element_type_visitor and only has to provide the three visitors. > > Are you happy with that? That seems like unnecessary boilerplate to me. I really don't see why classes derived from tdesc_element_visitor have to implement methods for nodes they don't care about. I added Yao in CC so he can chime in. Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Use visitors for make_gdb_type 2018-01-29 16:13 ` Simon Marchi @ 2018-01-29 16:54 ` Yao Qi 2018-01-30 15:16 ` Alan Hayward 0 siblings, 1 reply; 9+ messages in thread From: Yao Qi @ 2018-01-29 16:54 UTC (permalink / raw) To: Simon Marchi; +Cc: Alan Hayward, Philipp Rudo, Simon Marchi, gdb-patches, nd On Mon, Jan 29, 2018 at 4:12 PM, Simon Marchi <simon.marchi@ericsson.com> wrote: > > That seems like unnecessary boilerplate to me. I really don't see why classes derived > from tdesc_element_visitor have to implement methods for nodes they don't care about. > > I added Yao in CC so he can chime in. When I wrote tdesc_element_visitor, in my mind, it is an interface, so I expect child class implement all the methods, because at that moment, all methods are needed, no empty methods. However, the situation changed a little bit, as per Alan's needs, part of the methods of tdesc_element_visitor are needed, and the rest of methods are empty somewhere. I don't mind converting tdesc_element_visitor into a base class which has all these methods empty as a default. That is fine to me. By the way, Alan's approach is fine to me as well :) -- Yao (齐尧) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Use visitors for make_gdb_type 2018-01-29 16:54 ` Yao Qi @ 2018-01-30 15:16 ` Alan Hayward 2018-02-05 16:21 ` Simon Marchi 0 siblings, 1 reply; 9+ messages in thread From: Alan Hayward @ 2018-01-30 15:16 UTC (permalink / raw) To: Yao Qi; +Cc: Simon Marchi, Philipp Rudo, Simon Marchi, gdb-patches, nd > On 29 Jan 2018, at 16:54, Yao Qi <qiyaoltc@gmail.com> wrote: > > On Mon, Jan 29, 2018 at 4:12 PM, Simon Marchi <simon.marchi@ericsson.com> wrote: >> >> That seems like unnecessary boilerplate to me. I really don't see why classes derived >> from tdesc_element_visitor have to implement methods for nodes they don't care about. >> >> I added Yao in CC so he can chime in. > > When I wrote tdesc_element_visitor, in my mind, it is an interface, so > I expect child > class implement all the methods, because at that moment, all methods are needed, > no empty methods. However, the situation changed a little bit, as per > Alan's needs, > part of the methods of tdesc_element_visitor are needed, and the rest of methods > are empty somewhere. I don't mind converting tdesc_element_visitor into a base > class which has all these methods empty as a default. That is fine to > me. By the > way, Alan's approach is fine to me as well :) > I’ve removed the extra class and replaced with default implementations in tdesc_element_visitor. All ok? Tested on a make check on x86 targets=all build with target board unix native-gdbserver. Built for power (because it does not use new target descriptions), but am unable to test. 2018-01-30 Alan Hayward <alan.hayward@arm.com> * target-descriptions.c (tdesc_element_visitor) Add empty implementations. (tdesc_type): Move make_gdb_type from here. (tdesc_type_builtin): Likewise. (tdesc_type_vector): Likewise. (tdesc_type_with_fields): Move make_gdb_type_ functions from here. (make_gdb_type_struct): Move from tdesc_type_with_fields. (make_gdb_type_union): Likewise. (make_gdb_type_flags): Likewise. (make_gdb_type_enum): Likewise. (make_gdb_type): New function. (tdesc_register_type): Use static make_gdb_type. diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 1b20a12d769718e591dea6df8183c2e9ecfac990..ce4cf76899cccb1c009bc556aeb6a74f0913edbe 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -38,22 +38,36 @@ #include "completer.h" #include "readline/tilde.h" /* tilde_expand */ +static type *make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype); + /* The interface to visit different elements of target description. */ class tdesc_element_visitor { public: - virtual void visit_pre (const target_desc *e) = 0; - virtual void visit_post (const target_desc *e) = 0; + virtual void visit_pre (const target_desc *e) + {} + + virtual void visit_post (const target_desc *e) + {} + + virtual void visit_pre (const tdesc_feature *e) + {} + + virtual void visit_post (const tdesc_feature *e) + {} - virtual void visit_pre (const tdesc_feature *e) = 0; - virtual void visit_post (const tdesc_feature *e) = 0; + virtual void visit (const tdesc_type_builtin *e) + {} + + virtual void visit (const tdesc_type_vector *e) + {} - virtual void visit (const tdesc_type_builtin *e) = 0; - virtual void visit (const tdesc_type_vector *e) = 0; - virtual void visit (const tdesc_type_with_fields *e) = 0; + virtual void visit (const tdesc_type_with_fields *e) + {} - virtual void visit (const tdesc_reg *e) = 0; + virtual void visit (const tdesc_reg *e) + {} }; class tdesc_element @@ -223,11 +237,6 @@ struct tdesc_type : tdesc_element { return !(*this == other); } - - /* Construct, if necessary, and return the GDB type implementing this - target type for architecture GDBARCH. */ - - virtual type *make_gdb_type (struct gdbarch *gdbarch) const = 0; }; typedef std::unique_ptr<tdesc_type> tdesc_type_up; @@ -242,81 +251,6 @@ struct tdesc_type_builtin : tdesc_type { v.visit (this); } - - type *make_gdb_type (struct gdbarch *gdbarch) const override - { - switch (this->kind) - { - /* Predefined types. */ - case TDESC_TYPE_BOOL: - return builtin_type (gdbarch)->builtin_bool; - - case TDESC_TYPE_INT8: - return builtin_type (gdbarch)->builtin_int8; - - case TDESC_TYPE_INT16: - return builtin_type (gdbarch)->builtin_int16; - - case TDESC_TYPE_INT32: - return builtin_type (gdbarch)->builtin_int32; - - case TDESC_TYPE_INT64: - return builtin_type (gdbarch)->builtin_int64; - - case TDESC_TYPE_INT128: - return builtin_type (gdbarch)->builtin_int128; - - case TDESC_TYPE_UINT8: - return builtin_type (gdbarch)->builtin_uint8; - - case TDESC_TYPE_UINT16: - return builtin_type (gdbarch)->builtin_uint16; - - case TDESC_TYPE_UINT32: - return builtin_type (gdbarch)->builtin_uint32; - - case TDESC_TYPE_UINT64: - return builtin_type (gdbarch)->builtin_uint64; - - case TDESC_TYPE_UINT128: - return builtin_type (gdbarch)->builtin_uint128; - - case TDESC_TYPE_CODE_PTR: - return builtin_type (gdbarch)->builtin_func_ptr; - - case TDESC_TYPE_DATA_PTR: - return builtin_type (gdbarch)->builtin_data_ptr; - } - - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (gdb_type != NULL) - return gdb_type; - - switch (this->kind) - { - case TDESC_TYPE_IEEE_SINGLE: - return arch_float_type (gdbarch, -1, "builtin_type_ieee_single", - floatformats_ieee_single); - - case TDESC_TYPE_IEEE_DOUBLE: - return arch_float_type (gdbarch, -1, "builtin_type_ieee_double", - floatformats_ieee_double); - - case TDESC_TYPE_ARM_FPA_EXT: - return arch_float_type (gdbarch, -1, "builtin_type_arm_ext", - floatformats_arm_ext); - - case TDESC_TYPE_I387_EXT: - return arch_float_type (gdbarch, -1, "builtin_type_i387_ext", - floatformats_i387_ext); - } - - internal_error (__FILE__, __LINE__, - "Type \"%s\" has an unknown kind %d", - this->name.c_str (), this->kind); - - return NULL; - } }; /* tdesc_type for vector types. */ @@ -333,19 +267,6 @@ struct tdesc_type_vector : tdesc_type v.visit (this); } - type *make_gdb_type (struct gdbarch *gdbarch) const override - { - type *vector_gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (vector_gdb_type != NULL) - return vector_gdb_type; - - type *element_gdb_type = this->element_type->make_gdb_type (gdbarch); - vector_gdb_type = init_vector_type (element_gdb_type, this->count); - TYPE_NAME (vector_gdb_type) = xstrdup (this->name.c_str ()); - - return vector_gdb_type; - } - struct tdesc_type *element_type; int count; }; @@ -364,151 +285,264 @@ struct tdesc_type_with_fields : tdesc_type v.visit (this); } - type *make_gdb_type_struct (struct gdbarch *gdbarch) const + std::vector<tdesc_type_field> fields; + int size; +}; + +/* Convert a tdesc_type to a gdb type. */ + +static type * +make_gdb_type (struct gdbarch *gdbarch, struct tdesc_type *ttype) +{ + class gdb_type_creator : public tdesc_element_visitor { - type *struct_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_STRUCT); - TYPE_NAME (struct_gdb_type) = xstrdup (this->name.c_str ()); - TYPE_TAG_NAME (struct_gdb_type) = TYPE_NAME (struct_gdb_type); + public: + gdb_type_creator (struct gdbarch *gdbarch) + : m_gdbarch (gdbarch) + {} - for (const tdesc_type_field &f : this->fields) - { - if (f.start != -1 && f.end != -1) - { - /* Bitfield. */ - struct field *fld; - struct type *field_gdb_type; - int bitsize, total_size; - - /* This invariant should be preserved while creating types. */ - gdb_assert (this->size != 0); - if (f.type != NULL) - field_gdb_type = f.type->make_gdb_type (gdbarch); - else if (this->size > 4) - field_gdb_type = builtin_type (gdbarch)->builtin_uint64; - else - field_gdb_type = builtin_type (gdbarch)->builtin_uint32; - - fld = append_composite_type_field_raw - (struct_gdb_type, xstrdup (f.name.c_str ()), field_gdb_type); - - /* For little-endian, BITPOS counts from the LSB of - the structure and marks the LSB of the field. For - big-endian, BITPOS counts from the MSB of the - structure and marks the MSB of the field. Either - way, it is the number of bits to the "left" of the - field. To calculate this in big-endian, we need - the total size of the structure. */ - bitsize = f.end - f.start + 1; - total_size = this->size * TARGET_CHAR_BIT; - if (gdbarch_bits_big_endian (gdbarch)) - SET_FIELD_BITPOS (fld[0], total_size - f.start - bitsize); - else - SET_FIELD_BITPOS (fld[0], f.start); - FIELD_BITSIZE (fld[0]) = bitsize; - } - else - { - gdb_assert (f.start == -1 && f.end == -1); - type *field_gdb_type = f.type->make_gdb_type (gdbarch); - append_composite_type_field (struct_gdb_type, - xstrdup (f.name.c_str ()), - field_gdb_type); - } - } + type *get_type () + { + return m_type; + } - if (this->size != 0) - TYPE_LENGTH (struct_gdb_type) = this->size; + void visit (const tdesc_type_builtin *e) override + { + switch (e->kind) + { + /* Predefined types. */ + case TDESC_TYPE_BOOL: + m_type = builtin_type (m_gdbarch)->builtin_bool; + return; + case TDESC_TYPE_INT8: + m_type = builtin_type (m_gdbarch)->builtin_int8; + return; + case TDESC_TYPE_INT16: + m_type = builtin_type (m_gdbarch)->builtin_int16; + return; + case TDESC_TYPE_INT32: + m_type = builtin_type (m_gdbarch)->builtin_int32; + return; + case TDESC_TYPE_INT64: + m_type = builtin_type (m_gdbarch)->builtin_int64; + return; + case TDESC_TYPE_INT128: + m_type = builtin_type (m_gdbarch)->builtin_int128; + return; + case TDESC_TYPE_UINT8: + m_type = builtin_type (m_gdbarch)->builtin_uint8; + return; + case TDESC_TYPE_UINT16: + m_type = builtin_type (m_gdbarch)->builtin_uint16; + return; + case TDESC_TYPE_UINT32: + m_type = builtin_type (m_gdbarch)->builtin_uint32; + return; + case TDESC_TYPE_UINT64: + m_type = builtin_type (m_gdbarch)->builtin_uint64; + return; + case TDESC_TYPE_UINT128: + m_type = builtin_type (m_gdbarch)->builtin_uint128; + return; + case TDESC_TYPE_CODE_PTR: + m_type = builtin_type (m_gdbarch)->builtin_func_ptr; + return; + case TDESC_TYPE_DATA_PTR: + m_type = builtin_type (m_gdbarch)->builtin_data_ptr; + return; + } - return struct_gdb_type; - } + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; - type *make_gdb_type_union (struct gdbarch *gdbarch) const - { - type *union_gdb_type = arch_composite_type (gdbarch, NULL, TYPE_CODE_UNION); - TYPE_NAME (union_gdb_type) = xstrdup (this->name.c_str ()); + switch (e->kind) + { + case TDESC_TYPE_IEEE_SINGLE: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_ieee_single", + floatformats_ieee_single); + return; + + case TDESC_TYPE_IEEE_DOUBLE: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_ieee_double", + floatformats_ieee_double); + return; + case TDESC_TYPE_ARM_FPA_EXT: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_arm_ext", + floatformats_arm_ext); + return; + + case TDESC_TYPE_I387_EXT: + m_type = arch_float_type (m_gdbarch, -1, "builtin_type_i387_ext", + floatformats_i387_ext); + return; + } - for (const tdesc_type_field &f : this->fields) - { - type* field_gdb_type = f.type->make_gdb_type (gdbarch); - append_composite_type_field (union_gdb_type, xstrdup (f.name.c_str ()), - field_gdb_type); - - /* If any of the children of a union are vectors, flag the - union as a vector also. This allows e.g. a union of two - vector types to show up automatically in "info vector". */ - if (TYPE_VECTOR (field_gdb_type)) - TYPE_VECTOR (union_gdb_type) = 1; - } + internal_error (__FILE__, __LINE__, + "Type \"%s\" has an unknown kind %d", + e->name.c_str (), e->kind); + } - return union_gdb_type; - } + void visit (const tdesc_type_vector *e) override + { + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; + + type *element_gdb_type = make_gdb_type (m_gdbarch, e->element_type); + m_type = init_vector_type (element_gdb_type, e->count); + TYPE_NAME (m_type) = xstrdup (e->name.c_str ()); + return; + } - type *make_gdb_type_flags (struct gdbarch *gdbarch) const - { - type *flags_gdb_type = arch_flags_type (gdbarch, this->name.c_str (), - this->size * TARGET_CHAR_BIT); + void visit (const tdesc_type_with_fields *e) override + { + m_type = tdesc_find_type (m_gdbarch, e->name.c_str ()); + if (m_type != NULL) + return; - for (const tdesc_type_field &f : this->fields) - { - int bitsize = f.end - f.start + 1; + switch (e->kind) + { + case TDESC_TYPE_STRUCT: + make_gdb_type_struct (e); + return; + case TDESC_TYPE_UNION: + make_gdb_type_union (e); + return; + case TDESC_TYPE_FLAGS: + make_gdb_type_flags (e); + return; + case TDESC_TYPE_ENUM: + make_gdb_type_enum (e); + return; + } - gdb_assert (f.type != NULL); - type *field_gdb_type = f.type->make_gdb_type (gdbarch); - append_flags_type_field (flags_gdb_type, f.start, bitsize, - field_gdb_type, f.name.c_str ()); - } + internal_error (__FILE__, __LINE__, + "Type \"%s\" has an unknown kind %d", + e->name.c_str (), e->kind); + } - return flags_gdb_type; - } + private: - type *make_gdb_type_enum (struct gdbarch *gdbarch) const - { - type *enum_gdb_type = arch_type (gdbarch, TYPE_CODE_ENUM, - this->size * TARGET_CHAR_BIT, - this->name.c_str ()); + void make_gdb_type_struct (const tdesc_type_with_fields *e) + { + m_type = arch_composite_type (m_gdbarch, NULL, TYPE_CODE_STRUCT); + TYPE_NAME (m_type) = xstrdup (e->name.c_str ()); + TYPE_TAG_NAME (m_type) = TYPE_NAME (m_type); - TYPE_UNSIGNED (enum_gdb_type) = 1; - for (const tdesc_type_field &f : this->fields) - { - struct field *fld - = append_composite_type_field_raw (enum_gdb_type, + for (const tdesc_type_field &f : e->fields) + { + if (f.start != -1 && f.end != -1) + { + /* Bitfield. */ + struct field *fld; + struct type *field_gdb_type; + int bitsize, total_size; + + /* This invariant should be preserved while creating types. */ + gdb_assert (e->size != 0); + if (f.type != NULL) + field_gdb_type = make_gdb_type (m_gdbarch, f.type); + else if (e->size > 4) + field_gdb_type = builtin_type (m_gdbarch)->builtin_uint64; + else + field_gdb_type = builtin_type (m_gdbarch)->builtin_uint32; + + fld = append_composite_type_field_raw + (m_type, xstrdup (f.name.c_str ()), field_gdb_type); + + /* For little-endian, BITPOS counts from the LSB of + the structure and marks the LSB of the field. For + big-endian, BITPOS counts from the MSB of the + structure and marks the MSB of the field. Either + way, it is the number of bits to the "left" of the + field. To calculate this in big-endian, we need + the total size of the structure. */ + bitsize = f.end - f.start + 1; + total_size = e->size * TARGET_CHAR_BIT; + if (gdbarch_bits_big_endian (m_gdbarch)) + SET_FIELD_BITPOS (fld[0], total_size - f.start - bitsize); + else + SET_FIELD_BITPOS (fld[0], f.start); + FIELD_BITSIZE (fld[0]) = bitsize; + } + else + { + gdb_assert (f.start == -1 && f.end == -1); + type *field_gdb_type = make_gdb_type (m_gdbarch, f.type); + append_composite_type_field (m_type, xstrdup (f.name.c_str ()), - NULL); + field_gdb_type); + } + } - SET_FIELD_BITPOS (fld[0], f.start); - } + if (e->size != 0) + TYPE_LENGTH (m_type) = e->size; + } - return enum_gdb_type; - } + void make_gdb_type_union (const tdesc_type_with_fields *e) + { + m_type = arch_composite_type (m_gdbarch, NULL, TYPE_CODE_UNION); + TYPE_NAME (m_type) = xstrdup (e->name.c_str ()); - type *make_gdb_type (struct gdbarch *gdbarch) const override - { - type *gdb_type = tdesc_find_type (gdbarch, this->name.c_str ()); - if (gdb_type != NULL) - return gdb_type; + for (const tdesc_type_field &f : e->fields) + { + type* field_gdb_type = make_gdb_type (m_gdbarch, f.type); + append_composite_type_field (m_type, xstrdup (f.name.c_str ()), + field_gdb_type); + + /* If any of the children of a union are vectors, flag the + union as a vector also. This allows e.g. a union of two + vector types to show up automatically in "info vector". */ + if (TYPE_VECTOR (field_gdb_type)) + TYPE_VECTOR (m_type) = 1; + } + } - switch (this->kind) + void make_gdb_type_flags (const tdesc_type_with_fields *e) { - case TDESC_TYPE_STRUCT: - return make_gdb_type_struct (gdbarch); - case TDESC_TYPE_UNION: - return make_gdb_type_union (gdbarch); - case TDESC_TYPE_FLAGS: - return make_gdb_type_flags (gdbarch); - case TDESC_TYPE_ENUM: - return make_gdb_type_enum (gdbarch); + m_type = arch_flags_type (m_gdbarch, e->name.c_str (), + e->size * TARGET_CHAR_BIT); + + for (const tdesc_type_field &f : e->fields) + { + int bitsize = f.end - f.start + 1; + + gdb_assert (f.type != NULL); + type *field_gdb_type = make_gdb_type (m_gdbarch, f.type); + append_flags_type_field (m_type, f.start, bitsize, + field_gdb_type, f.name.c_str ()); + } } - internal_error (__FILE__, __LINE__, - "Type \"%s\" has an unknown kind %d", - this->name.c_str (), this->kind); + void make_gdb_type_enum (const tdesc_type_with_fields *e) + { + m_type = arch_type (m_gdbarch, TYPE_CODE_ENUM, e->size * TARGET_CHAR_BIT, + e->name.c_str ()); - return NULL; - } + TYPE_UNSIGNED (m_type) = 1; + for (const tdesc_type_field &f : e->fields) + { + struct field *fld + = append_composite_type_field_raw (m_type, + xstrdup (f.name.c_str ()), + NULL); - std::vector<tdesc_type_field> fields; - int size; -}; + SET_FIELD_BITPOS (fld[0], f.start); + } + } + + /* The gdbarch used. */ + struct gdbarch *m_gdbarch; + + /* The type created. */ + type *m_type; + }; + + gdb_type_creator gdb_type (gdbarch); + ttype->accept (gdb_type); + return gdb_type.get_type (); +} /* A feature from a target description. Each feature is a collection of other elements, e.g. registers and types. */ @@ -1216,7 +1250,7 @@ tdesc_register_type (struct gdbarch *gdbarch, int regno) { /* First check for a predefined or target defined type. */ if (reg->tdesc_type) - arch_reg->type = reg->tdesc_type->make_gdb_type (gdbarch); + arch_reg->type = make_gdb_type (gdbarch, reg->tdesc_type); /* Next try size-sensitive type shortcuts. */ else if (reg->type == "float") ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Use visitors for make_gdb_type 2018-01-30 15:16 ` Alan Hayward @ 2018-02-05 16:21 ` Simon Marchi 0 siblings, 0 replies; 9+ messages in thread From: Simon Marchi @ 2018-02-05 16:21 UTC (permalink / raw) To: Alan Hayward, Yao Qi; +Cc: Philipp Rudo, Simon Marchi, gdb-patches, nd On 2018-01-30 10:15 AM, Alan Hayward wrote: > > >> On 29 Jan 2018, at 16:54, Yao Qi <qiyaoltc@gmail.com> wrote: >> >> On Mon, Jan 29, 2018 at 4:12 PM, Simon Marchi <simon.marchi@ericsson.com> wrote: >>> >>> That seems like unnecessary boilerplate to me. I really don't see why classes derived >>> from tdesc_element_visitor have to implement methods for nodes they don't care about. >>> >>> I added Yao in CC so he can chime in. >> >> When I wrote tdesc_element_visitor, in my mind, it is an interface, so >> I expect child >> class implement all the methods, because at that moment, all methods are needed, >> no empty methods. However, the situation changed a little bit, as per >> Alan's needs, >> part of the methods of tdesc_element_visitor are needed, and the rest of methods >> are empty somewhere. I don't mind converting tdesc_element_visitor into a base >> class which has all these methods empty as a default. That is fine to >> me. By the >> way, Alan's approach is fine to me as well :) >> > > Iâve removed the extra class and replaced with default implementations in > tdesc_element_visitor. > > All ok? > > > Tested on a make check on x86 targets=all build with target board unix native-gdbserver. > Built for power (because it does not use new target descriptions), but am unable to test. Hi Alan, This version LGTM. Thanks, Simon ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-02-05 16:21 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-26 15:14 [PATCH] Use visitors for make_gdb_type Alan Hayward 2018-01-26 15:30 ` Alan Hayward 2018-01-29 2:24 ` Simon Marchi 2018-01-29 9:30 ` Philipp Rudo 2018-01-29 15:31 ` Alan Hayward 2018-01-29 16:13 ` Simon Marchi 2018-01-29 16:54 ` Yao Qi 2018-01-30 15:16 ` Alan Hayward 2018-02-05 16:21 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).