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

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

[Joel Brobecker]

From: KONRAD Frederic <konrad@adacore.com>

Hello,

This is a patch that one of my coworkers wrote, which I have been
meaning to contribute for a long time, but haven't because we are not
set up to run the official testsuite on this platform (because of
the way our baremetal compiler is set up). But since the patch is
quite straightforward in my opinion, I thought I would propose it
anyway.

Using the gdb.ada/call_pn.exp testcase, and running it by hand on
riscv64-elf, we get the following error:

    (gdb) call pn(55)
    Could not compute alignment of type

The problem occurs because the parameter's type is a TYPE_CODE_RANGE,
and that type code is not handled by riscv_type_alignment. So this patch
fixes the issue by handling TYPE_CODE_RANGE the same way we handle other
integral types.

gdb/ChangeLog:

        * gdb/riscv-rdep.c (riscv_type_alignment): Handle TYPE_CODE_RANGE.

Tested on riscv64-elf using AdaCore's testsuite.
OK to apply?

Thanks,
-- 
Joel

---
 gdb/riscv-tdep.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index fb5e2c5..3e8f564 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -1632,6 +1632,7 @@ riscv_type_alignment (struct type *t)
     default:
       error (_("Could not compute alignment of type"));
 
+    case TYPE_CODE_RANGE:
     case TYPE_CODE_RVALUE_REF:
     case TYPE_CODE_PTR:
     case TYPE_CODE_ENUM:
-- 
2.1.4

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

[Andrew Burgess]

* Joel Brobecker <brobecker@adacore.com> [2019-02-10 08:38:26 -0500]:

> From: KONRAD Frederic <konrad@adacore.com>
> 
> Hello,
> 
> This is a patch that one of my coworkers wrote, which I have been
> meaning to contribute for a long time, but haven't because we are not
> set up to run the official testsuite on this platform (because of
> the way our baremetal compiler is set up). But since the patch is
> quite straightforward in my opinion, I thought I would propose it
> anyway.
> 
> Using the gdb.ada/call_pn.exp testcase, and running it by hand on
> riscv64-elf, we get the following error:
> 
>     (gdb) call pn(55)
>     Could not compute alignment of type
> 
> The problem occurs because the parameter's type is a TYPE_CODE_RANGE,
> and that type code is not handled by riscv_type_alignment. So this patch
> fixes the issue by handling TYPE_CODE_RANGE the same way we handle other
> integral types.
> 
> gdb/ChangeLog:
> 
>         * gdb/riscv-rdep.c (riscv_type_alignment): Handle TYPE_CODE_RANGE.
> 
> Tested on riscv64-elf using AdaCore's testsuite.
> OK to apply?

I'm happy for this to go in.

Thanks,
Andrew



> 
> Thanks,
> -- 
> Joel
> 
> ---
>  gdb/riscv-tdep.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index fb5e2c5..3e8f564 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -1632,6 +1632,7 @@ riscv_type_alignment (struct type *t)
>      default:
>        error (_("Could not compute alignment of type"));
>  
> +    case TYPE_CODE_RANGE:
>      case TYPE_CODE_RVALUE_REF:
>      case TYPE_CODE_PTR:
>      case TYPE_CODE_ENUM:
> -- 
> 2.1.4
> 

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

[Joel Brobecker]

> > gdb/ChangeLog:
> > 
> >         * gdb/riscv-rdep.c (riscv_type_alignment): Handle TYPE_CODE_RANGE.
> > 
> > Tested on riscv64-elf using AdaCore's testsuite.
> > OK to apply?
> 
> I'm happy for this to go in.

Thanks Andrew. I know there is the option of switching over to using
type_align, but since this shouldn't cause any extra work to have
this one in, I pushed it for now.

-- 
Joel

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

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> gdb/ChangeLog:
Joel>         * gdb/riscv-rdep.c (riscv_type_alignment): Handle TYPE_CODE_RANGE.
Joel> Tested on riscv64-elf using AdaCore's testsuite.
Joel> OK to apply?

I don't have any issue with this, but I do wonder if
riscv_type_alignment can be removed and/or simplified in favor
type_align and the gdbarch method.

I see several ports have this issue.  type_align should be preferred,
IMO, because it respects any additional alignment specified in the
DWARF.  I assume there are latent bugs in function calling on various
platforms caused by this, though I haven't checked.

Tom

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

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> I don't have any issue with this, but I do wonder if
Tom> riscv_type_alignment can be removed and/or simplified in favor
Tom> type_align and the gdbarch method.

