public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/2] AArch64 AAPCS: Improve argument passing
@ 2019-01-16 15:57 Alan Hayward
  2019-01-16 15:57 ` [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++ Alan Hayward
  2019-01-16 15:57 ` [PATCH 2/2] AArch64 AAPCS: Ignore static members Alan Hayward
  0 siblings, 2 replies; 12+ messages in thread
From: Alan Hayward @ 2019-01-16 15:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: nd, Alan Hayward

This series improves the AAPCS argument passing on AArch64 (where small
structures are passed via float registers instead of on the stack).

The first patch ensures empty structures in C++ are dealt with correctly.
The second deals with static struct members.

It replaces the patch
"AArch64: Prevent infinite recursion when checking AAPCS types",
as the test failure is covered by the above. However that patch may still
be required in the future if I can craft a valid test case for it.

This patch adds a new test and expands an exisiting test. When run on
x86_64, many of the new test cases fail with:
amd64-tdep.c:958: internal-error: CORE_ADDR amd64_push_arguments(regcache*,
int, value**, CORE_ADDR, function_call_return_method): Assertion
`!"Unexpected register class."' failed
I had a look, but I don't know enough about the x86 argument passing.

Alan.


Alan Hayward (2):
  AArch64 AAPCS: Empty structs have non zero size in C++
  AArch64 AAPCS: Ignore static members

 gdb/aarch64-tdep.c                            |  16 ++
 .../gdb.base/infcall-nested-structs.exp       |  42 +++-
 .../gdb.cp/infcall-nested-static-structs.cc   | 222 ++++++++++++++++++
 .../gdb.cp/infcall-nested-static-structs.exp  | 173 ++++++++++++++
 4 files changed, 441 insertions(+), 12 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc
 create mode 100644 gdb/testsuite/gdb.cp/infcall-nested-static-structs.exp

-- 
2.17.2 (Apple Git-113)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++
  2019-01-16 15:57 [PATCH 0/2] AArch64 AAPCS: Improve argument passing Alan Hayward
@ 2019-01-16 15:57 ` Alan Hayward
  2019-01-17 17:08   ` Pedro Alves
  2019-01-16 15:57 ` [PATCH 2/2] AArch64 AAPCS: Ignore static members Alan Hayward
  1 sibling, 1 reply; 12+ messages in thread
From: Alan Hayward @ 2019-01-16 15:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: nd, Alan Hayward

When gdb.base/infcall-nested-structs.c is complied as C++, the structs
containing empty structs are no longer passed via float arguments.

This is because structs in C++ have a minimum size of 1.  This can then
cause padding in the struct, which is disallowed for AAPCS.

Add padding checks to AArch64 and add C++ compile variant to the test.

gdb/ChangeLog:

2019-01-16  Alan Hayward  <alan.hayward@arm.com>

	* aarch64-tdep.c (aapcs_is_vfp_call_or_return_candidate_1): Check
	for padding.

gdb/testsuite/ChangeLog:

2019-01-16  Alan Hayward  <alan.hayward@arm.com>

	* gdb.base/infcall-nested-structs.exp: Test C++ in addition to C.
---
 gdb/aarch64-tdep.c                            |  8 ++++
 .../gdb.base/infcall-nested-structs.exp       | 42 +++++++++++++------
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 407adc8a42..ff6ca5c6e0 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1392,6 +1392,14 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
 	      return -1;
 	    count += sub_count;
 	  }
+
+	/* Ensure there is no padding between the fields (allowing for empty
+	   zero length structs)  */
+	int ftype_length = (*fundamental_type == nullptr)
+			   ? 0 : TYPE_LENGTH (*fundamental_type);
+	if (count * ftype_length != TYPE_LENGTH (type))
+	  return -1;
+
 	return count;
       }
 
diff --git a/gdb/testsuite/gdb.base/infcall-nested-structs.exp b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
index b10e6d21eb..543702ac9a 100644
--- a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
+++ b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
@@ -24,6 +24,20 @@ if [target_info exists gdb,cannot_call_functions] {
     continue
 }
 
+# Only test C++ if we are able.  Always use C.
+if { [skip_cplus_tests] || [get_compiler_info "c++"] } {
+    set lang {c}
+} else {
+    set lang {c c++}
+}
+
+foreach l $lang {
+    set dir "$l"
+    remote_exec build "rm -rf [standard_output_file ${dir}]"
+    remote_exec build "mkdir -p [standard_output_file ${dir}]"
+}
+
+
 set int_types { tc ts ti tl tll }
 set float_types { tf td tld }
 set complex_types { tfc tdc tldc }
