diff mbox

[RFA] (riscv/ada) fix error when calling functions with range argument

Message ID 20190218233912.GK19092@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess Feb. 18, 2019, 11:39 p.m. UTC 
  * Tom Tromey <tom@tromey.com> [2019-02-17 07:25:57 -0700]:

> >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> Attached is a patch which does just that.
> 
> Joel> gdb/ChangeLog:
> 
> Joel>         * gdbtypes.c (type_align): Handle TYPE_CODE_RANGE the same as
> Joel>         integers and enumeration types.
> 
> Joel> Tested on x86_64-linux. Also tested on a variety of platforms
> Joel> (with CPUs being ARM, AArch64, Leon3 (SPARC-like), PowerPC,
> Joel> PowerPC64, RV64, Visium, x86, x86_64).
> 
> Joel> OK to push?
> 
> Yes, thanks for doing this.
> 
> I still have a to-do item to convert the various arch-specific
> type-alignment functions to use type_align.  In some cases I'm not sure
> how I'd test this though...

So I took a look at doing this switch for RISC-V, and the problem is
the handling of vectors.

The RISC-V alignment code contains this:

    case TYPE_CODE_ARRAY:
      if (TYPE_VECTOR (t))
	return std::min (TYPE_LENGTH (t), (unsigned) BIGGEST_ALIGNMENT);
      /* FALLTHROUGH */

    case TYPE_CODE_COMPLEX:
      return riscv_type_alignment (TYPE_TARGET_TYPE (t));

While the generic type_align just does this:

    case TYPE_CODE_ARRAY:
    case TYPE_CODE_COMPLEX:
    case TYPE_CODE_TYPEDEF:
      align = type_align (TYPE_TARGET_TYPE (type));
      break;

So, identical in the non-vector array case, but different in the
vector case.  The RISC-V vector handling is required, it fixes an
issue on RV64 in the gdb.base/gnu_vector.exp case.

I can see that at least ARM and AARCH64 also have different rules for
vectors than for arrays.

I wonder if we should change the relationship between 'type_align' and
'gdbarch_type_align'?

Currently 'gdbarch_type_align' is only called for "base" types, int,
float, etc.  But not compound types, arrays, structs, complex, etc.

What if instead, we always called 'gdbarch_type_align' from
'type_align' and we had 'default_type_align' return 0 for compound
types, and only provide an alignment for the "base" types.  Then
'type_align' would inspect the value returned from
'gdbarch_type_align', and if it sees 0, it breaks the compound type
up, and calls itself on the component type.

Below is a rough, untested patch of this idea, feedback welcome.

Thanks,
Andrew

Comments

Tom Tromey Feb. 19, 2019, 2:14 p.m. UTC | #1 
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> So I took a look at doing this switch for RISC-V, and the problem is
Andrew> the handling of vectors.
[...]
Andrew> I wonder if we should change the relationship between 'type_align' and
Andrew> 'gdbarch_type_align'?
[...]
Andrew> Below is a rough, untested patch of this idea, feedback welcome.

I think it is a good idea.  Thanks for looking into this.

I think it would be good to update the type_align documentation in
gdbarch.sh to explain what is expected, and in particular what returning
0 means.

Tom
diff mbox

Patch

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index d2e27d95558..86cfe3c7f5b 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -993,7 +993,35 @@  default_in_indirect_branch_thunk (gdbarch *gdbarch, CORE_ADDR pc)
 ULONGEST
 default_type_align (struct gdbarch *gdbarch, struct type *type)
 {
-  return type_length_units (check_typedef (type));
+  ULONGEST align;
+
+  switch (TYPE_CODE (type))
+    {
+    case TYPE_CODE_PTR:
+    case TYPE_CODE_FUNC:
+    case TYPE_CODE_FLAGS:
+    case TYPE_CODE_INT:
+    case TYPE_CODE_RANGE:
+    case TYPE_CODE_FLT:
+    case TYPE_CODE_ENUM:
+    case TYPE_CODE_REF:
+    case TYPE_CODE_RVALUE_REF:
+    case TYPE_CODE_CHAR:
+    case TYPE_CODE_BOOL:
+    case TYPE_CODE_DECFLOAT:
+    case TYPE_CODE_METHODPTR:
+    case TYPE_CODE_MEMBERPTR:
+      align = type_length_units (check_typedef (type));
+      break;
+    default:
+      /* Return 0 for more compound types, the type_align function in
+	 gdbtypes.c  will take care of breaking these types up and calling
+	 this method again on the component types.  */
+      align = 0;
+      break;
+    }
+
+  return align;
 }
 
 void
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index 0d293393dae..1d4515d02f3 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -2992,11 +2992,17 @@  type_raw_align (struct type *type)
 unsigned
 type_align (struct type *type)
 {
+  /* Check alignment provided in the debug information.  */
   unsigned raw_align = type_raw_align (type);
   if (raw_align != 0)
     return raw_align;
 
-  ULONGEST align = 0;
+  /* Allow the architecture to provide an alignment.  */
+  struct gdbarch *arch = get_type_arch (type);
+  ULONGEST align = gdbarch_type_align (arch, type);
+  if (align != 0)
+    return align;
+
   switch (TYPE_CODE (type))
     {
     case TYPE_CODE_PTR:
@@ -3013,10 +3019,7 @@  type_align (struct type *type)
     case TYPE_CODE_DECFLOAT:
     case TYPE_CODE_METHODPTR:
     case TYPE_CODE_MEMBERPTR:
-      {
-	struct gdbarch *arch = get_type_arch (type);
-	align = gdbarch_type_align (arch, type);
-      }
+      /* We have no further way to compute an alignment for this type.  */
       break;
 
     case TYPE_CODE_ARRAY: