diff mbox series

[05/16] gdb: inline address_space_{name, type_instance_flags}_to_{type_instance_flags, name}

Message ID 20260713-users-aktemur-type-instance-flags-v1-5-779cad0c85ec@amd.com
State New
Headers
Series Rewrite type instance flags as a struct with bitfields |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed

Commit Message

Aktemur, Baris July 13, 2026, 2 p.m. UTC 
  This is yet another refactoring step to treat Harvard address space
ids and address class ids separately and transparently from the fact
that they are stored in type instance flags.

The function 'address_space_name_to_type_instance_flags' converts
address space and address class names to type instance flags.  It
deals with the Harvard address space names "code" and "data" as well
as architecture specific address class names.  As a result, it may
produce type instance flags where either the Harvard address space
bits or the address class bits are set.  The function
'address_space_type_instance_flags_to_name' does the conversion in the
opposite direction.

Inline the functions and remove them.  This is a step towards
separating the handling of two concepts.

In type-stack.c, which is used by the parser to convert user inputs
into types, "@code" and "@data" and architecture-specific address
class names are treated the same, too, blurring the difference and
their storage in type instance flags.  While we inline the use of
address_space_name_to_type_instance_flags there, we also separate the
two topics by defining different tokens.  The patch still pushes type
instance flags into the type stack.  The subsequent patch will further
clean this up to store address class and address space ids.

A hardcoded shift operation will go away in a future patch.
---
 gdb/c-typeprint.c                                  | 21 ++++++++--
 gdb/gdbtypes.c                                     | 46 ----------------------
 gdb/gdbtypes.h                                     |  6 ---
 gdb/testsuite/gdb.base/address_space_qualifier.exp |  4 +-
 gdb/type-stack.c                                   | 37 +++++++++++++++--
 gdb/type-stack.h                                   | 31 +++++++++------
 6 files changed, 71 insertions(+), 74 deletions(-)

Comments

Tom Tromey July 21, 2026, 6:29 p.m. UTC | #1 
>>>>> Tankut Baris Aktemur <tankutbaris.aktemur@amd.com> writes:

> Inline the functions and remove them.  This is a step towards
> separating the handling of two concepts.

> In type-stack.c, which is used by the parser to convert user inputs
> into types, "@code" and "@data" and architecture-specific address
> class names are treated the same, too, blurring the difference and
> their storage in type instance flags.  While we inline the use of
> address_space_name_to_type_instance_flags there, we also separate the
> two topics by defining different tokens.  The patch still pushes type
> instance flags into the type stack.  The subsequent patch will further
> clean this up to store address class and address space ids.

On the one hand, this seems like a mild step backward in the sense that
if we ever wanted to support these things in non-C languages, it would
have to be reimplemented there.  OTOH, nobody has seen fit to do that in
the last 20 years or whatever it is.

> +  enum type_pieces piece;
> +  int int_val;
> +  unsigned int aclass;
> +
> +  /* Check for Harvard address space delimiters.  */
> +  if (streq (string, "code"))
> +    {
> +      piece = tp_harvard_aspace_identifier;
> +      int_val = TYPE_INSTANCE_FLAG_CODE_SPACE;
> +    }
> +  else if (streq (string, "data"))
> +    {
> +      piece = tp_harvard_aspace_identifier;
> +      int_val = TYPE_INSTANCE_FLAG_DATA_SPACE;
> +    }
> +  else if (gdbarch_address_class_name_to_id_p (gdbarch)
> +	   && gdbarch_address_class_name_to_id (gdbarch,
> +						string,
> +						aclass))
> +    {
> +      piece = tp_aclass_identifier;
> +      int_val = (enum type_instance_flag_value) (aclass << 4);
> +    }
> +  else
> +    error (_("Unknown address space/class specifier: \"%s\""), string);
> +
> +  element.piece = piece;
>    insert_into (slot, element);
> -  element.int_val
> -    = address_space_name_to_type_instance_flags (gdbarch, string);
> +  element.int_val = int_val;

I think the various 'if' branches might as well just assign directly to
element.*; and 'aclass' can be moved into the if like

  else if (unsigned int aclass = 0;
           gdbarch_address_class_name_to_id_p (...)
           && ...)

I didn't read the whole series yet but if we're going to use different
words, as is done in that error message, then some spot in the manual
ought to explain this.

Tom
Aktemur, Baris July 22, 2026, 10:29 a.m. UTC | #2 
AMD General