@@ -44,7 +58,7 @@ proc I2A { n } {
 # types of the struct fields within the source.  Run up to main.
 # Also updates the global "testfile" to reflect the most recent build.
 
-proc start_nested_structs_test { types } {
+proc start_nested_structs_test { lang types } {
     global testfile
     global srcfile
     global binfile
@@ -53,9 +67,11 @@ proc start_nested_structs_test { types } {
     global compile_flags
 
     standard_testfile .c
+    set dir "$lang"
 
     # Create the additional flags
     set flags $compile_flags
+    lappend flags $lang
 
     for {set n 0} {$n<[llength ${types}]} {incr n} {
 	set m [I2A ${n}]
@@ -64,7 +80,7 @@ proc start_nested_structs_test { types } {
 	append testfile "-" "$t"
     }
 
-    set binfile [standard_output_file ${testfile}]
+    set binfile [standard_output_file ${dir}/${testfile}]
     if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "${flags}"] != "" } {
 	unresolved "failed to compile"
 	return 0
@@ -125,48 +141,50 @@ proc run_tests {} {
 # Set up a test prefix, compile the test binary, run to main, and then
 # run some tests.
 
-proc start_gdb_and_run_tests { types } {
+proc start_gdb_and_run_tests { lang types } {
     set prefix "types"
 
     foreach t $types {
 	append prefix "-" "${t}"
     }
 
-    with_test_prefix $prefix {
-	if { [start_nested_structs_test $types] } {
-	    run_tests
+    foreach_with_prefix l $lang {
+	with_test_prefix $prefix {
+	    if { [start_nested_structs_test $l $types] } {
+		run_tests
+	    }
 	}
     }
 }
 
 foreach ta $int_types {
-    start_gdb_and_run_tests $ta
+    start_gdb_and_run_tests $lang $ta
 }
 
 if [support_complex_tests] {
     foreach ta $complex_types {
-	start_gdb_and_run_tests $ta
+	start_gdb_and_run_tests $lang $ta
     }
 }
 
 if ![gdb_skip_float_test] {
     foreach ta $float_types {
-	start_gdb_and_run_tests $ta
+	start_gdb_and_run_tests $lang $ta
     }
 
     foreach ta $int_types {
 	foreach tb $float_types {
-	    start_gdb_and_run_tests [list $ta $tb]
+	    start_gdb_and_run_tests $lang [list $ta $tb]
 	}
     }
 
     foreach ta $float_types {
 	foreach tb $int_types {
-	    start_gdb_and_run_tests [list $ta $tb]
+	    start_gdb_and_run_tests $lang [list $ta $tb]
 	}
 
 	foreach tb $float_types {
-	    start_gdb_and_run_tests [list $ta $tb]
+	    start_gdb_and_run_tests $lang [list $ta $tb]
 	}
     }
 }
-- 
2.17.2 (Apple Git-113)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/2] AArch64 AAPCS: Ignore static members
  2019-01-16 15:57 [PATCH 0/2] AArch64 AAPCS: Improve argument passing Alan Hayward
  2019-01-16 15:57 ` [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++ Alan Hayward
@ 2019-01-16 15:57 ` Alan Hayward
  2019-01-17 17:22   ` Pedro Alves
  1 sibling, 1 reply; 12+ messages in thread
From: Alan Hayward @ 2019-01-16 15:57 UTC (permalink / raw)
  To: gdb-patches; +Cc: nd, Alan Hayward

Static members in C++ structs are global data and therefore not part of the
list of struct members considered for passing in registers.

Note the corresponding code in GCC (from which the GDB AAPCS code is based)
does not have any static member checks due to the static members not being
part of the struct type at that point.

Add a new test based on gdb.base/infcall-nested-structs.exp, adding static
members.

Also fixes gdb.dwarf2/dw2-cp-infcall-ref-static.exp.

gdb/ChangeLog:

2019-01-16  Alan Hayward  <alan.hayward@arm.com>

	* aarch64-tdep.c (aapcs_is_vfp_call_or_return_candidate_1): Check
	for static members.
	(pass_in_v_vfp_candidate): Likewise.

gdb/testsuite/ChangeLog:

2019-01-16  Alan Hayward  <alan.hayward@arm.com>

	* gdb.cp/infcall-nested-static-structs.cc: New test.
	* gdb.cp/infcall-nested-static-structs.exp: New file.
---
 gdb/aarch64-tdep.c                            |   8 +
 .../gdb.cp/infcall-nested-static-structs.cc   | 222 ++++++++++++++++++
 .../gdb.cp/infcall-nested-static-structs.exp  | 173 ++++++++++++++
 3 files changed, 403 insertions(+)
 create mode 100644 gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc
 create mode 100644 gdb/testsuite/gdb.cp/infcall-nested-static-structs.exp

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index ff6ca5c6e0..863e62b5e0 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1384,6 +1384,10 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
 
 	for (int i = 0; i < TYPE_NFIELDS (type); i++)
 	  {
+	    /* Ignore any static fields.  */
+	    if (field_is_static (&TYPE_FIELD (type, i)))
+	      continue;
+
 	    struct type *member = check_typedef (TYPE_FIELD_TYPE (type, i));
 
 	    int sub_count = aapcs_is_vfp_call_or_return_candidate_1
@@ -1662,6 +1666,10 @@ pass_in_v_vfp_candidate (struct gdbarch *gdbarch, struct regcache *regcache,
     case TYPE_CODE_UNION:
       for (int i = 0; i < TYPE_NFIELDS (arg_type); i++)
 	{
+	  /* Don't include static fields.  */
+	  if (field_is_static (&TYPE_FIELD (arg_type, i)))
+	    continue;
+
 	  struct value *field = value_primitive_field (arg, 0, i, arg_type);
 	  struct type *field_type = check_typedef (value_type (field));
 
diff --git a/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc b/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc
new file mode 100644
index 0000000000..bec63728f4
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc
@@ -0,0 +1,222 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This file is used for testing GDBs ability to pass structures to, and
+   return structures from, functions, where those structs contain static
+   members.  This test is based on gdb.base/infcall-nested-structs.exp.  */
+
+#include <string.h>
+
+/* Useful abreviations.  */
+typedef char tc;
+typedef short ts;
+typedef int ti;
+typedef long tl;
+typedef long long tll;
+typedef float tf;
+typedef double td;
+typedef long double tld;
+
+#ifdef TEST_COMPLEX
+typedef float _Complex tfc;
+typedef double _Complex tdc;
+typedef long double _Complex tldc;
+#endif /* TEST_COMPLEX */
+
+#define MAKE_CHECK_FUNCS(TYPE)					\
+  int								\
+  check_arg_ ## TYPE (struct TYPE arg)				\
+  {								\
+    return cmp_ ## TYPE (arg, ref_val_ ## TYPE);		\
+  }								\
+								\
+  struct TYPE							\
+  rtn_str_ ## TYPE (void)					\
+  {								\
+    return (ref_val_ ## TYPE);					\
+  }
+
+#define REF_VAL(NAME) struct NAME ref_val_ ## NAME
+#define ES(NAME) struct { } NAME
+
+/* Test is either for a single type or two differing types.  */
+#if defined tA && ! defined tB
+#define tB tA
+#endif
+#if ! defined tB
+#error "Incorrect configuration of tA and tB defines"
+#endif
+
+/* Structures with two fields nested to various depths, one of which is static,
+   along with some empty structures.  */
+struct struct_02_01 { struct sa { struct sb { tA a; static tB b; } s1; } s2; };
+struct struct_02_02 { static tA a; struct { struct { ES(es1); } s1; } s2; tB b; };
+struct struct_02_03 { struct { struct { ES(es1); } s1; } s2; ES(es1); struct { struct { tA a; } s3; } s4; struct sa { struct sb { static tB b; } s5; } s6;};
+struct struct_02_04 { static tA a; tB b; };
+
+/* Structures with four fields nested to various depths, some of which are
+   static, along with some empty structures.  */
+struct struct_04_01 { struct sa { struct sb { static tA a; tB b; tA c; tB d; } s1; } s2; };
+struct struct_04_02 { tA a; struct { struct { ES(es1); } s1; } s2; tB b; struct { struct { ES(es1); } s2; } s3; static tA c; struct { struct { ES(es2); } s4; } s5; static tB d;};
+struct struct_04_03 { struct sa { struct sb { static tA a; } s3; } s4; struct sc { struct sd { static tB b; } s5; } s6; struct se { struct sf { static tA c; } s7; } s8; struct sg { struct sh { static tB d; } s9; } s10;};
+struct struct_04_04 { ES(es1); ES(es2); tA a; ES(es3); tB b; ES(es4); tA c; ES(es5); static tB d; };
+
+/* Structures with six fields nested to various depths, some of which are
+   static, along with some empty structures.  */
+struct struct_06_01 { struct sa { struct sb { tA a; static tB b; tA c; tB d; tA e; } s1; } s2; tB f; };
+struct struct_06_02 { tA a; static tB b; static tA c; tB d; tB e; tA f;};
+struct struct_06_03 { struct { struct { ES(es1); } s1; } s2; ES(es1); struct sa { struct sb { static tA a; } s3; } s4; struct sc { struct sd { tB b; } s5; } s6; struct se { struct sf { static tA c; } s7; } s8; struct sg { struct sh { static tB d; } s9; } s10; struct { struct { tA e; tB f; } s11; } s12;};
+struct struct_06_04 { ES(es1); ES(es2); static tA a; ES(es3); static tB b; ES(es4); static tA c; ES(es5); static tB d; ES(es6); static tA e; ES(es7); tB f; };
+
+int cmp_struct_02_01 (struct struct_02_01 a,
+			     struct struct_02_01 b)
+{ return a.s2.s1.a == b.s2.s1.a && a.s2.s1.b == a.s2.s1.b; }
+
+int cmp_struct_02_02 (struct struct_02_02 a,
+			     struct struct_02_02 b)
+{ return a.a == b.a && a.b == b.b; }
+
+int cmp_struct_02_03 (struct struct_02_03 a,
+			     struct struct_02_03 b)
+{ return a.s4.s3.a == b.s4.s3.a && a.s6.s5.b == b.s6.s5.b; }
+
+int cmp_struct_02_04 (struct struct_02_04 a,
+			     struct struct_02_04 b)
+{ return a.a == b.a && a.b == b.b; }
+
+int cmp_struct_04_01 (struct struct_04_01 a, struct struct_04_01 b)
+{ return a.s2.s1.a == b.s2.s1.a && a.s2.s1.b == a.s2.s1.b
+	 && a.s2.s1.c == b.s2.s1.c && a.s2.s1.d == a.s2.s1.d; }
+
+int cmp_struct_04_02 (struct struct_04_02 a, struct struct_04_02 b)
+{ return a.a == b.a && a.b == b.b && a.c == b.c && a.d == b.d; }
+
+int cmp_struct_04_03 (struct struct_04_03 a, struct struct_04_03 b)
+{ return a.s4.s3.a == b.s4.s3.a && a.s6.s5.b == b.s6.s5.b
+	 && a.s8.s7.c == b.s8.s7.c && a.s10.s9.d == b.s10.s9.d; }
+
+int cmp_struct_04_04 (struct struct_04_04 a, struct struct_04_04 b)
+{ return a.a == b.a && a.b == b.b && a.c == b.c && a.d == b.d; }
+
+int cmp_struct_06_01 (struct struct_06_01 a, struct struct_06_01 b)
+{ return a.s2.s1.a == b.s2.s1.a && a.s2.s1.b == a.s2.s1.b
+	 && a.s2.s1.c == b.s2.s1.c && a.s2.s1.d == a.s2.s1.d
+	 && a.s2.s1.e == b.s2.s1.e && a.f == b.f; }
+
+int cmp_struct_06_02 (struct struct_06_02 a, struct struct_06_02 b)
+{ return a.a == b.a && a.b == b.b && a.c == b.c && a.d == b.d && a.e == b.e
+	 && a.f == b.f; }
+
+int cmp_struct_06_03 (struct struct_06_03 a, struct struct_06_03 b)
+{ return a.s4.s3.a == b.s4.s3.a && a.s6.s5.b == b.s6.s5.b
+	 && a.s8.s7.c == b.s8.s7.c && a.s10.s9.d == b.s10.s9.d
+	 && a.s12.s11.e == b.s12.s11.e && a.s12.s11.f == b.s12.s11.f; }
+
+int cmp_struct_06_04 (struct struct_06_04 a, struct struct_06_04 b)
+{ return a.a == b.a && a.b == b.b && a.c == b.c && a.d == b.d && a.e == b.e
+	 && a.f == b.f; }
+
+/* Initialise static members.  */
+tB struct_02_01::sa::sb::b = '1';
+tA struct_02_02::a = '2';
+tB struct_02_03::sa::sb::b = '3';
+tA struct_02_04::a = '4';
+tA struct_04_01::sa::sb::a = '5';
+tA struct_04_02::c = '6';
+tB struct_04_02::d = '7';
+tA struct_04_03::sa::sb::a = '8';
+tB struct_04_03::sc::sd::b = '9';
+tA struct_04_03::se::sf::c = '0';
+tB struct_04_03::sg::sh::d = 'A';
+tB struct_04_04::d = 'B';
+tB struct_06_01::sa::sb::b = 'C';
+tB struct_06_02::b = 'D';
+tA struct_06_02::c = 'E';
+tA struct_06_03::sa::sb::a = 'F';
+tA struct_06_03::se::sf::c = 'G';
+tB struct_06_03::sg::sh::d = 'H';
+tA struct_06_04::a = 'I';
+tB struct_06_04::b = 'J';
+tA struct_06_04::c = 'K';
+tB struct_06_04::d = 'L';
+tA struct_06_04::e = 'M';
+
+REF_VAL(struct_02_01) = { { { 'a' } } };
+REF_VAL(struct_02_02) = { { { {} } }, 'b' };
+REF_VAL(struct_02_03) = { { { {} } }, {}, { { 'a' } }, { { } } };
+REF_VAL(struct_02_04) = { 'b' };
+REF_VAL(struct_04_01) = { { { 'b', 'c', 'd' } } };
+REF_VAL(struct_04_02) = { 'a', { { {} } }, 'b', { { {} } }, { { {} } } };
+REF_VAL(struct_04_03) = {};
+REF_VAL(struct_04_04) = { {}, {}, 'a', {}, 'b', {}, 'c', {} };
+REF_VAL(struct_06_01) = { { { 'a', 'c', 'd', 'e' } }, 'f' };
+REF_VAL(struct_06_02) = { 'a', 'd', 'e', 'f' };
+REF_VAL(struct_06_03) = { { { {} } }, {}, {}, { { 'b' } }, {}, /*{ { 'e', 'f' } }*/ };
+REF_VAL(struct_06_04) = { {}, {}, {}, {}, {}, {}, {}, 'f' };
+
+/* Create all of the functions GDB will call to check functionality.  */
+MAKE_CHECK_FUNCS(struct_02_01)
+MAKE_CHECK_FUNCS(struct_02_02)
+MAKE_CHECK_FUNCS(struct_02_03)
+MAKE_CHECK_FUNCS(struct_02_04)
+MAKE_CHECK_FUNCS(struct_04_01)
+MAKE_CHECK_FUNCS(struct_04_02)
+MAKE_CHECK_FUNCS(struct_04_03)
+MAKE_CHECK_FUNCS(struct_04_04)
+MAKE_CHECK_FUNCS(struct_06_01)
+MAKE_CHECK_FUNCS(struct_06_02)
+MAKE_CHECK_FUNCS(struct_06_03)
+MAKE_CHECK_FUNCS(struct_06_04)
+
+#define CALL_LINE(NAME) val += check_arg_ ## NAME (rtn_str_ ## NAME ())
+
+int
+call_all ()
+{
+  int val;
+
+  CALL_LINE(struct_02_01);
+  CALL_LINE(struct_02_02);
+  CALL_LINE(struct_02_03);
+  CALL_LINE(struct_02_04);
+  CALL_LINE(struct_04_01);
+  CALL_LINE(struct_04_02);
+  CALL_LINE(struct_04_03);
+  CALL_LINE(struct_04_04);
+  CALL_LINE(struct_06_01);
+  CALL_LINE(struct_06_02);
+  CALL_LINE(struct_06_03);
+  CALL_LINE(struct_06_04);
+
+  return (val != 4);
+}
+
+void
+breakpt (void)
+{
+  /* Nothing.  */
+}
+
+int
+main ()
+{
+  int res;
+
+  res = call_all ();
+  breakpt (); /* Break Here.  */
+  return res;
+}
diff --git a/gdb/testsuite/gdb.cp/infcall-nested-static-structs.exp b/gdb/testsuite/gdb.cp/infcall-nested-static-structs.exp
new file mode 100644
index 0000000000..23d8466be5
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/infcall-nested-static-structs.exp
@@ -0,0 +1,173 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2019 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Some targets can't call functions, so don't even bother with this
+# test.
+if [target_info exists gdb,cannot_call_functions] {
+    unsupported "this target can not call functions"
+    continue
+}
+
+# static members in structs/classes require C++
+if [get_compiler_info "c++"] {
+    untested "couldn't find a valid c++ compiler"
+    return -1
+}
+
+set int_types { tc ts ti tl tll }
+set float_types { tf td tld }
+set complex_types { tfc tdc tldc }
+
+set compile_flags {debug}
+if [support_complex_tests] {
+    lappend compile_flags "additional_flags=-DTEST_COMPLEX"
+}
+
+# Given N (0..25), return the corresponding alphabetic letter in upper
+# case.
+
+proc I2A { n } {
+    return [string range "ABCDEFGHIJKLMNOPQRSTUVWXYZ" $n $n]
+}
+
+# Compile a variant of nested-structs.cc using TYPES to specify the
+# types of the struct fields within the source.  Run up to main.
+# Also updates the global "testfile" to reflect the most recent build.
+
+proc start_nested_structs_test { types } {
+    global testfile
+    global srcfile
+    global binfile
+    global subdir
+    global srcdir
+    global compile_flags
+
+    standard_testfile .cc
+
+    # Create the additional flags
+    set flags $compile_flags
+
+    for {set n 0} {$n<[llength ${types}]} {incr n} {
+	set m [I2A ${n}]
+	set t [lindex ${types} $n]
+	lappend flags "additional_flags=-Dt${m}=${t}"
+	append testfile "-" "$t"
+    }
+
+    set binfile [standard_output_file ${testfile}]
+    if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "c++ ${flags}"] != "" } {
+	unresolved "failed to compile"
+	return 0
+    }
+
+    # Start with a fresh gdb.
+    clean_restart ${binfile}
+
+    # Make certain that the output is consistent
+    gdb_test_no_output "set print sevenbit-strings"
+    gdb_test_no_output "set print address off"
+    gdb_test_no_output "set print pretty off"
+    gdb_test_no_output "set width 0"
+    gdb_test_no_output "set print elements 300"
+
+    # Advance to main
+    if { ![runto_main] } then {
+	fail "can't run to main"
+	return 0
+    }
+
+    # Now continue forward to a suitable location to run the tests.
+    # Some targets only enable the FPU on first use, so ensure that we
+    # have used the FPU before we make calls from GDB to code that
+    # could use the FPU.
+    gdb_breakpoint [gdb_get_line_number "Break Here"] temporary
+    gdb_continue_to_breakpoint "breakpt" ".* Break Here\\. .*"
+
+    return 1
+}
+
+# Assuming GDB is stopped at main within a test binary, run some tests
+# passing structures, and reading return value structures.
+
+proc run_tests {} {
+    global gdb_prompt
+
+    foreach {name} {struct_02_01 struct_02_02 struct_02_03 struct_02_04 } {
+	gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"
+
+	set refval [ get_valueof "" "ref_val_${name}" "" ]
+	verbose -log "Refval: ${refval}"
+
+	set test "check return value ${name}"
+	if { ${refval} != "" } {
+	    set answer [ get_valueof "" "rtn_str_${name} ()" "XXXX"]
+	    verbose -log "Answer: ${answer}"
+	    gdb_assert [string eq ${answer} ${refval}] ${test}
+	} else {
+	    unresolved $test
+	}
+    }
+}
+
+# Set up a test prefix, compile the test binary, run to main, and then
+# run some tests.
+
+proc start_gdb_and_run_tests { types } {
+    set prefix "types"
+
+    foreach t $types {
+	append prefix "-" "${t}"
+    }
+
+    with_test_prefix $prefix {
+	if { [start_nested_structs_test $types] } {
+	    run_tests
+	}
+    }
+}
+
+foreach ta $int_types {
+    start_gdb_and_run_tests $ta
+}
+
+# if [support_complex_tests] {
+#     foreach ta $complex_types {
+# 	start_gdb_and_run_tests $ta
+#     }
+# }
+
+if ![gdb_skip_float_test] {
+    foreach ta $float_types {
+	start_gdb_and_run_tests $ta
+    }
+
+    foreach ta $int_types {
+	foreach tb $float_types {
+	    start_gdb_and_run_tests [list $ta $tb]
+	}
+    }
+
+    foreach ta $float_types {
+	foreach tb $int_types {
+	    start_gdb_and_run_tests [list $ta $tb]
+	}
+
+	foreach tb $float_types {
+	    start_gdb_and_run_tests [list $ta $tb]
+	}
+    }
+}
-- 
2.17.2 (Apple Git-113)

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++
  2019-01-16 15:57 ` [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++ Alan Hayward
@ 2019-01-17 17:08   ` Pedro Alves
  2019-01-18 10:34     ` Alan Hayward
  0 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2019-01-17 17:08 UTC (permalink / raw)
  To: Alan Hayward, gdb-patches; +Cc: nd

On 01/16/2019 03:57 PM, Alan Hayward wrote:
> When gdb.base/infcall-nested-structs.c is complied as C++, the structs
> containing empty structs are no longer passed via float arguments.

This reads a bit ambiguously.  Which is it?

#1 - No longer passed by GCC, but GDB still passes.
#2 - No longer passed by GDB, but GCC still passes.

> This is because structs in C++ have a minimum size of 1.  This can then
> cause padding in the struct, which is disallowed for AAPCS.

Does this "disallowed" mean that structs with padding are
not allowed to be passed via float arguments?  Took me a while to
grok that.

It'd be good to clarify the commit log.

> +foreach l $lang {
> +    set dir "$l"
> +    remote_exec build "rm -rf [standard_output_file ${dir}]"
> +    remote_exec build "mkdir -p [standard_output_file ${dir}]"

I think these should be

   remote_exec host

not "build" ?

For remote-host testing, where the compiler and debugger run on the
host machine.

Could you please file a bug for the x86 internal errors, and
kfail the test for x86?

Otherwise looks fine to me.

Thanks,
Pedro Alves

> +}
> +
> +
>  set int_types { tc ts ti tl tll }
>  set float_types { tf td tld }
>  set complex_types { tfc tdc tldc }
> @@ -44,7 +58,7 @@ proc I2A { n } {
>  # types of the struct fields within the source.  Run up to main.
>  # Also updates the global "testfile" to reflect the most recent build.
>  
> -proc start_nested_structs_test { types } {
> +proc start_nested_structs_test { lang types } {
>      global testfile
>      global srcfile
>      global binfile
> @@ -53,9 +67,11 @@ proc start_nested_structs_test { types } {
>      global compile_flags
>  
>      standard_testfile .c
> +    set dir "$lang"
>  
>      # Create the additional flags
>      set flags $compile_flags
> +    lappend flags $lang
>  
>      for {set n 0} {$n<[llength ${types}]} {incr n} {
>  	set m [I2A ${n}]
> @@ -64,7 +80,7 @@ proc start_nested_structs_test { types } {
>  	append testfile "-" "$t"
>      }
>  
> -    set binfile [standard_output_file ${testfile}]
> +    set binfile [standard_output_file ${dir}/${testfile}]
>      if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "${flags}"] != "" } {
>  	unresolved "failed to compile"
>  	return 0
> @@ -125,48 +141,50 @@ proc run_tests {} {
>  # Set up a test prefix, compile the test binary, run to main, and then
>  # run some tests.
>  
> -proc start_gdb_and_run_tests { types } {
> +proc start_gdb_and_run_tests { lang types } {

>      set prefix "types"
>  
>      foreach t $types {
>  	append prefix "-" "${t}"
>      }
>  
> -    with_test_prefix $prefix {
> -	if { [start_nested_structs_test $types] } {
> -	    run_tests
> +    foreach_with_prefix l $lang {
> +	with_test_prefix $prefix {
> +	    if { [start_nested_structs_test $l $types] } {
> +		run_tests
> +	    }
>  	}
>      }
>  }
>  
>  foreach ta $int_types {
> -    start_gdb_and_run_tests $ta
> +    start_gdb_and_run_tests $lang $ta
>  }
>  
>  if [support_complex_tests] {
>      foreach ta $complex_types {
> -	start_gdb_and_run_tests $ta
> +	start_gdb_and_run_tests $lang $ta
>      }
>  }
>  
>  if ![gdb_skip_float_test] {
>      foreach ta $float_types {
> -	start_gdb_and_run_tests $ta
> +	start_gdb_and_run_tests $lang $ta
>      }
>  
>      foreach ta $int_types {
>  	foreach tb $float_types {
> -	    start_gdb_and_run_tests [list $ta $tb]
> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>  	}
>      }
>  
>      foreach ta $float_types {
>  	foreach tb $int_types {
> -	    start_gdb_and_run_tests [list $ta $tb]
> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>  	}
>  
>  	foreach tb $float_types {
> -	    start_gdb_and_run_tests [list $ta $tb]
> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>  	}
>      }
>  }
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] AArch64 AAPCS: Ignore static members
  2019-01-16 15:57 ` [PATCH 2/2] AArch64 AAPCS: Ignore static members Alan Hayward
@ 2019-01-17 17:22   ` Pedro Alves
  2019-01-18 10:49     ` Alan Hayward
  0 siblings, 1 reply; 12+ messages in thread
From: Pedro Alves @ 2019-01-17 17:22 UTC (permalink / raw)
  To: Alan Hayward, gdb-patches; +Cc: nd

On 01/16/2019 03:57 PM, Alan Hayward wrote:
> Static members in C++ structs are global data and therefore not part of the
> list of struct members considered for passing in registers.
> 
> Note the corresponding code in GCC (from which the GDB AAPCS code is based)
> does not have any static member checks due to the static members not being
> part of the struct type at that point.
> 
> Add a new test based on gdb.base/infcall-nested-structs.exp, adding static
> members.

Diffing the files, it seems a lot is shared.  Would it be
possible and make sense to put the static members tests in
infcall-nested-structs.c, wrapped with #ifdef __cplusplus?

> diff --git a/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc b/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc
> new file mode 100644
> index 0000000000..bec63728f4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc
> @@ -0,0 +1,222 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2018 Free Software Foundation, Inc.

This would be 2018-2019.

> +
> +/* Structures with two fields nested to various depths, one of which is static,
> +   along with some empty structures.  */

Is the "along with some empty structures." part important for this test?
Kind of seems like the other bug bleeds into this test?

> +
> +int cmp_struct_02_01 (struct struct_02_01 a,
> +			     struct struct_02_01 b)
> +{ return a.s2.s1.a == b.s2.s1.a && a.s2.s1.b == a.s2.s1.b; }
> +
> +int cmp_struct_02_02 (struct struct_02_02 a,
> +			     struct struct_02_02 b)
> +{ return a.a == b.a && a.b == b.b; }
> +
> +int cmp_struct_02_03 (struct struct_02_03 a,
> +			     struct struct_02_03 b)
> +{ return a.s4.s3.a == b.s4.s3.a && a.s6.s5.b == b.s6.s5.b; }
> +
> +int cmp_struct_02_04 (struct struct_02_04 a,
> +			     struct struct_02_04 b)
> +{ return a.a == b.a && a.b == b.b; }
> +

While diffing I noticed formatting changes spurious here.

> +foreach ta $int_types {
> +    start_gdb_and_run_tests $ta
> +}
> +
> +# if [support_complex_tests] {
> +#     foreach ta $complex_types {
> +# 	start_gdb_and_run_tests $ta
> +#     }
> +# }
> +

Looks like you meant to remove this.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++
  2019-01-17 17:08   ` Pedro Alves
@ 2019-01-18 10:34     ` Alan Hayward
  2019-01-21 15:52       ` Alan Hayward
  0 siblings, 1 reply; 12+ messages in thread
From: Alan Hayward @ 2019-01-18 10:34 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches, nd

(Thanks, would have pushed, but outstanding question below)…


> On 17 Jan 2019, at 17:08, Pedro Alves <palves@redhat.com> wrote:
> 
> On 01/16/2019 03:57 PM, Alan Hayward wrote:
>> When gdb.base/infcall-nested-structs.c is complied as C++, the structs
>> containing empty structs are no longer passed via float arguments.
> 
> This reads a bit ambiguously.  Which is it?
> 
> #1 - No longer passed by GCC, but GDB still passes.
> #2 - No longer passed by GDB, but GCC still passes.
> 
>> This is because structs in C++ have a minimum size of 1.  This can then
>> cause padding in the struct, which is disallowed for AAPCS.
> 
> Does this "disallowed" mean that structs with padding are
> not allowed to be passed via float arguments?  Took me a while to
> grok that.
> 
> It'd be good to clarify the commit log.

I’ll change to:
When gdb.base/infcall-nested-structs.c is complied as C++, the compiler
will not pass structs containing empty structs via float arguments.
This is because structs in C++ have a minimum size of 1, causing padding
in the struct.  The AAPCS does not allow structs with padding to be
passed in float arguments.

>> +foreach l $lang {
>> +    set dir "$l"
>> +    remote_exec build "rm -rf [standard_output_file ${dir}]"
>> +    remote_exec build "mkdir -p [standard_output_file ${dir}]"
> 
> I think these should be
> 
>   remote_exec host
> 
> not "build" ?
> 
> For remote-host testing, where the compiler and debugger run on the
> host machine.
> 

This was due to copying from another test - I’ll raise a quick
patch to fix those up too.


> Could you please file a bug for the x86 internal errors, and
> kfail the test for x86?
> 

Will raise a bug.

The pattern for which tests pass and fail is not that simple.
Each structure gets tested 49 times on c++ (with different types).
Eg: For struct_01_01, 17 of them fail, but for struct_01_04 only
13 fail.

Is it ok to be over cautious (and have some XPASS results)
or do we really need a large messy if statement with all the
exact matches?


> Otherwise looks fine to me.
> 
> Thanks,
> Pedro Alves
> 
>> +}
>> +
>> +
>> set int_types { tc ts ti tl tll }
>> set float_types { tf td tld }
>> set complex_types { tfc tdc tldc }
>> @@ -44,7 +58,7 @@ proc I2A { n } {
>> # types of the struct fields within the source.  Run up to main.
>> # Also updates the global "testfile" to reflect the most recent build.
>> 
>> -proc start_nested_structs_test { types } {
>> +proc start_nested_structs_test { lang types } {
>>     global testfile
>>     global srcfile
>>     global binfile
>> @@ -53,9 +67,11 @@ proc start_nested_structs_test { types } {
>>     global compile_flags
>> 
>>     standard_testfile .c
>> +    set dir "$lang"
>> 
>>     # Create the additional flags
>>     set flags $compile_flags
>> +    lappend flags $lang
>> 
>>     for {set n 0} {$n<[llength ${types}]} {incr n} {
>> 	set m [I2A ${n}]
>> @@ -64,7 +80,7 @@ proc start_nested_structs_test { types } {
>> 	append testfile "-" "$t"
>>     }
>> 
>> -    set binfile [standard_output_file ${testfile}]
>> +    set binfile [standard_output_file ${dir}/${testfile}]
>>     if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "${flags}"] != "" } {
>> 	unresolved "failed to compile"
>> 	return 0
>> @@ -125,48 +141,50 @@ proc run_tests {} {
>> # Set up a test prefix, compile the test binary, run to main, and then
>> # run some tests.
>> 
>> -proc start_gdb_and_run_tests { types } {
>> +proc start_gdb_and_run_tests { lang types } {
> 
>>     set prefix "types"
>> 
>>     foreach t $types {
>> 	append prefix "-" "${t}"
>>     }
>> 
>> -    with_test_prefix $prefix {
>> -	if { [start_nested_structs_test $types] } {
>> -	    run_tests
>> +    foreach_with_prefix l $lang {
>> +	with_test_prefix $prefix {
>> +	    if { [start_nested_structs_test $l $types] } {
>> +		run_tests
>> +	    }
>> 	}
>>     }
>> }
>> 
>> foreach ta $int_types {
>> -    start_gdb_and_run_tests $ta
>> +    start_gdb_and_run_tests $lang $ta
>> }
>> 
>> if [support_complex_tests] {
>>     foreach ta $complex_types {
>> -	start_gdb_and_run_tests $ta
>> +	start_gdb_and_run_tests $lang $ta
>>     }
>> }
>> 
>> if ![gdb_skip_float_test] {
>>     foreach ta $float_types {
>> -	start_gdb_and_run_tests $ta
>> +	start_gdb_and_run_tests $lang $ta
>>     }
>> 
>>     foreach ta $int_types {
>> 	foreach tb $float_types {
>> -	    start_gdb_and_run_tests [list $ta $tb]
>> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>> 	}
>>     }
>> 
>>     foreach ta $float_types {
>> 	foreach tb $int_types {
>> -	    start_gdb_and_run_tests [list $ta $tb]
>> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>> 	}
>> 
>> 	foreach tb $float_types {
>> -	    start_gdb_and_run_tests [list $ta $tb]
>> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>> 	}
>>     }
>> }
>> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/2] AArch64 AAPCS: Ignore static members
  2019-01-17 17:22   ` Pedro Alves
@ 2019-01-18 10:49     ` Alan Hayward
  0 siblings, 0 replies; 12+ messages in thread
From: Alan Hayward @ 2019-01-18 10:49 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches, nd



> On 17 Jan 2019, at 17:22, Pedro Alves <palves@redhat.com> wrote:
> 
> On 01/16/2019 03:57 PM, Alan Hayward wrote:
>> Static members in C++ structs are global data and therefore not part of the
>> list of struct members considered for passing in registers.
>> 
>> Note the corresponding code in GCC (from which the GDB AAPCS code is based)
>> does not have any static member checks due to the static members not being
>> part of the struct type at that point.
>> 
>> Add a new test based on gdb.base/infcall-nested-structs.exp, adding static
>> members.
> 
> Diffing the files, it seems a lot is shared.  Would it be
> possible and make sense to put the static members tests in
> infcall-nested-structs.c, wrapped with #ifdef __cplusplus?
> 

My concern was not wanting to make the original test too big - it already has
7000 cases and takes 30secs to run for me.  I started with a single test, and
it looked messy.
Happy to merge them back together.

>> diff --git a/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc b/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc
>> new file mode 100644
>> index 0000000000..bec63728f4
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.cp/infcall-nested-static-structs.cc
>> @@ -0,0 +1,222 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2018 Free Software Foundation, Inc.
> 
> This would be 2018-2019.
> 
>> +
>> +/* Structures with two fields nested to various depths, one of which is static,
>> +   along with some empty structures.  */
> 
> Is the "along with some empty structures." part important for this test?
> Kind of seems like the other bug bleeds into this test?

That part isn’t really important in this test (as they will just hit
the code for the previous patch).
But, I thought it was worth having some cases that tested both together.
Maybe just needs that part of the comment removed.


> 
>> +
>> +int cmp_struct_02_01 (struct struct_02_01 a,
>> +			     struct struct_02_01 b)
>> +{ return a.s2.s1.a == b.s2.s1.a && a.s2.s1.b == a.s2.s1.b; }
>> +
>> +int cmp_struct_02_02 (struct struct_02_02 a,
>> +			     struct struct_02_02 b)
>> +{ return a.a == b.a && a.b == b.b; }
>> +
>> +int cmp_struct_02_03 (struct struct_02_03 a,
>> +			     struct struct_02_03 b)
>> +{ return a.s4.s3.a == b.s4.s3.a && a.s6.s5.b == b.s6.s5.b; }
>> +
>> +int cmp_struct_02_04 (struct struct_02_04 a,
>> +			     struct struct_02_04 b)
>> +{ return a.a == b.a && a.b == b.b; }
>> +
> 
> While diffing I noticed formatting changes spurious here.
> 
>> +foreach ta $int_types {
>> +    start_gdb_and_run_tests $ta
>> +}
>> +
>> +# if [support_complex_tests] {
>> +#     foreach ta $complex_types {
>> +# 	start_gdb_and_run_tests $ta
>> +#     }
>> +# }
>> +
> 
> Looks like you meant to remove this.
> 
> Thanks,
> Pedro Alves


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++
  2019-01-18 10:34     ` Alan Hayward
@ 2019-01-21 15:52       ` Alan Hayward
  2019-10-10 23:49         ` Tom de Vries
  0 siblings, 1 reply; 12+ messages in thread
From: Alan Hayward @ 2019-01-21 15:52 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches, nd

Pushed with changes suggested, including all xpass matches.

Alan.


diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index b051563937..7c5d74858d 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -1232,6 +1232,14 @@ aapcs_is_vfp_call_or_return_candidate_1 (struct type *type,
              return -1;
            count += sub_count;
          }
+
+       /* Ensure there is no padding between the fields (allowing for empty
+          zero length structs)  */
+       int ftype_length = (*fundamental_type == nullptr)
+                          ? 0 : TYPE_LENGTH (*fundamental_type);
+       if (count * ftype_length != TYPE_LENGTH (type))
+         return -1;
+
        return count;
       }

diff --git a/gdb/testsuite/gdb.base/infcall-nested-structs.exp b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
index b04d9aaa84..d7d1e3e00d 100644
--- a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
+++ b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
@@ -24,6 +24,20 @@ if [target_info exists gdb,cannot_call_functions] {
     continue
 }

+# Only test C++ if we are able.  Always use C.
+if { [skip_cplus_tests] || [get_compiler_info "c++"] } {
+    set lang {c}
+} else {
+    set lang {c c++}
+}
+
+foreach l $lang {
+    set dir "$l"
+    remote_exec host "rm -rf [standard_output_file ${dir}]"
+    remote_exec host "mkdir -p [standard_output_file ${dir}]"
+}
+
+
 set int_types { tc ts ti tl tll }
 set float_types { tf td tld }
 set complex_types { tfc tdc tldc }
@@ -31,6 +45,7 @@ set complex_types { tfc tdc tldc }
 set compile_flags {debug}
 if [support_complex_tests] {
     lappend compile_flags "additional_flags=-DTEST_COMPLEX"
+    lappend compile_flags "additional_flags=-Wno-psabi"
 }

 # Given N (0..25), return the corresponding alphabetic letter in upper
@@ -44,7 +59,7 @@ proc I2A { n } {
 # types of the struct fields within the source.  Run up to main.
 # Also updates the global "testfile" to reflect the most recent build.

-proc start_nested_structs_test { types } {
+proc start_nested_structs_test { lang types } {
     global testfile
     global srcfile
     global binfile
@@ -53,9 +68,11 @@ proc start_nested_structs_test { types } {
     global compile_flags

     standard_testfile .c
+    set dir "$lang"

     # Create the additional flags
     set flags $compile_flags
+    lappend flags $lang

     for {set n 0} {$n<[llength ${types}]} {incr n} {
        set m [I2A ${n}]
@@ -64,7 +81,7 @@ proc start_nested_structs_test { types } {
        append testfile "-" "$t"
     }

-    set binfile [standard_output_file ${testfile}]
+    set binfile [standard_output_file ${dir}/${testfile}]
     if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "${flags}"] != "" } {
        unresolved "failed to compile"
        return 0
@@ -99,13 +116,21 @@ proc start_nested_structs_test { types } {
 # Assuming GDB is stopped at main within a test binary, run some tests
 # passing structures, and reading return value structures.

-proc run_tests {} {
+proc run_tests { lang types } {
     global gdb_prompt

     foreach {name} {struct_01_01 struct_01_02 struct_01_03 struct_01_04
                     struct_02_01 struct_02_02 struct_02_03 struct_02_04
                     struct_04_01 struct_04_02 struct_04_03 struct_04_04
                     struct_05_01 struct_05_02 struct_05_03 struct_05_04} {
+
+       if { ( $lang == "c++"
+              && ( ( [regexp "struct_01_0(1|2|3)" $name match] && [regexp "^types-(td($|-)|tl(|l)(|-tf|-td|-tld)$)" $types match] )
+                   || ( $name == "struct_01_02" && $types == "types-tfc" )
+                   || ( $name == "struct_01_04" && [regexp "^types-(tf($|-)|ti(|-tf|-td|-tld)$)" $types match] )
+                   || ( $name == "struct_02_01" && [regexp "^types-tf-t(c|s|i)" $types match] ) ) ) } {
+           setup_xfail gdb/24104 "x86_64-*-linux*"
+       }
        gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"

        set refval [ get_valueof "" "ref_val_${name}" "" ]
@@ -113,8 +138,13 @@ proc run_tests {} {

        set test "check return value ${name}"
        if { ${refval} != "" } {
+
            set answer [ get_valueof "" "rtn_str_${name} ()" "XXXX"]
            verbose -log "Answer: ${answer}"
+
+           if { ($lang == "c++" && $name == "struct_02_01" && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] ) } {
+               setup_xfail gdb/24104 "x86_64-*-linux*"
+           }
            gdb_assert [string eq ${answer} ${refval}] ${test}
        } else {
            unresolved $test
@@ -125,48 +155,50 @@ proc run_tests {} {
 # Set up a test prefix, compile the test binary, run to main, and then
 # run some tests.

-proc start_gdb_and_run_tests { types } {
+proc start_gdb_and_run_tests { lang types } {
     set prefix "types"

     foreach t $types {
        append prefix "-" "${t}"
     }

-    with_test_prefix $prefix {
-       if { [start_nested_structs_test $types] } {
-           run_tests
+    foreach_with_prefix l $lang {
+       with_test_prefix $prefix {
+           if { [start_nested_structs_test $l $types] } {
+               run_tests $l $prefix
+           }
        }
     }
 }

 foreach ta $int_types {
-    start_gdb_and_run_tests $ta
+    start_gdb_and_run_tests $lang $ta
 }

 if [support_complex_tests] {
     foreach ta $complex_types {
-       start_gdb_and_run_tests $ta
+       start_gdb_and_run_tests $lang $ta
     }
 }

 if ![gdb_skip_float_test] {
     foreach ta $float_types {
-       start_gdb_and_run_tests $ta
+       start_gdb_and_run_tests $lang $ta
     }

     foreach ta $int_types {
        foreach tb $float_types {
-           start_gdb_and_run_tests [list $ta $tb]
+           start_gdb_and_run_tests $lang [list $ta $tb]
        }
     }

     foreach ta $float_types {
        foreach tb $int_types {
-           start_gdb_and_run_tests [list $ta $tb]
+           start_gdb_and_run_tests $lang [list $ta $tb]
        }

        foreach tb $float_types {
-           start_gdb_and_run_tests [list $ta $tb]
+           start_gdb_and_run_tests $lang [list $ta $tb]
        }
     }
 }

> On 18 Jan 2019, at 10:34, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> (Thanks, would have pushed, but outstanding question below)…
> 
> 
>> On 17 Jan 2019, at 17:08, Pedro Alves <palves@redhat.com> wrote:
>> 
>> On 01/16/2019 03:57 PM, Alan Hayward wrote:
>>> When gdb.base/infcall-nested-structs.c is complied as C++, the structs
>>> containing empty structs are no longer passed via float arguments.
>> 
>> This reads a bit ambiguously.  Which is it?
>> 
>> #1 - No longer passed by GCC, but GDB still passes.
>> #2 - No longer passed by GDB, but GCC still passes.
>> 
>>> This is because structs in C++ have a minimum size of 1.  This can then
>>> cause padding in the struct, which is disallowed for AAPCS.
>> 
>> Does this "disallowed" mean that structs with padding are
>> not allowed to be passed via float arguments?  Took me a while to
>> grok that.
>> 
>> It'd be good to clarify the commit log.
> 
> I’ll change to:
> When gdb.base/infcall-nested-structs.c is complied as C++, the compiler
> will not pass structs containing empty structs via float arguments.
> This is because structs in C++ have a minimum size of 1, causing padding
> in the struct.  The AAPCS does not allow structs with padding to be
> passed in float arguments.
> 
>>> +foreach l $lang {
>>> +    set dir "$l"
>>> +    remote_exec build "rm -rf [standard_output_file ${dir}]"
>>> +    remote_exec build "mkdir -p [standard_output_file ${dir}]"
>> 
>> I think these should be
>> 
>>  remote_exec host
>> 
>> not "build" ?
>> 
>> For remote-host testing, where the compiler and debugger run on the
>> host machine.
>> 
> 
> This was due to copying from another test - I’ll raise a quick
> patch to fix those up too.
> 
> 
>> Could you please file a bug for the x86 internal errors, and
>> kfail the test for x86?
>> 
> 
> Will raise a bug.
> 
> The pattern for which tests pass and fail is not that simple.
> Each structure gets tested 49 times on c++ (with different types).
> Eg: For struct_01_01, 17 of them fail, but for struct_01_04 only
> 13 fail.
> 
> Is it ok to be over cautious (and have some XPASS results)
> or do we really need a large messy if statement with all the
> exact matches?
> 
> 
>> Otherwise looks fine to me.
>> 
>> Thanks,
>> Pedro Alves
>> 
>>> +}
>>> +
>>> +
>>> set int_types { tc ts ti tl tll }
>>> set float_types { tf td tld }
>>> set complex_types { tfc tdc tldc }
>>> @@ -44,7 +58,7 @@ proc I2A { n } {
>>> # types of the struct fields within the source.  Run up to main.
>>> # Also updates the global "testfile" to reflect the most recent build.
>>> 
>>> -proc start_nested_structs_test { types } {
>>> +proc start_nested_structs_test { lang types } {
>>>    global testfile
>>>    global srcfile
>>>    global binfile
>>> @@ -53,9 +67,11 @@ proc start_nested_structs_test { types } {
>>>    global compile_flags
>>> 
>>>    standard_testfile .c
>>> +    set dir "$lang"
>>> 
>>>    # Create the additional flags
>>>    set flags $compile_flags
>>> +    lappend flags $lang
>>> 
>>>    for {set n 0} {$n<[llength ${types}]} {incr n} {
>>> 	set m [I2A ${n}]
>>> @@ -64,7 +80,7 @@ proc start_nested_structs_test { types } {
>>> 	append testfile "-" "$t"
>>>    }
>>> 
>>> -    set binfile [standard_output_file ${testfile}]
>>> +    set binfile [standard_output_file ${dir}/${testfile}]
>>>    if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable "${flags}"] != "" } {
>>> 	unresolved "failed to compile"
>>> 	return 0
>>> @@ -125,48 +141,50 @@ proc run_tests {} {
>>> # Set up a test prefix, compile the test binary, run to main, and then
>>> # run some tests.
>>> 
>>> -proc start_gdb_and_run_tests { types } {
>>> +proc start_gdb_and_run_tests { lang types } {
>> 
>>>    set prefix "types"
>>> 
>>>    foreach t $types {
>>> 	append prefix "-" "${t}"
>>>    }
>>> 
>>> -    with_test_prefix $prefix {
>>> -	if { [start_nested_structs_test $types] } {
>>> -	    run_tests
>>> +    foreach_with_prefix l $lang {
>>> +	with_test_prefix $prefix {
>>> +	    if { [start_nested_structs_test $l $types] } {
>>> +		run_tests
>>> +	    }
>>> 	}
>>>    }
>>> }
>>> 
>>> foreach ta $int_types {
>>> -    start_gdb_and_run_tests $ta
>>> +    start_gdb_and_run_tests $lang $ta
>>> }
>>> 
>>> if [support_complex_tests] {
>>>    foreach ta $complex_types {
>>> -	start_gdb_and_run_tests $ta
>>> +	start_gdb_and_run_tests $lang $ta
>>>    }
>>> }
>>> 
>>> if ![gdb_skip_float_test] {
>>>    foreach ta $float_types {
>>> -	start_gdb_and_run_tests $ta
>>> +	start_gdb_and_run_tests $lang $ta
>>>    }
>>> 
>>>    foreach ta $int_types {
>>> 	foreach tb $float_types {
>>> -	    start_gdb_and_run_tests [list $ta $tb]
>>> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>>> 	}
>>>    }
>>> 
>>>    foreach ta $float_types {
>>> 	foreach tb $int_types {
>>> -	    start_gdb_and_run_tests [list $ta $tb]
>>> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>>> 	}
>>> 
>>> 	foreach tb $float_types {
>>> -	    start_gdb_and_run_tests [list $ta $tb]
>>> +	    start_gdb_and_run_tests $lang [list $ta $tb]
>>> 	}
>>>    }
>>> }
>>> 
>> 
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++
  2019-01-21 15:52       ` Alan Hayward
@ 2019-10-10 23:49         ` Tom de Vries
  2019-10-11 12:14           ` [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_arguments Tom de Vries
  0 siblings, 1 reply; 12+ messages in thread
From: Tom de Vries @ 2019-10-10 23:49 UTC (permalink / raw)
  To: Alan Hayward, Pedro Alves; +Cc: gdb-patches, nd

On 21-01-2019 16:52, Alan Hayward wrote:

> +       if { ( $lang == "c++"
> +              && ( ( [regexp "struct_01_0(1|2|3)" $name match] && [regexp "^types-(td($|-)|tl(|l)(|-tf|-td|-tld)$)" $types match] )
> +                   || ( $name == "struct_01_02" && $types == "types-tfc" )
> +                   || ( $name == "struct_01_04" && [regexp "^types-(tf($|-)|ti(|-tf|-td|-tld)$)" $types match] )
> +                   || ( $name == "struct_02_01" && [regexp "^types-tf-t(c|s|i)" $types match] ) ) ) } {
> +           setup_xfail gdb/24104 "x86_64-*-linux*"
> +       }
>         gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"


> +           if { ($lang == "c++" && $name == "struct_02_01" && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] ) } {
> +               setup_xfail gdb/24104 "x86_64-*-linux*"
> +           }
>             gdb_assert [string eq ${answer} ${refval}] ${test}

These should have been kfails, not xfails.

Thanks,
- Tom

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_arguments
  2019-10-10 23:49         ` Tom de Vries
@ 2019-10-11 12:14           ` Tom de Vries
  2019-10-14 13:10             ` Alan Hayward
  0 siblings, 1 reply; 12+ messages in thread
From: Tom de Vries @ 2019-10-11 12:14 UTC (permalink / raw)
  To: Alan Hayward, Pedro Alves; +Cc: gdb-patches

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

[ was: Re: [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size
in C++ ]

On 11-10-2019 01:49, Tom de Vries wrote:
> On 21-01-2019 16:52, Alan Hayward wrote:
> 
>> +       if { ( $lang == "c++"
>> +              && ( ( [regexp "struct_01_0(1|2|3)" $name match] && [regexp "^types-(td($|-)|tl(|l)(|-tf|-td|-tld)$)" $types match] )
>> +                   || ( $name == "struct_01_02" && $types == "types-tfc" )
>> +                   || ( $name == "struct_01_04" && [regexp "^types-(tf($|-)|ti(|-tf|-td|-tld)$)" $types match] )
>> +                   || ( $name == "struct_02_01" && [regexp "^types-tf-t(c|s|i)" $types match] ) ) ) } {
>> +           setup_xfail gdb/24104 "x86_64-*-linux*"
>> +       }
>>         gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"
> 
> 
>> +           if { ($lang == "c++" && $name == "struct_02_01" && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] ) } {
>> +               setup_xfail gdb/24104 "x86_64-*-linux*"
>> +           }
>>             gdb_assert [string eq ${answer} ${refval}] ${test}
> 
> These should have been kfails, not xfails.

This patch fixes PR24104.

OK for trunk?

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-tdep-Fix-Unexpected-register-class-assert-in-amd64_push_arguments.patch --]
[-- Type: text/x-patch, Size: 6630 bytes --]

[gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_arguments

Atm, when executing gdb.base/infcall-nested-structs.exp on x86_64-linux, we get:
...
FAIL: gdb.base/infcall-nested-structs.exp: l=c++: types-tc-tf: \
  p/d check_arg_struct_02_01 (ref_val_struct_02_01)
FAIL: gdb.base/infcall-nested-structs.exp: l=c++: types-ts-tf: \
  p/d check_arg_struct_02_01 (ref_val_struct_02_01)
FAIL: gdb.base/infcall-nested-structs.exp: l=c++: types-ti-tf: \
  p/d check_arg_struct_02_01 (ref_val_struct_02_01)

                === gdb Summary ===

nr of expected passes            9255
nr of unexpected failures        3
nr of expected failures          142
...

The 3 FAILs are reported as PR tdep/25096.

The 142 XFAILs are for a gdb assertion failure, reported in PR tdep/24104,
which should have been KFAILs since there's a problem in gdb rather than in
the environment.

A minimal version of the assertion failure looks like this. Consider test.c:
...
struct s { struct { } es1; long f; };
struct s ref = { {}, 'f' };

int __attribute__((noinline,noclone)) check (struct s arg)
{ return arg.f == 'f'; }

int main (void)
{ return check (ref); }
...

When calling 'check (ref)' from main, we have '1' as expected:
...
$ g++ test3.c -g && ( ./a.out; echo $? )
1
...

But when calling 'check (ref)' from the gdb prompt, we get:
...
$ gdb a.out -batch -ex start -ex "p check (ref)"
Temporary breakpoint 1 at 0x4004f7: file test.c, line 8.

Temporary breakpoint 1, main () at test.c:8
8       { return check (ref); }
src/gdb/amd64-tdep.c:982: internal-error: \
  CORE_ADDR amd64_push_arguments(regcache*, int, value**, CORE_ADDR, \
                                 function_call_return_method): \
  Assertion `!"Unexpected register class."' failed.
...

The assert happens in this loop in amd64_push_arguments:
...
          for (j = 0; len > 0; j++, len -= 8)
            {
              int regnum = -1;
              int offset = 0;

              switch (theclass[j])
                {
                case AMD64_INTEGER:
                  regnum = integer_regnum[integer_reg++];
                  break;

                case AMD64_SSE:
                  regnum = sse_regnum[sse_reg++];
                  break;

                case AMD64_SSEUP:
                  gdb_assert (sse_reg > 0);
                  regnum = sse_regnum[sse_reg - 1];
                  offset = 8;
                  break;

                default:
                  gdb_assert (!"Unexpected register class.");
                }
		...
            }
...
when processing theclass[0], which is AMD64_NO_CLASS:
...
(gdb) p theclass
$1 = {AMD64_NO_CLASS, AMD64_INTEGER}
...

The layout of struct s is that the empty field es1 occupies one byte (due to
c++) at offset 0, and the long field f occupies 8 bytes at offset 8.

When compiling at -O2, we can see from the disassembly of main:
...
  4003f0:       48 8b 3d 41 0c 20 00    mov    0x200c41(%rip),%rdi \
                                               # 601038 <ref+0x8>
  4003f7:       e9 e4 00 00 00          jmpq   4004e0 <_Z5check1s>
  4003fc:       0f 1f 40 00             nopl   0x0(%rax)
...
that check is called with field f passed in %rdi, meaning that the
classification in theclass is correct, it's just not supported in the loop in
amd64_push_arguments mentioned above.

Fix the assert by implementing support for 'AMD64_NO_CLASS' in that loop.

This exposes 9 more FAILs of the PR tdep/25096 type, so mark all 12 of them as
KFAIL.

Tested on x86_64-linux.

Tested with g++ 4.8.5, 7.4.1, 8.3.1, 9.2.1.  With 4.8.5, 3 of the 12 KFAILs
are KPASSing.

Tested with clang++ 5.0.2 (which requires removing additional_flags=-Wno-psabi
and adding additional_flags=-Wno-deprecated).

gdb/ChangeLog:

2019-10-11  Tom de Vries  <tdevries@suse.de>

	PR tdep/24104
	* amd64-tdep.c (amd64_push_arguments): Handle AMD64_NO_CLASS in loop
	that handles 'theclass'.

gdb/testsuite/ChangeLog:

2019-10-11  Tom de Vries  <tdevries@suse.de>

	PR tdep/24104
	* gdb.base/infcall-nested-structs.exp: Remove XFAIL for PR tdep/24104.
	Add KFAIL for PR tdep/25096.

---
 gdb/amd64-tdep.c                                  |  3 +++
 gdb/testsuite/gdb.base/infcall-nested-structs.exp | 18 ++++++------------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 232d16d7345..9006ec0167a 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -978,6 +978,9 @@ if (return_method == return_method_struct)
 		  offset = 8;
 		  break;
 
+		case AMD64_NO_CLASS:
+		  continue;
+
 		default:
 		  gdb_assert (!"Unexpected register class.");
 		}
diff --git a/gdb/testsuite/gdb.base/infcall-nested-structs.exp b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
index f5fbf44ed16..957eb31bdc2 100644
--- a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
+++ b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
@@ -132,16 +132,9 @@ proc run_tests { lang types } {
 	    continue
 	}
 
-	if { $lang == "c++"
-	     && ( ( [regexp "struct_01_0(1|2|3)" $name match] && [regexp "^types-(td($|-)|tl(|l)(|-tf|-td|-tld)$)" $types match] )
-		  || ( $name == "struct_01_02" && $types == "types-tfc" )
-		  || ( $name == "struct_01_04" && [regexp "^types-(tf($|-)|ti(|-tf|-td|-tld)$)" $types match] )
-		  || ( $name == "struct_02_01" && [regexp "^types-tf-t(c|s|i)" $types match] )
-		  || ( $name == "struct_static_02_02" && [regexp "^types-(t(f|d|ld)-t(d|l|ll)$|t(d|l|ll)$|t(c|s|i|l|ll)-td)" $types match] )
-		  || ( $name == "struct_static_02_03" && [regexp "^types-(ti-t(f|l|d|)|tf(-|$)|ti$)" $types match] )
-		  || ( $name == "struct_static_04_02" && [regexp "^types-(t(c|s)-tf|tf-ts)" $types match] )
-		  || ( $name == "struct_static_06_04" && ![regexp "^types-(t(c|dc|ldc|ld)$|t.-tld|tl(l|d)-tld|t(f|d|ld)-tc)" $types match] ) ) } {
-	    setup_xfail gdb/24104 "x86_64-*-linux*"
+	if { $lang == "c++" && $name == "struct_02_01"
+	     && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] } {
+	    setup_kfail gdb/25096 "x86_64-*-linux*"
 	}
 	gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"
 
@@ -154,8 +147,9 @@ proc run_tests { lang types } {
 	    set answer [ get_valueof "" "rtn_str_${name} ()" "XXXX"]
 	    verbose -log "Answer: ${answer}"
 
-	    if { ($lang == "c++" && $name == "struct_02_01" && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] ) } {
-		setup_xfail gdb/24104 "x86_64-*-linux*"
+	    if { $lang == "c++" && $name == "struct_02_01"
+		 && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] } {
+		setup_kfail gdb/25096 "x86_64-*-linux*"
 	    }
 	    gdb_assert [string eq ${answer} ${refval}] ${test}
 	} else {

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_arguments
  2019-10-11 12:14           ` [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_arguments Tom de Vries
@ 2019-10-14 13:10             ` Alan Hayward
  2019-10-14 15:23               ` Tom de Vries
  0 siblings, 1 reply; 12+ messages in thread
From: Alan Hayward @ 2019-10-14 13:10 UTC (permalink / raw)
  To: Tom de Vries; +Cc: Pedro Alves, gdb-patches, nd



> On 11 Oct 2019, at 13:14, Tom de Vries <tdevries@suse.de> wrote:
> 
> [ was: Re: [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size
> in C++ ]
> 
> On 11-10-2019 01:49, Tom de Vries wrote:
>> On 21-01-2019 16:52, Alan Hayward wrote:
>> 
>>> +       if { ( $lang == "c++"
>>> +              && ( ( [regexp "struct_01_0(1|2|3)" $name match] && [regexp "^types-(td($|-)|tl(|l)(|-tf|-td|-tld)$)" $types match] )
>>> +                   || ( $name == "struct_01_02" && $types == "types-tfc" )
>>> +                   || ( $name == "struct_01_04" && [regexp "^types-(tf($|-)|ti(|-tf|-td|-tld)$)" $types match] )
>>> +                   || ( $name == "struct_02_01" && [regexp "^types-tf-t(c|s|i)" $types match] ) ) ) } {
>>> +           setup_xfail gdb/24104 "x86_64-*-linux*"
>>> +       }
>>>        gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"
>> 
>> 
>>> +           if { ($lang == "c++" && $name == "struct_02_01" && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] ) } {
>>> +               setup_xfail gdb/24104 "x86_64-*-linux*"
>>> +           }
>>>            gdb_assert [string eq ${answer} ${refval}] ${test}
>> 
>> These should have been kfails, not xfails.
> 
> This patch fixes PR24104.
> 
> OK for trunk?
> 
> Thanks,
> - Tom
> 
> [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_arguments
> 
> Atm, when executing gdb.base/infcall-nested-structs.exp on x86_64-linux, we get:
> ...
> FAIL: gdb.base/infcall-nested-structs.exp: l=c++: types-tc-tf: \
>  p/d check_arg_struct_02_01 (ref_val_struct_02_01)
> FAIL: gdb.base/infcall-nested-structs.exp: l=c++: types-ts-tf: \
>  p/d check_arg_struct_02_01 (ref_val_struct_02_01)
> FAIL: gdb.base/infcall-nested-structs.exp: l=c++: types-ti-tf: \
>  p/d check_arg_struct_02_01 (ref_val_struct_02_01)
> 
>                === gdb Summary ===
> 
> nr of expected passes            9255
> nr of unexpected failures        3
> nr of expected failures          142
> ...
> 
> The 3 FAILs are reported as PR tdep/25096.
> 
> The 142 XFAILs are for a gdb assertion failure, reported in PR tdep/24104,
> which should have been KFAILs since there's a problem in gdb rather than in
> the environment.
> 
> A minimal version of the assertion failure looks like this. Consider test.c:
> ...
> struct s { struct { } es1; long f; };
> struct s ref = { {}, 'f' };
> 
> int __attribute__((noinline,noclone)) check (struct s arg)
> { return arg.f == 'f'; }
> 
> int main (void)
> { return check (ref); }
> ...
> 
> When calling 'check (ref)' from main, we have '1' as expected:
> ...
> $ g++ test3.c -g && ( ./a.out; echo $? )
> 1
> ...
> 
> But when calling 'check (ref)' from the gdb prompt, we get:
> ...
> $ gdb a.out -batch -ex start -ex "p check (ref)"
> Temporary breakpoint 1 at 0x4004f7: file test.c, line 8.
> 
> Temporary breakpoint 1, main () at test.c:8
> 8       { return check (ref); }
> src/gdb/amd64-tdep.c:982: internal-error: \
>  CORE_ADDR amd64_push_arguments(regcache*, int, value**, CORE_ADDR, \
>                                 function_call_return_method): \
>  Assertion `!"Unexpected register class."' failed.
> ...
> 
> The assert happens in this loop in amd64_push_arguments:
> ...
>          for (j = 0; len > 0; j++, len -= 8)
>            {
>              int regnum = -1;
>              int offset = 0;
> 
>              switch (theclass[j])
>                {
>                case AMD64_INTEGER:
>                  regnum = integer_regnum[integer_reg++];
>                  break;
> 
>                case AMD64_SSE:
>                  regnum = sse_regnum[sse_reg++];
>                  break;
> 
>                case AMD64_SSEUP:
>                  gdb_assert (sse_reg > 0);
>                  regnum = sse_regnum[sse_reg - 1];
>                  offset = 8;
>                  break;
> 
>                default:
>                  gdb_assert (!"Unexpected register class.");
>                }
> 		...
>            }
> ...
> when processing theclass[0], which is AMD64_NO_CLASS:
> ...
> (gdb) p theclass
> $1 = {AMD64_NO_CLASS, AMD64_INTEGER}
> ...
> 
> The layout of struct s is that the empty field es1 occupies one byte (due to
> c++) at offset 0, and the long field f occupies 8 bytes at offset 8.
> 

This makes sense.


> When compiling at -O2, we can see from the disassembly of main:
> ...
>  4003f0:       48 8b 3d 41 0c 20 00    mov    0x200c41(%rip),%rdi \
>                                               # 601038 <ref+0x8>
>  4003f7:       e9 e4 00 00 00          jmpq   4004e0 <_Z5check1s>
>  4003fc:       0f 1f 40 00             nopl   0x0(%rax)
> ...
> that check is called with field f passed in %rdi, meaning that the
> classification in theclass is correct, it's just not supported in the loop in
> amd64_push_arguments mentioned above.
> 
> Fix the assert by implementing support for 'AMD64_NO_CLASS' in that loop.
> 
> This exposes 9 more FAILs of the PR tdep/25096 type, so mark all 12 of them as
> KFAIL.

When I run the test, I get three unexpected passes:


# of expected passes		9388
# of unknown successes		3
# of known failures		9

KPASS: gdb.base/infcall-nested-structs.exp: l=c++: types-tc-tf: p/d check_arg_struct_02_01 (ref_val_struct_02_01) (PRMS gdb/25096)
KPASS: gdb.base/infcall-nested-structs.exp: l=c++: types-ts-tf: p/d check_arg_struct_02_01 (ref_val_struct_02_01) (PRMS gdb/25096)
KPASS: gdb.base/infcall-nested-structs.exp: l=c++: types-ti-tf: p/d check_arg_struct_02_01 (ref_val_struct_02_01) (PRMS gdb/25096)


Other than that, I’m happy with the patch.


Alan.

> 
> Tested on x86_64-linux.
> 
> Tested with g++ 4.8.5, 7.4.1, 8.3.1, 9.2.1.  With 4.8.5, 3 of the 12 KFAILs
> are KPASSing.
> 
> Tested with clang++ 5.0.2 (which requires removing additional_flags=-Wno-psabi
> and adding additional_flags=-Wno-deprecated).
> 
> gdb/ChangeLog:
> 
> 2019-10-11  Tom de Vries  <tdevries@suse.de>
> 
> 	PR tdep/24104
> 	* amd64-tdep.c (amd64_push_arguments): Handle AMD64_NO_CLASS in loop
> 	that handles 'theclass'.
> 
> gdb/testsuite/ChangeLog:
> 
> 2019-10-11  Tom de Vries  <tdevries@suse.de>
> 
> 	PR tdep/24104
> 	* gdb.base/infcall-nested-structs.exp: Remove XFAIL for PR tdep/24104.
> 	Add KFAIL for PR tdep/25096.
> 
> ---
> gdb/amd64-tdep.c                                  |  3 +++
> gdb/testsuite/gdb.base/infcall-nested-structs.exp | 18 ++++++------------
> 2 files changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 232d16d7345..9006ec0167a 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -978,6 +978,9 @@ if (return_method == return_method_struct)
> 		  offset = 8;
> 		  break;
> 
> +		case AMD64_NO_CLASS:
> +		  continue;
> +
> 		default:
> 		  gdb_assert (!"Unexpected register class.");
> 		}
> diff --git a/gdb/testsuite/gdb.base/infcall-nested-structs.exp b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
> index f5fbf44ed16..957eb31bdc2 100644
> --- a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
> +++ b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
> @@ -132,16 +132,9 @@ proc run_tests { lang types } {
> 	    continue
> 	}
> 
> -	if { $lang == "c++"
> -	     && ( ( [regexp "struct_01_0(1|2|3)" $name match] && [regexp "^types-(td($|-)|tl(|l)(|-tf|-td|-tld)$)" $types match] )
> -		  || ( $name == "struct_01_02" && $types == "types-tfc" )
> -		  || ( $name == "struct_01_04" && [regexp "^types-(tf($|-)|ti(|-tf|-td|-tld)$)" $types match] )
> -		  || ( $name == "struct_02_01" && [regexp "^types-tf-t(c|s|i)" $types match] )
> -		  || ( $name == "struct_static_02_02" && [regexp "^types-(t(f|d|ld)-t(d|l|ll)$|t(d|l|ll)$|t(c|s|i|l|ll)-td)" $types match] )
> -		  || ( $name == "struct_static_02_03" && [regexp "^types-(ti-t(f|l|d|)|tf(-|$)|ti$)" $types match] )
> -		  || ( $name == "struct_static_04_02" && [regexp "^types-(t(c|s)-tf|tf-ts)" $types match] )
> -		  || ( $name == "struct_static_06_04" && ![regexp "^types-(t(c|dc|ldc|ld)$|t.-tld|tl(l|d)-tld|t(f|d|ld)-tc)" $types match] ) ) } {
> -	    setup_xfail gdb/24104 "x86_64-*-linux*"
> +	if { $lang == "c++" && $name == "struct_02_01"
> +	     && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] } {
> +	    setup_kfail gdb/25096 "x86_64-*-linux*"
> 	}
> 	gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"
> 
> @@ -154,8 +147,9 @@ proc run_tests { lang types } {
> 	    set answer [ get_valueof "" "rtn_str_${name} ()" "XXXX"]
> 	    verbose -log "Answer: ${answer}"
> 
> -	    if { ($lang == "c++" && $name == "struct_02_01" && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] ) } {
> -		setup_xfail gdb/24104 "x86_64-*-linux*"
> +	    if { $lang == "c++" && $name == "struct_02_01"
> +		 && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] } {
> +		setup_kfail gdb/25096 "x86_64-*-linux*"
> 	    }
> 	    gdb_assert [string eq ${answer} ${refval}] ${test}
> 	} else {


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_arguments
  2019-10-14 13:10             ` Alan Hayward
@ 2019-10-14 15:23               ` Tom de Vries
  0 siblings, 0 replies; 12+ messages in thread
From: Tom de Vries @ 2019-10-14 15:23 UTC (permalink / raw)
  To: Alan Hayward; +Cc: Pedro Alves, gdb-patches, nd

On 14-10-2019 15:10, Alan Hayward wrote:
>> This exposes 9 more FAILs of the PR tdep/25096 type, so mark all 12 of them as
>> KFAIL.
> When I run the test, I get three unexpected passes:
> 
> 
> # of expected passes		9388
> # of unknown successes		3
> # of known failures		9
> 
> KPASS: gdb.base/infcall-nested-structs.exp: l=c++: types-tc-tf: p/d check_arg_struct_02_01 (ref_val_struct_02_01) (PRMS gdb/25096)
> KPASS: gdb.base/infcall-nested-structs.exp: l=c++: types-ts-tf: p/d check_arg_struct_02_01 (ref_val_struct_02_01) (PRMS gdb/25096)
> KPASS: gdb.base/infcall-nested-structs.exp: l=c++: types-ti-tf: p/d check_arg_struct_02_01 (ref_val_struct_02_01) (PRMS gdb/25096)

That could be due to registers happening to have the correct value.

Do these turn into KFAILs if you add:
...
diff --git a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
b/gdb/testsuite/gdb.base/infcall-nested-structs.
exp
index 957eb31bdc2..f62f636aa11 100644
--- a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
+++ b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
@@ -134,6 +134,7 @@ proc run_tests { lang types } {

        if { $lang == "c++" && $name == "struct_02_01"
             && [regexp "^types-(tf-t(c|s|i)|t(c|s|i)-tf)" $types match] } {
+           gdb_test_no_output "set \$xmm0.v2_int64\[0\] = 0"
            setup_kfail gdb/25096 "x86_64-*-linux*"
        }
        gdb_test "p/d check_arg_${name} (ref_val_${name})" "= 1"
...
?

Thanks,
- Tom

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-10-14 15:23 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16 15:57 [PATCH 0/2] AArch64 AAPCS: Improve argument passing Alan Hayward
2019-01-16 15:57 ` [PATCH 1/2] AArch64 AAPCS: Empty structs have non zero size in C++ Alan Hayward
2019-01-17 17:08   ` Pedro Alves
2019-01-18 10:34     ` Alan Hayward
2019-01-21 15:52       ` Alan Hayward
2019-10-10 23:49         ` Tom de Vries
2019-10-11 12:14           ` [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_arguments Tom de Vries
2019-10-14 13:10             ` Alan Hayward
2019-10-14 15:23               ` Tom de Vries
2019-01-16 15:57 ` [PATCH 2/2] AArch64 AAPCS: Ignore static members Alan Hayward
2019-01-17 17:22   ` Pedro Alves
2019-01-18 10:49     ` Alan Hayward

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).