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

[Alan Hayward <Alan dot Hayward at arm dot com>]

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