[PATCH 2/3] Use type_instance_flags more throughout
Luis Machado
luis.machado@linaro.org
Tue Aug 25 11:36:33 GMT 2020
Hi,
On 8/21/20 11:45 AM, Pedro Alves wrote:
> The next patch in this series will rewrites enum_flags fixing some API
> holes. That would cause build failures around code using
> type_instance_flags. Or rather, that should be using it, but wasn't.
>
> This patch fixes it by using type_instance_flags throughout instead of
> plain integers.
>
> Note that we can't make the seemingly obvious change to struct
> type::instance_flags:
>
> - unsigned instance_flags : 9;
> + ENUM_BITFIELD (type_instance_flag_value) instance_flags : 9;
>
> Because G++ complains then that 9 bits isn't sufficient for holding
> all values of type_instance_flag_value.
I ran into the problem above the last time I used instance flags
(address classes), but mostly because of the (rather silly) 9 bit (or
whatever current number is there) limitation.
Isn't this more an artifact of trying to save space where there isn't a
need to anymore? Wouldn't a 64-bit integer suffice here? That would give
us 64 flags worth of data and virtually no problems until we ran out of
bits.
Honestly, I don't particularly like the hackery that is going on in
traits.h, just to catch something that may be easier fixed with a longer
integer type.
I understand the C++ standard is evolving, but GDB's code base is still
full of old code. We don't necessarily need to convert GDB's code base
to use the latest and greatest out there, right? We just need to make it
good quality and well maintained?
Just an opinion from someone that has used these flags before and has
been bitten by their storage-space-saving ugliness.
>
> So the patch adds a cast to TYPE_INSTANCE_FLAGS, and adds a separate
> SET_TYPE_INSTANCE_FLAGS macro.
>
> gdb/ChangeLog:
>
> * dwarf2/read.c (read_tag_pointer_type): Use type_instance_flags.
> * eval.c (fake_method::fake_method): Use SET_TYPE_INSTANCE_FLAGS.
> * gdbarch.h, gdbarch.c: Regenerate.
> * gdbarch.sh (address_class_type_flags): Use type_instance_flags.
> (address_class_name_to_type_flags): Use type_instance_flags and
> bool.
> * gdbtypes.c (address_space_name_to_int)
> (address_space_int_to_name, make_qualified_type): Use
> type_instance_flags.
> (make_qualified_type): Use type_instance_flags and
> SET_TYPE_INSTANCE_FLAGS.
> (make_type_with_address_space, make_cv_type, make_vector_type)
> (check_typedef): Use type_instance_flags.
> (recursive_dump_type): Cast type_instance_flags to unsigned for
> printing.
> (copy_type_recursive): Use SET_TYPE_INSTANCE_FLAGS.
> * gdbtypes.h (TYPE_INSTANCE_FLAGS): Return a type_instance_flags.
> (SET_TYPE_INSTANCE_FLAGS): New.
> (address_space_name_to_int, address_space_int_to_name)
> (make_type_with_address_space): Pass flags using
> type_instance_flags instead of int.
> * stabsread.c (cleanup_undefined_types_noname): Use
> SET_TYPE_INSTANCE_FLAGS.
> * type-stack.c (type_stack::follow_types): Use type_instance_flags.
> ---
> gdb/dwarf2/read.c | 7 +++----
> gdb/eval.c | 2 +-
> gdb/gdbarch.c | 6 +++---
> gdb/gdbarch.h | 12 ++++++------
> gdb/gdbarch.sh | 8 ++++----
> gdb/gdbtypes.c | 58 ++++++++++++++++++++++++++++++-------------------------
> gdb/gdbtypes.h | 15 +++++++++-----
> gdb/stabsread.c | 2 +-
> gdb/type-stack.c | 4 ++--
> 9 files changed, 62 insertions(+), 52 deletions(-)
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 0ac8533263..4ced5ac02b 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -17292,10 +17292,9 @@ read_tag_pointer_type (struct die_info *die, struct dwarf2_cu *cu)
> {
> if (gdbarch_address_class_type_flags_p (gdbarch))
> {
> - int type_flags;
> -
> - type_flags = gdbarch_address_class_type_flags
> - (gdbarch, byte_size, addr_class);
> + type_instance_flags type_flags
> + = gdbarch_address_class_type_flags (gdbarch, byte_size,
> + addr_class);
> gdb_assert ((type_flags & ~TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL)
> == 0);
> type = make_type_with_address_space (type, type_flags);
> diff --git a/gdb/eval.c b/gdb/eval.c
> index c62c35f318..cd300ddfef 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -659,7 +659,7 @@ fake_method::fake_method (type_instance_flags flags,
> TYPE_LENGTH (type) = 1;
> type->set_code (TYPE_CODE_METHOD);
> TYPE_CHAIN (type) = type;
> - TYPE_INSTANCE_FLAGS (type) = flags;
> + SET_TYPE_INSTANCE_FLAGS (type, flags);
> if (num_types > 0)
> {
> if (param_types[num_types - 1] == NULL)
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index f8fe03ca68..2be959ecfc 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -3501,7 +3501,7 @@ gdbarch_address_class_type_flags_p (struct gdbarch *gdbarch)
> return gdbarch->address_class_type_flags != NULL;
> }
>
> -int
> +type_instance_flags
> gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class)
> {
> gdb_assert (gdbarch != NULL);
> @@ -3566,8 +3566,8 @@ gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch)
> return gdbarch->address_class_name_to_type_flags != NULL;
> }
>
> -int
> -gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr)
> +bool
> +gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr)
> {
> gdb_assert (gdbarch != NULL);
> gdb_assert (gdbarch->address_class_name_to_type_flags != NULL);
> diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h
> index 7a3060e628..8a4a384fda 100644
> --- a/gdb/gdbarch.h
> +++ b/gdb/gdbarch.h
> @@ -848,8 +848,8 @@ extern void set_gdbarch_have_nonsteppable_watchpoint (struct gdbarch *gdbarch, i
>
> extern int gdbarch_address_class_type_flags_p (struct gdbarch *gdbarch);
>
> -typedef int (gdbarch_address_class_type_flags_ftype) (int byte_size, int dwarf2_addr_class);
> -extern int gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class);
> +typedef type_instance_flags (gdbarch_address_class_type_flags_ftype) (int byte_size, int dwarf2_addr_class);
> +extern type_instance_flags gdbarch_address_class_type_flags (struct gdbarch *gdbarch, int byte_size, int dwarf2_addr_class);
> extern void set_gdbarch_address_class_type_flags (struct gdbarch *gdbarch, gdbarch_address_class_type_flags_ftype *address_class_type_flags);
>
> extern int gdbarch_address_class_type_flags_to_name_p (struct gdbarch *gdbarch);
> @@ -866,13 +866,13 @@ extern bool gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdb_by
> extern void set_gdbarch_execute_dwarf_cfa_vendor_op (struct gdbarch *gdbarch, gdbarch_execute_dwarf_cfa_vendor_op_ftype *execute_dwarf_cfa_vendor_op);
>
> /* Return the appropriate type_flags for the supplied address class.
> - This function should return 1 if the address class was recognized and
> - type_flags was set, zero otherwise. */
> + This function should return true if the address class was recognized and
> + type_flags was set, false otherwise. */
>
> extern int gdbarch_address_class_name_to_type_flags_p (struct gdbarch *gdbarch);
>
> -typedef int (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
> -extern int gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, int *type_flags_ptr);
> +typedef bool (gdbarch_address_class_name_to_type_flags_ftype) (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr);
> +extern bool gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, const char *name, type_instance_flags *type_flags_ptr);
> extern void set_gdbarch_address_class_name_to_type_flags (struct gdbarch *gdbarch, gdbarch_address_class_name_to_type_flags_ftype *address_class_name_to_type_flags);
>
> /* Is a register in a group */
> diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh
> index 6d3c5c889d..7e9204119b 100755
> --- a/gdb/gdbarch.sh
> +++ b/gdb/gdbarch.sh
> @@ -689,16 +689,16 @@ v;int;cannot_step_breakpoint;;;0;0;;0
> # See comment in target.h about continuable, steppable and
> # non-steppable watchpoints.
> v;int;have_nonsteppable_watchpoint;;;0;0;;0
> -F;int;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
> +F;type_instance_flags;address_class_type_flags;int byte_size, int dwarf2_addr_class;byte_size, dwarf2_addr_class
> M;const char *;address_class_type_flags_to_name;int type_flags;type_flags
> # Execute vendor-specific DWARF Call Frame Instruction. OP is the instruction.
> # FS are passed from the generic execute_cfa_program function.
> m;bool;execute_dwarf_cfa_vendor_op;gdb_byte op, struct dwarf2_frame_state *fs;op, fs;;default_execute_dwarf_cfa_vendor_op;;0
>
> # Return the appropriate type_flags for the supplied address class.
> -# This function should return 1 if the address class was recognized and
> -# type_flags was set, zero otherwise.
> -M;int;address_class_name_to_type_flags;const char *name, int *type_flags_ptr;name, type_flags_ptr
> +# This function should return true if the address class was recognized and
> +# type_flags was set, false otherwise.
> +M;bool;address_class_name_to_type_flags;const char *name, type_instance_flags *type_flags_ptr;name, type_flags_ptr
> # Is a register in a group
> m;int;register_reggroup_p;int regnum, struct reggroup *reggroup;regnum, reggroup;;default_register_reggroup_p;;0
> # Fetch the pointer to the ith function argument.
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index c1eb03d898..64e44bfe23 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -574,11 +574,11 @@ lookup_function_type_with_arguments (struct type *type,
> /* Identify address space identifier by name --
> return the integer flag defined in gdbtypes.h. */
>
> -int
> +type_instance_flags
> address_space_name_to_int (struct gdbarch *gdbarch,
> const char *space_identifier)
> {
> - int type_flags;
> + type_instance_flags type_flags;
>
> /* Check for known address space delimiters. */
> if (!strcmp (space_identifier, "code"))
> @@ -598,7 +598,8 @@ address_space_name_to_int (struct gdbarch *gdbarch,
> gdbtypes.h -- return the string version of the adress space name. */
>
> const char *
> -address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
> +address_space_int_to_name (struct gdbarch *gdbarch,
> + type_instance_flags space_flag)
> {
> if (space_flag & TYPE_INSTANCE_FLAG_CODE_SPACE)
> return "code";
> @@ -617,7 +618,7 @@ address_space_int_to_name (struct gdbarch *gdbarch, int space_flag)
> STORAGE must be in the same obstack as TYPE. */
>
> static struct type *
> -make_qualified_type (struct type *type, int new_flags,
> +make_qualified_type (struct type *type, type_instance_flags new_flags,
> struct type *storage)
> {
> struct type *ntype;
> @@ -657,7 +658,7 @@ make_qualified_type (struct type *type, int new_flags,
> TYPE_CHAIN (type) = ntype;
>
> /* Now set the instance flags and return the new type. */
> - TYPE_INSTANCE_FLAGS (ntype) = new_flags;
> + SET_TYPE_INSTANCE_FLAGS (ntype, new_flags);
>
> /* Set length of new type to that of the original type. */
> TYPE_LENGTH (ntype) = TYPE_LENGTH (type);
> @@ -675,13 +676,14 @@ make_qualified_type (struct type *type, int new_flags,
> representations. */
>
> struct type *
> -make_type_with_address_space (struct type *type, int space_flag)
> +make_type_with_address_space (struct type *type,
> + type_instance_flags space_flag)
> {
> - int new_flags = ((TYPE_INSTANCE_FLAGS (type)
> - & ~(TYPE_INSTANCE_FLAG_CODE_SPACE
> - | TYPE_INSTANCE_FLAG_DATA_SPACE
> - | TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL))
> - | space_flag);
> + type_instance_flags new_flags = ((TYPE_INSTANCE_FLAGS (type)
> + & ~(TYPE_INSTANCE_FLAG_CODE_SPACE
> + | TYPE_INSTANCE_FLAG_DATA_SPACE
> + | TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL))
> + | space_flag);
>
> return make_qualified_type (type, new_flags, NULL);
> }
> @@ -705,9 +707,9 @@ make_cv_type (int cnst, int voltl,
> {
> struct type *ntype; /* New type */
>
> - int new_flags = (TYPE_INSTANCE_FLAGS (type)
> - & ~(TYPE_INSTANCE_FLAG_CONST
> - | TYPE_INSTANCE_FLAG_VOLATILE));
> + type_instance_flags new_flags = (TYPE_INSTANCE_FLAGS (type)
> + & ~(TYPE_INSTANCE_FLAG_CONST
> + | TYPE_INSTANCE_FLAG_VOLATILE));
>
> if (cnst)
> new_flags |= TYPE_INSTANCE_FLAG_CONST;
> @@ -1410,7 +1412,6 @@ void
> make_vector_type (struct type *array_type)
> {
> struct type *inner_array, *elt_type;
> - int flags;
>
> /* Find the innermost array type, in case the array is
> multi-dimensional. */
> @@ -1421,7 +1422,8 @@ make_vector_type (struct type *array_type)
> elt_type = TYPE_TARGET_TYPE (inner_array);
> if (elt_type->code () == TYPE_CODE_INT)
> {
> - flags = TYPE_INSTANCE_FLAGS (elt_type) | TYPE_INSTANCE_FLAG_NOTTEXT;
> + type_instance_flags flags
> + = TYPE_INSTANCE_FLAGS (elt_type) | TYPE_INSTANCE_FLAG_NOTTEXT;
> elt_type = make_qualified_type (elt_type, flags, NULL);
> TYPE_TARGET_TYPE (inner_array) = elt_type;
> }
> @@ -2732,12 +2734,13 @@ struct type *
> check_typedef (struct type *type)
> {
> struct type *orig_type = type;
> - /* While we're removing typedefs, we don't want to lose qualifiers.
> - E.g., const/volatile. */
> - int instance_flags = TYPE_INSTANCE_FLAGS (type);
>
> gdb_assert (type);
>
> + /* While we're removing typedefs, we don't want to lose qualifiers.
> + E.g., const/volatile. */
> + type_instance_flags instance_flags = TYPE_INSTANCE_FLAGS (type);
> +
> while (type->code () == TYPE_CODE_TYPEDEF)
> {
> if (!TYPE_TARGET_TYPE (type))
> @@ -2778,10 +2781,13 @@ check_typedef (struct type *type)
> outer cast in a chain of casting win), instead of assuming
> "it can't happen". */
> {
> - const int ALL_SPACES = (TYPE_INSTANCE_FLAG_CODE_SPACE
> - | TYPE_INSTANCE_FLAG_DATA_SPACE);
> - const int ALL_CLASSES = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
> - int new_instance_flags = TYPE_INSTANCE_FLAGS (type);
> + const type_instance_flags ALL_SPACES
> + = (TYPE_INSTANCE_FLAG_CODE_SPACE
> + | TYPE_INSTANCE_FLAG_DATA_SPACE);
> + const type_instance_flags ALL_CLASSES
> + = TYPE_INSTANCE_FLAG_ADDRESS_CLASS_ALL;
> + type_instance_flags new_instance_flags
> + = TYPE_INSTANCE_FLAGS (type);
>
> /* Treat code vs data spaces and address classes separately. */
> if ((instance_flags & ALL_SPACES) != 0)
> @@ -5026,7 +5032,7 @@ recursive_dump_type (struct type *type, int spaces)
> gdb_print_host_address (TYPE_CHAIN (type), gdb_stdout);
> printf_filtered ("\n");
> printfi_filtered (spaces, "instance_flags 0x%x",
> - TYPE_INSTANCE_FLAGS (type));
> + (unsigned) TYPE_INSTANCE_FLAGS (type));
> if (TYPE_CONST (type))
> {
> puts_filtered (" TYPE_CONST");
> @@ -5300,7 +5306,7 @@ copy_type_recursive (struct objfile *objfile,
> if (type->name ())
> new_type->set_name (xstrdup (type->name ()));
>
> - TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
> + SET_TYPE_INSTANCE_FLAGS (new_type, TYPE_INSTANCE_FLAGS (type));
> TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
>
> /* Copy the fields. */
> @@ -5427,7 +5433,7 @@ copy_type (const struct type *type)
> gdb_assert (TYPE_OBJFILE_OWNED (type));
>
> new_type = alloc_type_copy (type);
> - TYPE_INSTANCE_FLAGS (new_type) = TYPE_INSTANCE_FLAGS (type);
> + SET_TYPE_INSTANCE_FLAGS (new_type, TYPE_INSTANCE_FLAGS (type));
> TYPE_LENGTH (new_type) = TYPE_LENGTH (type);
> memcpy (TYPE_MAIN_TYPE (new_type), TYPE_MAIN_TYPE (type),
> sizeof (struct main_type));
> diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
> index 55a6dafb7e..b42cef6137 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -1585,7 +1585,10 @@ extern void allocate_gnat_aux_type (struct type *);
> TYPE_ZALLOC (type, \
> sizeof (*TYPE_MAIN_TYPE (type)->type_specific.func_stuff)))
>
> -#define TYPE_INSTANCE_FLAGS(thistype) (thistype)->instance_flags
> +#define TYPE_INSTANCE_FLAGS(thistype) \
> + type_instance_flags ((enum type_instance_flag_value) (thistype)->instance_flags)
> +#define SET_TYPE_INSTANCE_FLAGS(thistype, flags) \
> + (thistype)->instance_flags = flags
> #define TYPE_MAIN_TYPE(thistype) (thistype)->main_type
> #define TYPE_TARGET_TYPE(thistype) TYPE_MAIN_TYPE(thistype)->target_type
> #define TYPE_POINTER_TYPE(thistype) (thistype)->pointer_type
> @@ -2117,12 +2120,14 @@ extern struct type *make_atomic_type (struct type *);
>
> extern void replace_type (struct type *, struct type *);
>
> -extern int address_space_name_to_int (struct gdbarch *, const char *);
> +extern type_instance_flags address_space_name_to_int (struct gdbarch *,
> + const char *);
>
> -extern const char *address_space_int_to_name (struct gdbarch *, int);
> +extern const char *address_space_int_to_name (struct gdbarch *,
> + type_instance_flags);
>
> -extern struct type *make_type_with_address_space (struct type *type,
> - int space_identifier);
> +extern struct type *make_type_with_address_space
> + (struct type *type, type_instance_flags space_identifier);
>
> extern struct type *lookup_memberptr_type (struct type *, struct type *);
>
> diff --git a/gdb/stabsread.c b/gdb/stabsread.c
> index d2ff54a47b..ed31dc0111 100644
> --- a/gdb/stabsread.c
> +++ b/gdb/stabsread.c
> @@ -4397,7 +4397,7 @@ cleanup_undefined_types_noname (struct objfile *objfile)
> and needs to be copied over from the reference type.
> Since replace_type expects them to be identical, we need
> to set these flags manually before hand. */
> - TYPE_INSTANCE_FLAGS (nat.type) = TYPE_INSTANCE_FLAGS (*type);
> + SET_TYPE_INSTANCE_FLAGS (nat.type, TYPE_INSTANCE_FLAGS (*type));
> replace_type (nat.type, *type);
> }
> }
> diff --git a/gdb/type-stack.c b/gdb/type-stack.c
> index f8661d7565..608142c849 100644
> --- a/gdb/type-stack.c
> +++ b/gdb/type-stack.c
> @@ -109,7 +109,7 @@ type_stack::follow_types (struct type *follow_type)
> int done = 0;
> int make_const = 0;
> int make_volatile = 0;
> - int make_addr_space = 0;
> + type_instance_flags make_addr_space = 0;
> bool make_restrict = false;
> bool make_atomic = false;
> int array_size;
> @@ -128,7 +128,7 @@ type_stack::follow_types (struct type *follow_type)
> make_volatile = 1;
> break;
> case tp_space_identifier:
> - make_addr_space = pop_int ();
> + make_addr_space = (enum type_instance_flag_value) pop_int ();
> break;
> case tp_atomic:
> make_atomic = true;
>
More information about the Gdb-patches
mailing list