[PATCH 1/2] gdb: simplify code in check_typedef

[PATCH 1/2] gdb: simplify code in check_typedef

[Tankut Baris Aktemur]

Simplify a code portion in check_typedef where the conditions are
unnecessary.  Also remove the comment that says "treat address spaces
and address classes separately", because since the commit 92fdad7
"gdb: convert type instance flags to bitfields", they are separate
fields; so, the comment does not look useful.
---
 gdb/gdbtypes.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 9098727959e..ad401872941 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3062,12 +3062,8 @@ check_typedef (struct type *type)
 	 "it can't happen".  */
       {
 	type_instance_flags new_instance_flags = type->instance_flags ();
-
-	/* Treat code vs data spaces and address classes separately.  */
-	if (instance_flags.harvard_aspace != HARVARD_ASPACE_NONE)
-	  new_instance_flags.harvard_aspace = HARVARD_ASPACE_NONE;
-	if (instance_flags.address_class != 0)
-	  new_instance_flags.address_class = 0;
+	new_instance_flags.harvard_aspace = HARVARD_ASPACE_NONE;
+	new_instance_flags.address_class = 0;
 
 	instance_flags |= new_instance_flags;
       }
-- 
2.53.0

[PATCH 2/2] gdb: preserve lhs type's address spaces/classes in check_typedef

[Tankut Baris Aktemur]

