Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64

Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c 
File gdb/amd64-tdep.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c@645 
PS1, Line 645:     theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
If we reach here, isn't subclass[1] always NO_CLASS?  So this if would be unnecessary?



-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................


Patch Set 1:

(1 comment)

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c 
File gdb/amd64-tdep.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c@645 
PS1, Line 645:     theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
> If we reach here, isn't subclass[1] always NO_CLASS?  So this if would be unnecessary?
Doing:
...
   if (pos == 0)
-    theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
+    {
+      gdb_assert (subclass[1] == AMD64_NO_CLASS);
+      theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
+    }
...
gives 180 failure in gdb.base/infcall-nested-structs.exp.

The assert is triggered for the cases where amd64_classify sets theclass[1], in this test-case:
- long double
- double _Complex.

Structurewise, we have (leaving out the massive comment that makes it hard to read):
...
  theclass[pos] = amd64_merge_classes (theclass[pos], subclass[0]);
  if (bitsize <= 64 && pos == 0 && endpos == 1)
    theclass[1] = amd64_merge_classes (theclass[1], subclass[0]);
  if (pos == 0)
    theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
...
so 'reach here' means just pos == 0.

This would actually be more precise:
...
-  if (pos == 0)
+  if (pos == 0 && endpos == 1)
     theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
...
and therefore we could restructure to:
...
  theclass[pos] = amd64_merge_classes (theclass[pos], subclass[0]);
  if (pos == 0 && endpos == 1)
    {
      theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
      if (bitsize <= 64)
        theclass[1] = amd64_merge_classes (theclass[1], subclass[0]);
    }
...
But I don't think we want to do refactoring like this in this commit.



-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................


Patch Set 1:

(1 comment)

So, it LGTM, but is there anybody else that you think should review it?  Andrew or Alan perhaps?

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c 
File gdb/amd64-tdep.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31/1/gdb/amd64-tdep.c@645 
PS1, Line 645:     theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
> Doing: […]
Ah ok that makes it clear.  I thought that non-aggregate fields would always just use the first eightbyte, therefore just set subclass[0] and leave subclass[1] untouched.  But that's not the case, so nevermind.



-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64

[Andrew Burgess (Code Review)]

Andrew Burgess has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................


Patch Set 1: Code-Review+1

The explanation sounds reasonable, the code looks good to me.  I don't claim to be an x86-64 ABI expert though, but assuming no test regressions I'm happy with this change.


-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64

[Tom de Vries (Code Review)]

Tom de Vries has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> (1 comment)
> 
> So, it LGTM, but is there anybody else that you think should review it?  Andrew or Alan perhaps?

Andrew already reviewed by now :)

Given that we're still not at +2, we could ask Alan, but he hasn't registered yet, so AFAIU I can't mark him as reviewer.


-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

Change in binutils-gdb[master]: [gdb/tdep] Fix inferior call arg passing for amd64

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................


Patch Set 1: Code-Review+2

> Patch Set 1:
> 
> > Patch Set 1:
> > 
> > (1 comment)
> > 
> > So, it LGTM, but is there anybody else that you think should review it?  Andrew or Alan perhaps?
> 
> Andrew already reviewed by now :)
> 
> Given that we're still not at +2, we could ask Alan, but he hasn't registered yet, so AFAIU I can't mark him as reviewer.

Yeah, well given the quite extensive test and the fact that Andrew also looked at it, I think it's pretty safe to merge.  Alan, you are welcome to review this post-merge, of course :)


-- 
To view, visit https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
To unsubscribe, or for help writing mail filters, visit https://gnutoolchain-gerrit.osci.io/r/settings

[review] [gdb/tdep] Fix inferior call arg passing for amd64

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................

[gdb/tdep] Fix inferior call arg passing for amd64

We currently have 12 KFAILS in gdb.base/infcall-nested-structs.exp for
PR tdep/25096.

A minimal version of the failure looks like this.  Consider test.c:
...
struct s { int c; struct { int a; float b; } s1; };
struct s ref = { 0, { 'a', 'b' } };