Also now I wonder whether the TYPE_CODE_RANGE case in
gdbtypes.c:type_align ought to be changed.  Right now it just returns 0,
which means "cannot be determined".

Tom

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

[Joel Brobecker]

> Tom> I don't have any issue with this, but I do wonder if
> Tom> riscv_type_alignment can be removed and/or simplified in favor
> Tom> type_align and the gdbarch method.
> 
> Also now I wonder whether the TYPE_CODE_RANGE case in
> gdbtypes.c:type_align ought to be changed.  Right now it just returns 0,
> which means "cannot be determined".

Makes sense to me. I don't see this as being any problem at all, but
just in case, Let's make the change internally at AdaCore, and
have it be evaluated on all our platforms.

-- 
Joel

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

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

> > Tom> I don't have any issue with this, but I do wonder if
> > Tom> riscv_type_alignment can be removed and/or simplified in favor
> > Tom> type_align and the gdbarch method.
> > 
> > Also now I wonder whether the TYPE_CODE_RANGE case in
> > gdbtypes.c:type_align ought to be changed.  Right now it just returns 0,
> > which means "cannot be determined".
> 
> Makes sense to me. I don't see this as being any problem at all, but
> just in case, Let's make the change internally at AdaCore, and
> have it be evaluated on all our platforms.

Attached is a patch which does just that.

gdb/ChangeLog:

        * gdbtypes.c (type_align): Handle TYPE_CODE_RANGE the same as
        integers and enumeration types.

Tested on x86_64-linux. Also tested on a variety of platforms
(with CPUs being ARM, AArch64, Leon3 (SPARC-like), PowerPC,
PowerPC64, RV64, Visium, x86, x86_64).

OK to push?
-- 
Joel

[-- Attachment #2: 0001-type_align-handle-range-types-the-same-as-ints-and-e.patch --]
[-- Type: text/x-diff, Size: 1381 bytes --]

From c6cec5f43eefc87120ae148fc3edf695a345438d Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Sun, 17 Feb 2019 07:40:43 -0500
Subject: [PATCH] type_align: handle range types the same as ints and enums

This commit enhances type_align to handle TYPE_CODE_RANGE types
the same as integers and enums, rather than returning zero,
which means for this function that it could not determine its
alignment.

gdb/ChangeLog:

	* gdbtypes.c (type_align): Handle TYPE_CODE_RANGE the same as
        integers and enumeration types.

Tested on x86_64-linux. Also tested on a variety of platforms
(with CPUs being ARM, AArch64, Leon3 (SPARC-like), PowerPC,
PowerPC64, RV64, Visium, x86, x86_64).
---
 gdb/gdbtypes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index d1ca304..6758783 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3003,6 +3003,7 @@ type_align (struct type *type)
     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:
@@ -3047,7 +3048,6 @@ type_align (struct type *type)
       break;
 
     case TYPE_CODE_SET:
-    case TYPE_CODE_RANGE:
     case TYPE_CODE_STRING:
       /* Not sure what to do here, and these can't appear in C or C++
 	 anyway.  */
-- 
2.1.4

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

[Tom Tromey]

>>>>> "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...

Tom

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

[Joel Brobecker]

> 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.

Thank you. Pushed to master.

> 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...

Besides the build bots that exist, the only architectures I think
you can reasonably test are the architectures that we have at AdaCore.
Testing the change with AdaCore's testsuite isn't going to guaranty
no regression, but it's better than nothing. If there are specific
maintainers for those arches, we can ask them too. For the others,
maybe what we could do is just compare the two implementations as
best we can, and see if their code has any deviation, and if they
do, check the ABI...

-- 
Joel

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

[Andrew Burgess]

* 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


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:

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

[Tom Tromey]

>>>>> "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

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

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2019-02-12 09:54:13 -0700]:

> >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> gdb/ChangeLog:
> Joel>         * gdb/riscv-rdep.c (riscv_type_alignment): Handle TYPE_CODE_RANGE.
> Joel> Tested on riscv64-elf using AdaCore's testsuite.
> Joel> OK to apply?
> 
> I don't have any issue with this, but I do wonder if
> riscv_type_alignment can be removed and/or simplified in favor
> type_align and the gdbarch method.
> 
> I see several ports have this issue.  type_align should be preferred,
> IMO, because it respects any additional alignment specified in the
> DWARF.  I assume there are latent bugs in function calling on various
> platforms caused by this, though I haven't checked.

I'll test changing RISC-V over to using gdbtypes.c:type_align.

Thanks,
Andrew