Change in binutils-gdb[master]: [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_argum...

Change in binutils-gdb[master]: [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_argum...

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

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


Patch Set 1: Code-Review+2

Thanks for the detailed explanation.  This LGTM.


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

Change in binutils-gdb[master]: [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_argum...

[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/+/30
......................................................................


Patch Set 1:

https://sourceware.org/ml/gdb-patches/2019-10/msg00343.html:
...
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"
...
?
...


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

[review] [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_argum...

[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/+/30
......................................................................

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

gdb/testsuite/ChangeLog:

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.

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


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3827c4d..eeba0ee 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+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'.
+
 2019-10-15  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* linespec.c (decode_digits_ordinary): Update comment.
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 232d16d..9006ec0 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -978,6 +978,9 @@
 		  offset = 8;
 		  break;
 
+		case AMD64_NO_CLASS:
+		  continue;
+
 		default:
 		  gdb_assert (!"Unexpected register class.");
 		}
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 877d0de..3277ee3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,11 @@
 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.
+
+2019-10-16  Tom de Vries  <tdevries@suse.de>
+
 	PR testsuite/25059
 	* gdb.cp/local-static.exp (do_test): Add xfails for gcc PR debug/55541.
 
diff --git a/gdb/testsuite/gdb.base/infcall-nested-structs.exp b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
index f5fbf44..957eb31 100644
--- a/gdb/testsuite/gdb.base/infcall-nested-structs.exp
+++ b/gdb/testsuite/gdb.base/infcall-nested-structs.exp
@@ -132,16 +132,9 @@
 	    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 @@
 	    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 {

Re: Change in binutils-gdb[master]: [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_argum...

[Tom de Vries]

On 16-10-2019 13:08, Alan Hayward wrote:
>> On 15 Oct 2019, at 18:23, Alan Hayward <Alan.Hayward@arm.com> wrote:
>>
>>
>>
>>> On 15 Oct 2019, at 09:04, Tom de Vries (Code Review) <gerrit@gnutoolchain-gerrit.osci.io> wrote:
>>>
>>> Tom de Vries has posted comments on this change.
>>>
>>> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/30
>>> ......................................................................
>>>
>>>
>>> Patch Set 1:
>>>
>>> https://sourceware.org/ml/gdb-patches/2019-10/msg00343.html:
>>> ...
>>> 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"
>>> ...
>>> ?
>>> ...
>>
>> I still get KPASSes with that change.
>> Also tried adding the line to the next block in the test with the same if statements, but it didn’t help.
>>
>>
> 
> Sorry, ignore my comment.
> I was accidentally running using the 8.3 branch.
> When I use the latest master, it all works for me.
> 

Thanks, that explains it for me.  The three KPASSES is what you see when
you apply the patch to a version that does not contain the commit
6dfc0041129 "gdb/testsuite: Fix typos in infcall-nested-structs.c".

Thanks,
- Tom

Re: Change in binutils-gdb[master]: [gdb/tdep] Fix 'Unexpected register class' assert in amd64_push_argum...

[Alan Hayward]

> On 15 Oct 2019, at 18:23, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> 
> 
>> On 15 Oct 2019, at 09:04, Tom de Vries (Code Review) <gerrit@gnutoolchain-gerrit.osci.io> wrote:
>> 
>> Tom de Vries has posted comments on this change.
>> 
>> Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/30
>> ......................................................................
>> 
>> 
>> Patch Set 1:
>> 
>> https://sourceware.org/ml/gdb-patches/2019-10/msg00343.html:
>> ...
>> 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"
>> ...
>> ?
>> ...
> 
> I still get KPASSes with that change.
> Also tried adding the line to the next block in the test with the same if statements, but it didn’t help.
> 
> 

Sorry, ignore my comment.
I was accidentally running using the 8.3 branch.
When I use the latest master, it all works for me.

Alan.




> If it helps, my system is:
> 
> Ubuntu 16.04.3 LTS (GNU/Linux 4.4.0-142-generic x86_64)
> 
> 
> End of /proc/cpuinfo:
> 
> processor	: 55
> vendor_id	: GenuineIntel
> cpu family	: 6
> model		: 79
> model name	: Intel(R) Xeon(R) CPU E5-2690 v4 @ 2.60GHz
> stepping	: 1
> microcode	: 0xb000036
> cpu MHz		: 1200.000
> cache size	: 35840 KB
> physical id	: 1
> siblings	: 28
> core id		: 14
> cpu cores	: 14
> apicid		: 61
> initial apicid	: 61
> fpu		: yes
> fpu_exception	: yes
> cpuid level	: 20
> wp		: yes
> flags		: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 sdbg fma cx16 xtpr pdcm pcid dca sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand lahf_lm abm 3dnowprefetch epb invpcid_single intel_pt ssbd ibrs ibpb stibp kaiser tpr_shadow vnmi flexpriority ept vpid fsgsbase tsc_adjust bmi1 hle avx2 smep bmi2 erms invpcid rtm cqm rdseed adx smap xsaveopt cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local dtherm ida arat pln pts flush_l1d
> bugs		: cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf
> bogomips	: 5196.88
> clflush size	: 64
> cache_alignment	: 64
> address sizes	: 46 bits physical, 48 bits virtual
> power management:
> 
> 
>