int __attribute__((noinline,noclone)) check (struct s arg)
{ return arg.s1.a == 'a' && arg.s1.b == 'b' && arg.c == 0; }

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

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

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

Temporary breakpoint 1, main () at test.c:8
8       { return check (ref); }
$1 = 0
...

The layout of struct s is this:
- the field c occupies 4 bytes at offset 0,
- the s1.a field occupies 4 bytes at offset 4, and
- the s1.b field occupies 4 bytes at offset 8.

When compiling at -O2, we can see from the disassembly of main:
...
  4003f0:       48 8b 3d 31 0c 20 00    mov    0x200c31(%rip),%rdi \
                                               # 601028 <ref>
  4003f7:       f3 0f 10 05 31 0c 20    movss  0x200c31(%rip),%xmm0 \
                                               # 601030 <ref+0x8>
  4003fe:       00
  4003ff:       e9 ec 00 00 00          jmpq   4004f0 <_Z5check1s>
...
that check is called with fields c and s1.a passed in %rdi, and s1.b passed
in %xmm0.

However, the classification in theclass (a variable representing the first and
second eightbytes, to put it in SYSV X86_64 psABI terms) in
amd64_push_arguments is incorrect:
...
(gdb) p theclass
$1 = {AMD64_INTEGER, AMD64_INTEGER}
...
and therefore the struct is passed using %rdi and %rsi instead of using %rdi
and %xmm0, which explains the failure.

The reason that we're misclassifying the argument in amd64_classify_aggregate
has to do with how nested struct are handled.

Rather than using fields c and s1.a for the first eightbyte, and using field
s1.b for the second eightbyte, instead field c is used for the first
eightbyte, and fields s1.a and s1.b are classified together in an intermediate
eightbyte, which is then used to merge with both the first and second
eightbyte.

Fix this by factoring out a new function amd64_classify_aggregate_field, and
letting it recursively handle fields of nested structs.

Tested on x86_64-linux.

Tested with g++ 4.8.5, 7.4.1, 8.3.1, 9.2.1.

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

gdb/ChangeLog:

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

	PR tdep/25096
	* amd64-tdep.c (amd64_classify_aggregate_field): Factor out of ...
	(amd64_classify_aggregate): ... here.
	(amd64_classify_aggregate_field): Handled fiels of nested structs
	recursively.

gdb/testsuite/ChangeLog:

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

	PR tdep/25096
	* gdb.base/infcall-nested-structs.exp: Remove PR25096 KFAILs.

Change-Id: Id55c74755f0a431ce31223acc86865718ae0c123
---
M gdb/ChangeLog
M gdb/amd64-tdep.c
M gdb/testsuite/ChangeLog
M gdb/testsuite/gdb.base/infcall-nested-structs.exp
4 files changed, 80 insertions(+), 55 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index eeba0ee..8748257 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-10-16  Tom de Vries  <tdevries@suse.de>
 
+	PR tdep/25096
+	* amd64-tdep.c (amd64_classify_aggregate_field): Factor out of ...
+	(amd64_classify_aggregate): ... here.
+	(amd64_classify_aggregate_field): Handled fiels of nested structs
+	recursively.
+
+2019-10-16  Tom de Vries  <tdevries@suse.de>
+
 	PR tdep/24104
 	* amd64-tdep.c (amd64_push_arguments): Handle AMD64_NO_CLASS in loop
 	that handles 'theclass'.
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 9006ec0..67a4d3f 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -579,6 +579,72 @@
   return false;
 }
 