Hi Tom,

Thank you for your review.

On Tuesday, July 21, 2026 8:30 PM, Tom Tromey wrote:
...
> > +  enum type_pieces piece;
> > +  int int_val;
> > +  unsigned int aclass;
> > +
> > +  /* Check for Harvard address space delimiters.  */
> > +  if (streq (string, "code"))
> > +    {
> > +      piece = tp_harvard_aspace_identifier;
> > +      int_val = TYPE_INSTANCE_FLAG_CODE_SPACE;
> > +    }
> > +  else if (streq (string, "data"))
> > +    {
> > +      piece = tp_harvard_aspace_identifier;
> > +      int_val = TYPE_INSTANCE_FLAG_DATA_SPACE;
> > +    }
> > +  else if (gdbarch_address_class_name_to_id_p (gdbarch)
> > +        && gdbarch_address_class_name_to_id (gdbarch,
> > +                                             string,
> > +                                             aclass))
> > +    {
> > +      piece = tp_aclass_identifier;
> > +      int_val = (enum type_instance_flag_value) (aclass << 4);
> > +    }
> > +  else
> > +    error (_("Unknown address space/class specifier: \"%s\""), string);
> > +
> > +  element.piece = piece;
> >    insert_into (slot, element);
> > -  element.int_val
> > -    = address_space_name_to_type_instance_flags (gdbarch, string);
> > +  element.int_val = int_val;
>
> I think the various 'if' branches might as well just assign directly to
> element.*; and 'aclass' can be moved into the if like
>
>   else if (unsigned int aclass = 0;
>            gdbarch_address_class_name_to_id_p (...)
>            && ...)

I had chosen to write it the way I posted because we're inserting
two elements back-to-back, which are of different union kinds.
Looking at the code again based on your comments, I think we can
do better if we have an overload of the insert_into method.  I'm
adding a new refactoring patch and this patch will look better
based on that, IMHO.  Please check in v2.

>
> I didn't read the whole series yet but if we're going to use different
> words, as is done in that error message, then some spot in the manual
> ought to explain this.

I went through the GDB manual, and I don't see address classes or Harvard
address spaces described anywhere.  The built-in `@code` and `@data` syntax
is also not explained.  This seems to be a gap in the documentation.

I can write a small section for this, but let me do that in a separate
submission.

Regards,
-Baris
Tom Tromey July 22, 2026, 1:38 p.m. UTC | #3 
>>>>> Aktemur, Baris <TankutBaris.Aktemur@amd.com> writes:

>> I didn't read the whole series yet but if we're going to use different
>> words, as is done in that error message, then some spot in the manual
>> ought to explain this.

> I went through the GDB manual, and I don't see address classes or Harvard
> address spaces described anywhere.  The built-in `@code` and `@data` syntax
> is also not explained.  This seems to be a gap in the documentation.

> I can write a small section for this, but let me do that in a separate
> submission.

Sure, if you don't mind, that would be nice to have.

I wonder if @code and @data are actually ever used.

Tom
diff mbox series

Patch

diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index aaace85aa29..e3096a7859b 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -19,6 +19,7 @@ 
 #include "event-top.h"
 #include "bfd.h"
 #include "symtab.h"
+#include "gdbarch.h"
 #include "gdbtypes.h"
 #include "expression.h"
 #include "value.h"
@@ -482,10 +483,22 @@  c_type_print_modifier (struct type *type, struct ui_file *stream,
       did_print_modifier = 1;
     }
 
