Re: [PATCH 2/2] [testsuite] PR 80759 Remove gas extensions from do-test.S, fix other problems

[Daniel Santos <daniel.santos@pobox.com>]

Thanks you for your assistance Rainer!

On 05/19/2017 04:03 AM, Rainer Orth wrote:
> unfortunately, it still doesn't, as explained in the PR.  The multilib
> support is still wrong/non-existant.

I guess I thought for some reason that would magically appear in 
TEST_ALWAYS_FLAGS. :)  I've explicitly added it for now, but I haven't 
yet found where -m64 gets fed in the normal flow of things and I would 
rather know I'm doing things as closely as possible to how the rest if 
the test harness does it.

>> (I have SVN write privs now, so I can even commit it myself).
> Please always include ChangeLog entries with your patch submissions so
> one can easily see what you've change
> (cf. https://gcc.gnu.org/contribute.html).
>
> Thanks.
>          Rainer

I hate when I forget that!  I'll be sure to remember when I resubmit.

>> Use of .struct in do_test.S causes breakages when gas isn't the
>> assembler (e.g., Solaris).  I also wasn't including TEST_ALWAYS_FLAGS in
>> my CFLAGS resulting in super-ugly log files.  Finally, this patch
>> eliminates spam of "test unsupported" (limiting it to one printing).
>>
>> Signed-off-by: Daniel Santos <daniel.santos@pobox.com>
>> ---
>>   .../gcc.target/x86_64/abi/ms-sysv/do-test.S        | 26 +++++-----------------
>>   .../gcc.target/x86_64/abi/ms-sysv/ms-sysv.c        |  7 ++++++
>>   .../gcc.target/x86_64/abi/ms-sysv/ms-sysv.exp      | 24 ++++++++++++--------
>>   3 files changed, 27 insertions(+), 30 deletions(-)
>>
>> diff --git a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
>> index 1395235fd1e..967eb959fbc 100644
>> --- a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
>> +++ b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
>> @@ -46,22 +46,6 @@ fn:
>>   #  define MOVAPS movaps
>>   # endif
>>   
>> -/* TODO: Is there a cleaner way to provide these offsets?  */
>> -	.struct 0
>> -test_data_save:
>> -
>> -	.struct test_data_save + 224
>> -test_data_input:
>> -
>> -	.struct test_data_save + 448
>> -test_data_output:
>> -
>> -	.struct test_data_save + 672
>> -test_data_fn:
>> -
>> -	.struct test_data_save + 680
>> -test_data_retaddr:
>> -
>>   	.text
>>   
>>   regs_to_mem:
>> @@ -132,23 +116,23 @@ L0:
>>   	call	regs_to_mem
>>   
>>   	# Load register with random data
>> -	lea	test_data + test_data_input(%rip), %rax
>> +	lea	test_data + 224(%rip), %rax
>>   	call	mem_to_regs
>>   
>>   	# Save original return address
>>   	pop	%rax
>> -	movq    %rax, test_data + test_data_retaddr(%rip)
>> +	movq    %rax, test_data + 680(%rip)
>>   
>>   	# Call the test function
>> -	call	*test_data + test_data_fn(%rip)
>> +	call	*test_data + 672(%rip)
>>   
>>   	# Restore the original return address
>> -	movq    test_data + test_data_retaddr(%rip), %rcx
>> +	movq    test_data + 680(%rip), %rcx
>>   	push	%rcx
>>   
>>   	# Save test function return value and store resulting register values
>>   	push	%rax
>> -	lea	test_data + test_data_output(%rip), %rax
>> +	lea	test_data + 448(%rip), %rax
>>   	call	regs_to_mem
>>   
>>   	# Restore registers
>> diff --git a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
>> index 2a011f5103d..7cec312c386 100644
>> --- a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
>> +++ b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
>> @@ -346,6 +346,13 @@ int main (int argc, char *argv[])
>>     assert (!((long)&test_data.regdata[REG_SET_INPUT] & 15));
>>     assert (!((long)&test_data.regdata[REG_SET_OUTPUT] & 15));
>>   
>> +  /* Verify offsets hard-coded into assembly.  */
>> +  assert (__builtin_offsetof (struct test_data, regdata[REG_SET_SAVE]) == 0);
>> +  assert (__builtin_offsetof (struct test_data, regdata[REG_SET_INPUT]) == 224);
>> +  assert (__builtin_offsetof (struct test_data, regdata[REG_SET_OUTPUT]) == 448);
>> +  assert (__builtin_offsetof (struct test_data, fn) == 672);
>> +  assert (__builtin_offsetof (struct test_data, retaddr) == 680);
>> +
>>     while ((c = getopt (argc, argv, "s:f")) != -1)
>>       {
>>         switch (c)
> while .struct is a gas extension and doesn't work with the Solaris/x86
> /bin/as, having the same (mostly unexplained) constants hardcoded in two
> places isn't exactly helpful.  I'd suggest moving them to (say)
> ms-sysv.h and include that from both do-test.S (which is preprocessed
> assembler after all) and ms-sysv.c.
>
> 	Rainer

Well, I don't have an ms-sysv.h, but I suppose I can add one.

I'm starting to lean more towards the idea of plucking out the portion 
of asm that uses these offsets, moving that to an inline asm function 
and having the code in do-test.S just jmp to it.  I wish there was some 
sort of "naked" attribute for x86 since I'm not well versed in every way 
that the compiler can change it in a way that wouldn't be friendly.

void __attribute__((optimize ("-O0 -fno-split-stack")))
do_test_body (void)
{
   __asm__ __volatile__ (
	"# Save registers\n"
"	lea	%0, %%rax\n"
"	call	regs_to_mem\n"
"\n"
"	# Load registers with random data\n"
"	lea	%1, %%rax\n"
"	call	mem_to_regs\n"
"\n"
"	# Save original return address\n"
"	pop	%%rax\n"
"	movq    %%rax, %4\n"
"\n"
"	# Call the test function\n"
"	call	*%3\n"
"\n"
"	# Restore the original return address\n"
"	movq    %4, %%rcx\n"
"	push	%%rcx\n"
"\n"
"	# Save test function return value and store resulting register values\n"
"	push	%%rax\n"
"	lea	%2, %%rax\n"
"	call	regs_to_mem\n"
"\n"
"	# Restore registers\n"
"	lea	%0, %%rax\n"
"	call	mem_to_regs\n"
"	pop	%%rax\n"
"	retq\n\t"
FUNC_END("do_test_unaligned")
FUNC_END("do_test_aligned")
	::
	"m"(test_data.regdata[REG_SET_SAVE]),
	"m"(test_data.regdata[REG_SET_INPUT]),
	"m"(test_data.regdata[REG_SET_OUTPUT]),
	"m"(test_data.fn),
	"m"(test_data.retaddr):);
}

Any thoughts on that approach?  I haven't actually tested it yet. (It 
would probably nice if I put "memory" in the clobber.)

Thanks,
Daniel