+/* Classify field I of TYPE starting at BITOFFSET according to the rules for
+   structures and union types, and store the result in THECLASS.  */
+
+static void
+amd64_classify_aggregate_field (struct type *type, int i,
+				enum amd64_reg_class theclass[2],
+				unsigned int bitoffset)
+{
+  struct type *subtype = check_typedef (TYPE_FIELD_TYPE (type, i));
+  int bitpos = bitoffset + TYPE_FIELD_BITPOS (type, i);
+  int pos = bitpos / 64;
+  enum amd64_reg_class subclass[2];
+  int bitsize = TYPE_FIELD_BITSIZE (type, i);
+  int endpos;
+
+  if (bitsize == 0)
+    bitsize = TYPE_LENGTH (subtype) * 8;
+  endpos = (bitpos + bitsize - 1) / 64;
+
+  /* Ignore static fields, or empty fields, for example nested
+     empty structures.*/
+  if (field_is_static (&TYPE_FIELD (type, i)) || bitsize == 0)
+    return;
+
+  if (TYPE_CODE (subtype) == TYPE_CODE_STRUCT
+      || TYPE_CODE (subtype) == TYPE_CODE_UNION)
+    {
+      /* Each field of an object is classified recursively.  */
+      int j;
+      for (j = 0; j < TYPE_NFIELDS (subtype); j++)
+	amd64_classify_aggregate_field (subtype, j, theclass, bitpos);
+      return;
+    }
+
+  gdb_assert (pos == 0 || pos == 1);
+
+  amd64_classify (subtype, subclass);
+  theclass[pos] = amd64_merge_classes (theclass[pos], subclass[0]);
+  if (bitsize <= 64 && pos == 0 && endpos == 1)
+    /* This is a bit of an odd case:  We have a field that would
+       normally fit in one of the two eightbytes, except that
+       it is placed in a way that this field straddles them.
+       This has been seen with a structure containing an array.
+
+       The ABI is a bit unclear in this case, but we assume that
+       this field's class (stored in subclass[0]) must also be merged
+       into class[1].  In other words, our field has a piece stored
+       in the second eight-byte, and thus its class applies to
+       the second eight-byte as well.
+
+       In the case where the field length exceeds 8 bytes,
+       it should not be necessary to merge the field class
+       into class[1].  As LEN > 8, subclass[1] is necessarily
+       different from AMD64_NO_CLASS.  If subclass[1] is equal
+       to subclass[0], then the normal class[1]/subclass[1]
+       merging will take care of everything.  For subclass[1]
+       to be different from subclass[0], I can only see the case
+       where we have a SSE/SSEUP or X87/X87UP pair, which both
+       use up all 16 bytes of the aggregate, and are already
+       handled just fine (because each portion sits on its own
+       8-byte).  */
+    theclass[1] = amd64_merge_classes (theclass[1], subclass[0]);
+  if (pos == 0)
+    theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
+}
+
 /* Classify TYPE according to the rules for aggregate (structures and
    arrays) and union types, and store the result in CLASS.  */
 
@@ -619,53 +685,7 @@
 		  || TYPE_CODE (type) == TYPE_CODE_UNION);
 
       for (i = 0; i < TYPE_NFIELDS (type); i++)
-	{
-	  struct type *subtype = check_typedef (TYPE_FIELD_TYPE (type, i));
-	  int pos = TYPE_FIELD_BITPOS (type, i) / 64;
-	  enum amd64_reg_class subclass[2];
-	  int bitsize = TYPE_FIELD_BITSIZE (type, i);
-	  int endpos;
-
-	  if (bitsize == 0)
-	    bitsize = TYPE_LENGTH (subtype) * 8;
-	  endpos = (TYPE_FIELD_BITPOS (type, i) + bitsize - 1) / 64;
-
-	  /* Ignore static fields, or empty fields, for example nested
-	     empty structures.*/
-	  if (field_is_static (&TYPE_FIELD (type, i)) || bitsize == 0)
-	    continue;
-
-	  gdb_assert (pos == 0 || pos == 1);
-
-	  amd64_classify (subtype, subclass);
-	  theclass[pos] = amd64_merge_classes (theclass[pos], subclass[0]);
-	  if (bitsize <= 64 && pos == 0 && endpos == 1)
-	    /* This is a bit of an odd case:  We have a field that would
-	       normally fit in one of the two eightbytes, except that
-	       it is placed in a way that this field straddles them.
-	       This has been seen with a structure containing an array.
-
-	       The ABI is a bit unclear in this case, but we assume that
-	       this field's class (stored in subclass[0]) must also be merged
-	       into class[1].  In other words, our field has a piece stored
-	       in the second eight-byte, and thus its class applies to
-	       the second eight-byte as well.
-
-	       In the case where the field length exceeds 8 bytes,
-	       it should not be necessary to merge the field class
-	       into class[1].  As LEN > 8, subclass[1] is necessarily
-	       different from AMD64_NO_CLASS.  If subclass[1] is equal
-	       to subclass[0], then the normal class[1]/subclass[1]
-	       merging will take care of everything.  For subclass[1]
-	       to be different from subclass[0], I can only see the case
-	       where we have a SSE/SSEUP or X87/X87UP pair, which both
-	       use up all 16 bytes of the aggregate, and are already
-	       handled just fine (because each portion sits on its own
-	       8-byte).  */
-	    theclass[1] = amd64_merge_classes (theclass[1], subclass[0]);
-	  if (pos == 0)
-	    theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
-	}
+	amd64_classify_aggregate_field (type, i, theclass, 0);
     }
 
   /* 4. Then a post merger cleanup is done:  */
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3277ee3..befc4bb 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2019-10-16  Tom de Vries  <tdevries@suse.de>
 