-  address_space_id
-    = address_space_type_instance_flags_to_name (type->arch (),
-						 type->instance_flags ());
-  if (address_space_id)
+  address_space_id = nullptr;
+
+  if (TYPE_CODE_SPACE (type))
+    address_space_id = "code";
+  else if (TYPE_DATA_SPACE (type))
+    address_space_id = "data";
+  else
+    {
+      unsigned int aclass = TYPE_ADDRESS_CLASS (type);
+      if (aclass != 0
+	  && gdbarch_address_class_id_to_name_p (type->arch ()))
+	address_space_id = gdbarch_address_class_id_to_name (type->arch (),
+							     aclass);
+    }
+
+  if (address_space_id != nullptr)
     {
       if (did_print_modifier || need_pre_space)
 	gdb_printf (stream, " ");
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index f972a1f4278..97f1b3e1417 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -536,52 +536,6 @@  lookup_function_type_with_arguments (struct type *return_type,
   return create_function_type (return_type, nparams, param_types);
 }
 
-/* Identify address space identifier by name -- return a
-   type_instance_flags.  */
-
-type_instance_flags
-address_space_name_to_type_instance_flags (struct gdbarch *gdbarch,
-					   const char *space_identifier)
-{
-  /* Check for known address space delimiters.  */
-  if (streq (space_identifier, "code"))
-    return TYPE_INSTANCE_FLAG_CODE_SPACE;
-  else if (streq (space_identifier, "data"))
-    return TYPE_INSTANCE_FLAG_DATA_SPACE;
-
-  unsigned int aclass;
-  if (gdbarch_address_class_name_to_id_p (gdbarch)
-      && gdbarch_address_class_name_to_id (gdbarch,
-					   space_identifier,
-					   aclass))
-    {
-      return (enum type_instance_flag_value) (aclass << 4);
-    }
-  else
-    error (_("Unknown address space specifier: \"%s\""), space_identifier);
-}
-
-/* Identify address space identifier by type_instance_flags and return
-   the string version of the address space name.  */
-
-const char *
-address_space_type_instance_flags_to_name (struct gdbarch *gdbarch,
-					   type_instance_flags space_flag)
-{
-  if (space_flag & TYPE_INSTANCE_FLAG_CODE_SPACE)
-    return "code";
-  else if (space_flag & TYPE_INSTANCE_FLAG_DATA_SPACE)
-    return "data";
-
-  unsigned int aclass = TYPE_ADDRESS_CLASS_FROM_INSTANCE_FLAGS (space_flag);
-
-  if (aclass != 0
-      && gdbarch_address_class_id_to_name_p (gdbarch))
-    return gdbarch_address_class_id_to_name (gdbarch, aclass);
-  else
-    return NULL;
-}
-
 /* Create a new type with instance flags NEW_FLAGS, based on TYPE.
 
    If STORAGE is non-NULL, create the new type instance there.
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index 3c21d4ad214..1d5c96ab2c1 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -2420,12 +2420,6 @@  extern struct type *make_atomic_type (struct type *);
 
 extern void replace_type (struct type *, struct type *);
 
-extern type_instance_flags address_space_name_to_type_instance_flags
-  (struct gdbarch *, const char *);
-
-extern const char *address_space_type_instance_flags_to_name
-  (struct gdbarch *, type_instance_flags);
-
 extern struct type *make_type_with_address_space
   (struct type *type, type_instance_flags space_identifier);
 
diff --git a/gdb/testsuite/gdb.base/address_space_qualifier.exp b/gdb/testsuite/gdb.base/address_space_qualifier.exp
index 49437dad60d..3e94e895493 100644
--- a/gdb/testsuite/gdb.base/address_space_qualifier.exp
+++ b/gdb/testsuite/gdb.base/address_space_qualifier.exp
@@ -22,12 +22,12 @@  gdb_test_no_output "set language c"
 
 with_test_prefix "C" {
 	gdb_test "p *(@somerandomqualifiername int *) 0x12345678" \
-		"Unknown address space specifier: \"somerandomqualifiername\""
+		"Unknown address space/class specifier: \"somerandomqualifiername\""
 }
 
 gdb_test_no_output "set language c++"
 
 with_test_prefix "C++" {
 	gdb_test "p *(@somerandomqualifiername int *) 0x12345678" \
-		"Unknown address space specifier: \"somerandomqualifiername\""
+		"Unknown address space/class specifier: \"somerandomqualifiername\""
 }
diff --git a/gdb/type-stack.c b/gdb/type-stack.c
index 7e790a003ec..e2783eaab11 100644
--- a/gdb/type-stack.c
+++ b/gdb/type-stack.c
@@ -20,6 +20,7 @@ 
 #include "type-stack.h"
 
 #include "gdbtypes.h"
+#include "gdbarch.h"
 
 /* See type-stack.h.  */
 
@@ -63,10 +64,35 @@  type_stack::insert (struct gdbarch *gdbarch, const char *string)
   else
     slot = 0;
 
-  element.piece = tp_space_identifier;
+  enum type_pieces piece;
+  int int_val;
+  unsigned int aclass;
+
+  /* Check for Harvard address space delimiters.  */
+  if (streq (string, "code"))
+    {
+      piece = tp_harvard_aspace_identifier;
+      int_val = TYPE_INSTANCE_FLAG_CODE_SPACE;
+    }
+  else if (streq (string, "data"))
+    {
+      piece = tp_harvard_aspace_identifier;
+      int_val = TYPE_INSTANCE_FLAG_DATA_SPACE;
+    }
+  else if (gdbarch_address_class_name_to_id_p (gdbarch)
+	   && gdbarch_address_class_name_to_id (gdbarch,
+						string,
+						aclass))
+    {
+      piece = tp_aclass_identifier;
+      int_val = (enum type_instance_flag_value) (aclass << 4);
+    }
+  else
+    error (_("Unknown address space/class specifier: \"%s\""), string);
+
+  element.piece = piece;
   insert_into (slot, element);
-  element.int_val
-    = address_space_name_to_type_instance_flags (gdbarch, string);
+  element.int_val = int_val;
   insert_into (slot, element);
 }
 
@@ -125,7 +151,10 @@  type_stack::follow_types (struct type *follow_type)
       case tp_volatile:
 	make_volatile = 1;
 	break;
-      case tp_space_identifier:
+      case tp_harvard_aspace_identifier:
+	make_addr_space = (enum type_instance_flag_value) pop_int ();
+	break;
+      case tp_aclass_identifier:
 	make_addr_space = (enum type_instance_flag_value) pop_int ();
 	break;
       case tp_atomic:
diff --git a/gdb/type-stack.h b/gdb/type-stack.h
index 318ef715b2e..85e07549ee9 100644
--- a/gdb/type-stack.h
+++ b/gdb/type-stack.h
@@ -45,9 +45,12 @@  enum type_pieces
   tp_function_with_arguments,
   tp_const,
   tp_volatile,
-  /* An address space identifier.  The address space is also pushed on
+  /* An Harvard address space identifier (i.e. "code" or "data").  The
+     address space is also pushed on the stack.  */
+  tp_harvard_aspace_identifier,
+  /* An address class identifier.  The address class is also pushed on
      the stack.  */
-  tp_space_identifier,
+  tp_aclass_identifier,
   tp_atomic,
   tp_restrict,
   /* A separate type stack, which is also pushed onto this type
@@ -114,7 +117,8 @@  struct type_stack
      accept an integer argument are allowed.  */
   void push (enum type_pieces tp, int n)
   {
-    gdb_assert (tp == tp_array || tp == tp_space_identifier || tp == tp_kind);
+    gdb_assert (tp == tp_array || tp == tp_harvard_aspace_identifier
+		|| tp == tp_aclass_identifier || tp == tp_kind);
     type_stack_elt elt;
     elt.int_val = n;
     m_elements.push_back (elt);
@@ -172,7 +176,8 @@  struct type_stack
     type_stack_elt elt = m_elements.back ();
     m_elements.pop_back ();
     type_pieces tp = elt.piece;
-    gdb_assert (tp == tp_array || tp == tp_space_identifier || tp == tp_kind);
+    gdb_assert (tp == tp_array || tp == tp_harvard_aspace_identifier
+		|| tp == tp_aclass_identifier || tp == tp_kind);
     elt = m_elements.back ();
     m_elements.pop_back ();
     return elt.int_val;
@@ -204,13 +209,14 @@  struct type_stack
     return elt.stack_val;
   }
 
-  /* Insert a tp_space_identifier and the corresponding address space
-     value into the stack.  STRING is the name of an address space, as
-     recognized by address_space_name_to_type_instance_flags.  If the
-     stack is empty, the new elements are simply pushed.  If the stack
-     is not empty, this function assumes that the first item on the
-     stack is a tp_pointer, and the new values are inserted above the
-     first item.  */
+  /* Insert an address space or address class identifier and the
+     corresponding id value into the stack.  STRING is the name of a
+     Harvard address space ("code" or "data"), or the name of an
+     address class as recognized by gdbarch_address_class_name_to_id.
+     If the stack is empty, the new elements are simply pushed.  If
+     the stack is not empty, this function assumes that the first item
+     on the stack is a tp_pointer, and the new values are inserted
+     above the first item.  */
 
   void insert (struct gdbarch *gdbarch, const char *string);
 
@@ -252,7 +258,8 @@  struct type_stack
   {
     return (tp == tp_array || tp == tp_kind || tp == tp_type_stack
 	    || tp == tp_function_with_arguments
-	    || tp == tp_space_identifier);
+	    || tp == tp_harvard_aspace_identifier
+	    || tp == tp_aclass_identifier);
   }