In commit 92fdad7 "gdb: convert type instance flags to bitfields",
`operator|=` of type_instance_flags required the address space and
address class values of left-hand-side to be zero.  This introduced
the following bug (thanks to Keith Seitz for reporting it at
https://inbox.sourceware.org/gdb-patches/053e90c0-53f0-4748-9d27-0237b9f21221@redhat.com/T/#u):

  typedef int myint;

  (gdb) ptype (@code myint) 3
  ../../src/gdb/gdbtypes.h:127: internal-error: operator|=: Assertion
  `harvard_aspace == 0' failed.
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  ----- Backtrace -----
  0x5bb1d1 gdb_internal_backtrace_1
          ../../src/gdb/bt-utils.c:122
  0x5bb210 _Z22gdb_internal_backtracev
          ../../src/gdb/bt-utils.c:173
  0xdfbbaa internal_vproblem
          ../../src/gdb/utils.c:434
  0xdfbf45 _Z15internal_verrorPKciS0_P13__va_list_tag
          ../../src/gdb/utils.c:514
  0x162763f _Z18internal_error_locPKciS0_z
          ../../src/gdbsupport/errors.cc:57
  0x87e8e6 _ZN19type_instance_flagsoRERKS_
          ../../src/gdb/gdbtypes.h:127
  0x875d71 _Z13check_typedefP4type
          ../../src/gdb/gdbtypes.c:3072

The |= operator is used in check_typedef as follows:

      /* Preserve the instance flags as we traverse down the typedef chain.

         Handling address spaces/classes is nasty, what do we do if there's a
         conflict?
         E.g., what if an outer typedef marks the type as class_1 and an inner
         typedef marks the type as class_2?
         This is the wrong place to do such error checking.  We leave it to
         the code that created the typedef in the first place to flag the
         error.  We just pick the outer address space (akin to letting the
         outer cast in a chain of casting win), instead of assuming
         "it can't happen".  */
      {
        type_instance_flags new_instance_flags = type->instance_flags ();
        new_instance_flags.harvard_aspace = HARVARD_ASPACE_NONE;
        new_instance_flags.address_class = 0;

        instance_flags |= new_instance_flags;
      }

So, the assertion in operator|= was applied on the wrong operand.  The
outer type, which is the left-hand-side in this case, should preserve
its values whereas the right-hand-side should have 0 as its address
space and address class values.

Fix the bug by updating the assertion.

Furthermore, rename operator|= to "merge".  Type instance flags are no
longer stored as a bitmask value, but rather as a struct.  Having an
operator like |= gives the wrong impression that we are doing a
bitmask OR.  Using a method makes the intention clearer.

Include a regression test.
---
 gdb/gdbtypes.c                            |  2 +-
 gdb/gdbtypes.h                            | 14 +++++++-------
 gdb/testsuite/gdb.cp/typedef-operator.exp |  3 +++
 3 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index ad401872941..66c00da389a 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3065,7 +3065,7 @@ check_typedef (struct type *type)
 	new_instance_flags.harvard_aspace = HARVARD_ASPACE_NONE;
 	new_instance_flags.address_class = 0;
 
-	instance_flags |= new_instance_flags;
+	instance_flags.merge (new_instance_flags);
       }
     }
 
diff --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h
index eda09641248..f8d7ed5ab91 100644
--- a/gdb/gdbtypes.h
+++ b/gdb/gdbtypes.h
@@ -119,21 +119,21 @@ struct type_instance_flags
     return !(*this == other);
   }
 
-  type_instance_flags &operator|= (const type_instance_flags &other)
+  /* Merge OTHER flags to THIS, except address spaces and address
+     classes.  Require OTHER's address space and address class to be
+     zero, and preserve the address space and address class values of
+     THIS.  */
+  void merge (const type_instance_flags &other)
   {
     is_const = is_const || other.is_const;
     is_volatile = is_volatile || other.is_volatile;
 
-    gdb_assert (harvard_aspace == 0);
-    harvard_aspace = other.harvard_aspace;
-
-    gdb_assert (address_class == 0);
-    address_class = other.address_class;
+    gdb_assert (other.harvard_aspace == HARVARD_ASPACE_NONE);
+    gdb_assert (other.address_class == 0);
 
     is_nottext = is_nottext || other.is_nottext;
     is_restrict = is_restrict || other.is_restrict;
     is_atomic = is_atomic || other.is_atomic;
-    return *this;
   }
 
   /* Constant type.  If this is set, the corresponding type has a
diff --git a/gdb/testsuite/gdb.cp/typedef-operator.exp b/gdb/testsuite/gdb.cp/typedef-operator.exp
index 9513b3725dc..e03502769a4 100644
--- a/gdb/testsuite/gdb.cp/typedef-operator.exp
+++ b/gdb/testsuite/gdb.cp/typedef-operator.exp
@@ -32,3 +32,6 @@ if {![runto_main]} {
 }
 
 gdb_test "p *v" " = 42" "test typedef"
+
+# Test that address spaces are preserved.
+gdb_test "ptype (@data D)u" "type = @data class C .*"
-- 
2.53.0

Re: [PATCH 1/2] gdb: simplify code in check_typedef

[Keith Seitz]

Hi,

On 7/27/26 2:20 AM, Tankut Baris Aktemur wrote:
> Simplify a code portion in check_typedef where the conditions are
> unnecessary.  Also remove the comment that says "treat address spaces
> and address classes separately", because since the commit 92fdad7
> "gdb: convert type instance flags to bitfields", they are separate
> fields; so, the comment does not look useful.

Thank you for the quick turnaround for such a, well, minor
corner case. It is very appreciated.

One concern I still have (maybe this is no longer an issue?):
the old logic (and the remaining comment in check_typedef) is outer-wins
on conflict, otherwise inherit from the inner typedef.

Always clearing the RHS and having `merge` never take aspace/class from
it fixes the outer-qualified case, but drops an aspace that appears only
on an inner typedef.

How about encoding outer-wins in `merge` instead, and dropping the 
clearing in check_typedef? That would preserve the previous behavior
(and match the comment).

For example:

type_instance_flags::merge:

    if (harvard_aspace == HARVARD_ASPACE_NONE)
      harvard_aspace = other.harvard_aspace;
    if (address_class == 0)
      address_class = other.address_class;

and check_typedef:

    instance_flags.merge (type->instance_flags ());

I completely agree with the naming change to `merge'. That is
certainly clearer for me.

Thanks,
Keith