+	PR tdep/25096
+	* gdb.base/infcall-nested-structs.exp: Remove PR25096 KFAILs.
+
+2019-10-16  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.
diff --git a/gdb/testsuite/gdb.base/infcall-nested-structs.exp b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
index 957eb31..982149f 100644
--- a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
+++ b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
@@ -132,10 +132,6 @@
 	    continue
 	}
 
-	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"
 
 	set refval [ get_valueof "" "ref_val_${name}" "" ]
@@ -147,10 +143,6 @@
 	    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_kfail gdb/25096 "x86_64-*-linux*"
-	    }
 	    gdb_assert [string eq ${answer} ${refval}] ${test}
 	} else {
 	    unresolved $test

[review] [gdb/tdep] Fix inferior call arg passing for amd64

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has uploaded a new patch set version (#2) to the change originally created by Tom de Vries.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/31
......................................................................

[gdb/tdep] Fix inferior call arg passing for amd64

We currently have 12 KFAILS in gdb.base/infcall-nested-structs.exp for
PR tdep/25096.

A minimal version of the failure looks like this.  Consider test.c:
...
struct s { int c; struct { int a; float b; } s1; };
struct s ref = { 0, { 'a', 'b' } };

int __attribute__((noinline,noclone)) check (struct s arg)
{ return arg.s1.a == 'a' && arg.s1.b == 'b' && arg.c == 0; }

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

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

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

Temporary breakpoint 1, main () at test.c:8
8       { return check (ref); }
$1 = 0
...

The layout of struct s is this:
- the field c occupies 4 bytes at offset 0,
- the s1.a field occupies 4 bytes at offset 4, and
- the s1.b field occupies 4 bytes at offset 8.

When compiling at -O2, we can see from the disassembly of main:
...
  4003f0:       48 8b 3d 31 0c 20 00    mov    0x200c31(%rip),%rdi \
                                               # 601028 <ref>
  4003f7:       f3 0f 10 05 31 0c 20    movss  0x200c31(%rip),%xmm0 \
                                               # 601030 <ref+0x8>
  4003fe:       00
  4003ff:       e9 ec 00 00 00          jmpq   4004f0 <_Z5check1s>
...
that check is called with fields c and s1.a passed in %rdi, and s1.b passed
in %xmm0.

However, the classification in theclass (a variable representing the first and
second eightbytes, to put it in SYSV X86_64 psABI terms) in
amd64_push_arguments is incorrect:
...
(gdb) p theclass
$1 = {AMD64_INTEGER, AMD64_INTEGER}
...
and therefore the struct is passed using %rdi and %rsi instead of using %rdi
and %xmm0, which explains the failure.

The reason that we're misclassifying the argument in amd64_classify_aggregate
has to do with how nested struct are handled.

Rather than using fields c and s1.a for the first eightbyte, and using field
s1.b for the second eightbyte, instead field c is used for the first
eightbyte, and fields s1.a and s1.b are classified together in an intermediate
eightbyte, which is then used to merge with both the first and second
eightbyte.

Fix this by factoring out a new function amd64_classify_aggregate_field, and
letting it recursively handle fields of nested structs.

Tested on x86_64-linux.

Tested with g++ 4.8.5, 7.4.1, 8.3.1, 9.2.1.

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

gdb/ChangeLog:

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

	PR tdep/25096
	* amd64-tdep.c (amd64_classify_aggregate_field): Factor out of ...
	(amd64_classify_aggregate): ... here.
	(amd64_classify_aggregate_field): Handled fiels of nested structs
	recursively.

gdb/testsuite/ChangeLog:

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

	PR tdep/25096
	* gdb.base/infcall-nested-structs.exp: Remove PR25096 KFAILs.

Change-Id: Id55c74755f0a431ce31223acc86865718ae0c123
---
M gdb/ChangeLog
M gdb/amd64-tdep.c
M gdb/testsuite/ChangeLog
M gdb/testsuite/gdb.base/infcall-nested-structs.exp
4 files changed, 80 insertions(+), 55 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index eeba0ee..8748257 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,13 @@
 2019-10-16  Tom de Vries  <tdevries@suse.de>
 
+	PR tdep/25096
+	* amd64-tdep.c (amd64_classify_aggregate_field): Factor out of ...
+	(amd64_classify_aggregate): ... here.
+	(amd64_classify_aggregate_field): Handled fiels of nested structs
+	recursively.
+
+2019-10-16  Tom de Vries  <tdevries@suse.de>
+
 	PR tdep/24104
 	* amd64-tdep.c (amd64_push_arguments): Handle AMD64_NO_CLASS in loop
 	that handles 'theclass'.
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 9006ec0..67a4d3f 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -579,6 +579,72 @@
   return false;
 }
 
+/* Classify field I of TYPE starting at BITOFFSET according to the rules for
+   structures and union types, and store the result in THECLASS.  */
+
+static void
+amd64_classify_aggregate_field (struct type *type, int i,
+				enum amd64_reg_class theclass[2],
+				unsigned int bitoffset)
+{
+  struct type *subtype = check_typedef (TYPE_FIELD_TYPE (type, i));
+  int bitpos = bitoffset + TYPE_FIELD_BITPOS (type, i);
+  int pos = bitpos / 64;
+  enum amd64_reg_class subclass[2];
+  int bitsize = TYPE_FIELD_BITSIZE (type, i);
+  int endpos;
+
+  if (bitsize == 0)
+    bitsize = TYPE_LENGTH (subtype) * 8;
+  endpos = (bitpos + bitsize - 1) / 64;
+
+  /* Ignore static fields, or empty fields, for example nested
+     empty structures.*/
+  if (field_is_static (&TYPE_FIELD (type, i)) || bitsize == 0)
+    return;
+
+  if (TYPE_CODE (subtype) == TYPE_CODE_STRUCT
+      || TYPE_CODE (subtype) == TYPE_CODE_UNION)
+    {
+      /* Each field of an object is classified recursively.  */
+      int j;
+      for (j = 0; j < TYPE_NFIELDS (subtype); j++)
+	amd64_classify_aggregate_field (subtype, j, theclass, bitpos);
+      return;
+    }
+
+  gdb_assert (pos == 0 || pos == 1);
+
+  amd64_classify (subtype, subclass);
+  theclass[pos] = amd64_merge_classes (theclass[pos], subclass[0]);
+  if (bitsize <= 64 && pos == 0 && endpos == 1)
+    /* This is a bit of an odd case:  We have a field that would
+       normally fit in one of the two eightbytes, except that
+       it is placed in a way that this field straddles them.
+       This has been seen with a structure containing an array.
+
+       The ABI is a bit unclear in this case, but we assume that
+       this field's class (stored in subclass[0]) must also be merged
+       into class[1].  In other words, our field has a piece stored
+       in the second eight-byte, and thus its class applies to
+       the second eight-byte as well.
+
+       In the case where the field length exceeds 8 bytes,
+       it should not be necessary to merge the field class
+       into class[1].  As LEN > 8, subclass[1] is necessarily
+       different from AMD64_NO_CLASS.  If subclass[1] is equal
+       to subclass[0], then the normal class[1]/subclass[1]
+       merging will take care of everything.  For subclass[1]
+       to be different from subclass[0], I can only see the case
+       where we have a SSE/SSEUP or X87/X87UP pair, which both
+       use up all 16 bytes of the aggregate, and are already
+       handled just fine (because each portion sits on its own
+       8-byte).  */
+    theclass[1] = amd64_merge_classes (theclass[1], subclass[0]);
+  if (pos == 0)
+    theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
+}
+
 /* Classify TYPE according to the rules for aggregate (structures and
    arrays) and union types, and store the result in CLASS.  */
 
@@ -619,53 +685,7 @@
 		  || TYPE_CODE (type) == TYPE_CODE_UNION);
 
       for (i = 0; i < TYPE_NFIELDS (type); i++)
