[PATCH] s390: Implement 'type_align' gdbarch method

[PATCH] s390: Implement 'type_align' gdbarch method

[Andreas Arnez]

The align.exp test case yields many FAILs on s390x, since GDB's _Alignoff
doesn't always agree with the compiler's.  On s390x, the maximum alignment
is 8, but GDB returns an alignment of 16 for 16-byte data types such as
"long double".

This is fixed by implementing the type_align gdbarch method.  The new
method returns an alignment of 8 for all integer, floating-point, and
vector types larger than 8 bytes.  With this change, all align.exp tests
pass.

gdb/ChangeLog:

	* s390-tdep.c (s390_type_align): New function.
	(s390_gdbarch_init): Set it as type_align gdbarch method.
---
 gdb/s390-tdep.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index 4b931017a8..871efc59bc 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -52,6 +52,37 @@ constexpr gdb_byte s390_break_insn[] = { 0x0, 0x1 };
 
 typedef BP_MANIPULATION (s390_break_insn) s390_breakpoint;
 
+/* Types.  */
+
+/* Implement the gdbarch type alignment method.  */
+
+static ULONGEST
+s390_type_align (gdbarch *gdbarch, struct type *t)
+{
+  t = check_typedef (t);
+
+  if (TYPE_LENGTH (t) > 8)
+    {
+      switch (TYPE_CODE (t))
+	{
+	case TYPE_CODE_INT:
+	case TYPE_CODE_RANGE:
+	case TYPE_CODE_FLT:
+	case TYPE_CODE_ENUM:
+	case TYPE_CODE_CHAR:
+	case TYPE_CODE_BOOL:
+	case TYPE_CODE_DECFLOAT:
+	  return 8;
+
+	case TYPE_CODE_ARRAY:
+	  if (TYPE_VECTOR (t))
+	    return 8;
+	  break;
+	}
+    }
+  return 0;
+}
+
 /* Decoding S/390 instructions.  */
 
 /* Read a single instruction from address AT.  */
@@ -6944,6 +6975,8 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_long_double_bit (gdbarch, 128);
   set_gdbarch_long_double_format (gdbarch, floatformats_ia64_quad);
 
+  set_gdbarch_type_align (gdbarch, s390_type_align);
+
   /* Breakpoints.  */
   /* Amount PC must be decremented by after a breakpoint.  This is
      often the number of bytes returned by gdbarch_breakpoint_from_pc but not
-- 
2.17.0

Re: [PATCH] s390: Implement 'type_align' gdbarch method

[Tom Tromey]

>>>>> "Andreas" == Andreas Arnez <arnez@linux.ibm.com> writes:

Andreas> gdb/ChangeLog:

Andreas> 	* s390-tdep.c (s390_type_align): New function.
Andreas> 	(s390_gdbarch_init): Set it as type_align gdbarch method.

FWIW this looks reasonable to me.

Tom

Re: [PATCH] s390: Implement 'type_align' gdbarch method

[Andreas Arnez]

On Thu, Aug 08 2019, Tom Tromey wrote:

>>>>>> "Andreas" == Andreas Arnez <arnez@linux.ibm.com> writes:
>
> Andreas> gdb/ChangeLog:
>
> Andreas> 	* s390-tdep.c (s390_type_align): New function.
> Andreas> 	(s390_gdbarch_init): Set it as type_align gdbarch method.
>
> FWIW this looks reasonable to me.

Thanks, pushed as git commit 1022c627db.

--
Andreas

Re: [PATCH] s390: Implement 'type_align' gdbarch method

[Tom de Vries]

On 08-08-19 13:40, Andreas Arnez wrote:
> The align.exp test case yields many FAILs on s390x, since GDB's _Alignoff
> doesn't always agree with the compiler's.  On s390x, the maximum alignment
> is 8, but GDB returns an alignment of 16 for 16-byte data types such as
> "long double".
> 
> This is fixed by implementing the type_align gdbarch method.  The new
> method returns an alignment of 8 for all integer, floating-point, and
> vector types larger than 8 bytes.  With this change, all align.exp tests
> pass.
> 

Hi,

in the "zSeries ELF Application Binary Interface Supplement" document I
find long double listed with 16-byte size and alignment.

Likewise in the "IBM XL C/C++ for Linux on z Systems Optimization and
Programming Guide".

So I wonder, is this patch hardcoding the assumptions of a single
compiler implementation (gcc) in gdb, thereby possibly breaking
functionality in gdb when debugging executables generated by other
compilers?

If so, ISTM the correct way to fix this is to get gcc to emit the
non-standard type alignment in the debug info.

Thanks,
- Tom

Re: [PATCH] s390: Implement 'type_align' gdbarch method

[Ulrich Weigand]

Tom de Vries wrote:

> in the "zSeries ELF Application Binary Interface Supplement" document I
> find long double listed with 16-byte size and alignment.
> 
> Likewise in the "IBM XL C/C++ for Linux on z Systems Optimization and
> Programming Guide".
> 
> So I wonder, is this patch hardcoding the assumptions of a single
> compiler implementation (gcc) in gdb, thereby possibly breaking
> functionality in gdb when debugging executables generated by other
> compilers?

This was an error in the original ABI document, unfortunately.
We are currently working on an updated ABI document that will
fix this (and several other errors).  Andreas should know the
current status of this update.

To my knowledge, every Linux on Z compiler in existance has
implemented the 8-byte alignment for long double.  (This is
certainly true for GCC and LLVM; I cannot check XL C since
this is no longer supported on Linux.)

There is in fact a good reason for having (at most) 8-byte
alignment for all standard types: the ABI only guarantees that
the incoming stack pointer is 8-byte aligned.  Having any larger
alignment requirement on a standard type would basically force
compilers to implement dynamic stack realignment.

(If I were to re-design the ABI from scratch today, that
would certainly be one of the things I'd do differently
-- but we are where we are.)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH] s390: Implement 'type_align' gdbarch method

[Tom de Vries]

On 30-08-19 17:16, Ulrich Weigand wrote:
> Tom de Vries wrote:
> 
>> in the "zSeries ELF Application Binary Interface Supplement" document I
>> find long double listed with 16-byte size and alignment.
>>
>> Likewise in the "IBM XL C/C++ for Linux on z Systems Optimization and
>> Programming Guide".
>>
>> So I wonder, is this patch hardcoding the assumptions of a single
>> compiler implementation (gcc) in gdb, thereby possibly breaking
>> functionality in gdb when debugging executables generated by other
>> compilers?
> 
> This was an error in the original ABI document, unfortunately.
> We are currently working on an updated ABI document that will
> fix this (and several other errors).  Andreas should know the
> current status of this update.
> 
> To my knowledge, every Linux on Z compiler in existance has
> implemented the 8-byte alignment for long double.  (This is
> certainly true for GCC and LLVM; I cannot check XL C since
> this is no longer supported on Linux.)
> 
> There is in fact a good reason for having (at most) 8-byte
> alignment for all standard types: the ABI only guarantees that
> the incoming stack pointer is 8-byte aligned.  Having any larger
> alignment requirement on a standard type would basically force
> compilers to implement dynamic stack realignment.
> 
> (If I were to re-design the ABI from scratch today, that
> would certainly be one of the things I'd do differently
> -- but we are where we are.)

I understand it now, thanks for the detailed explanation.

- Tom