-	{
-	  struct type *subtype = check_typedef (TYPE_FIELD_TYPE (type, i));
-	  int pos = TYPE_FIELD_BITPOS (type, i) / 64;
-	  enum amd64_reg_class subclass[2];
-	  int bitsize = TYPE_FIELD_BITSIZE (type, i);
-	  int endpos;
-
-	  if (bitsize == 0)
-	    bitsize = TYPE_LENGTH (subtype) * 8;
-	  endpos = (TYPE_FIELD_BITPOS (type, i) + bitsize - 1) / 64;
-
-	  /* Ignore static fields, or empty fields, for example nested
-	     empty structures.*/
-	  if (field_is_static (&TYPE_FIELD (type, i)) || bitsize == 0)
-	    continue;
-
-	  gdb_assert (pos == 0 || pos == 1);
-
-	  amd64_classify (subtype, subclass);
-	  theclass[pos] = amd64_merge_classes (theclass[pos], subclass[0]);
-	  if (bitsize <= 64 && pos == 0 && endpos == 1)
-	    /* This is a bit of an odd case:  We have a field that would
-	       normally fit in one of the two eightbytes, except that
-	       it is placed in a way that this field straddles them.
-	       This has been seen with a structure containing an array.
-
-	       The ABI is a bit unclear in this case, but we assume that
-	       this field's class (stored in subclass[0]) must also be merged
-	       into class[1].  In other words, our field has a piece stored
-	       in the second eight-byte, and thus its class applies to
-	       the second eight-byte as well.
-
-	       In the case where the field length exceeds 8 bytes,
-	       it should not be necessary to merge the field class
-	       into class[1].  As LEN > 8, subclass[1] is necessarily
-	       different from AMD64_NO_CLASS.  If subclass[1] is equal
-	       to subclass[0], then the normal class[1]/subclass[1]
-	       merging will take care of everything.  For subclass[1]
-	       to be different from subclass[0], I can only see the case
-	       where we have a SSE/SSEUP or X87/X87UP pair, which both
-	       use up all 16 bytes of the aggregate, and are already
-	       handled just fine (because each portion sits on its own
-	       8-byte).  */
-	    theclass[1] = amd64_merge_classes (theclass[1], subclass[0]);
-	  if (pos == 0)
-	    theclass[1] = amd64_merge_classes (theclass[1], subclass[1]);
-	}
+	amd64_classify_aggregate_field (type, i, theclass, 0);
     }
 
   /* 4. Then a post merger cleanup is done:  */
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3277ee3..befc4bb 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,10 @@
 2019-10-16  Tom de Vries  <tdevries@suse.de>
 
+	PR tdep/25096
+	* gdb.base/infcall-nested-structs.exp: Remove PR25096 KFAILs.
+
+2019-10-16  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.
diff --git a/gdb/testsuite/gdb.base/infcall-nested-structs.exp b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
index 957eb31..982149f 100644
--- a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
+++ b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
@@ -132,10 +132,6 @@
 	    continue
 	}
 
-	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"
 
 	set refval [ get_valueof "" "ref_val_${name}" "" ]
@@ -147,10 +143,6 @@
 	    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_kfail gdb/25096 "x86_64-*-linux*"
-	    }
 	    gdb_assert [string eq ${answer} ${refval}] ${test}
 	} else {
 	    unresolved $test