[PATCH] Fix ms-sysv.exp testsuite FAILs (PR c/83117)

[PATCH] Fix ms-sysv.exp testsuite FAILs (PR c/83117)

[Jakub Jelinek]

Hi!

As mentioned in the PR, my C FE rvalue folding patch allows folding
const variable initializers into the uses of those variables in rvalue
contexts more than before, and so we get warnings about UB in the test,
because an unprototyped function is cast to a function type with ellipsis in
it.

It isn't entirely clear what exactly the test wants to test, as mentioned
in the PR, this is one of the options how to solve it, by dropping the
const it can't be optimized in the FEs (the optimizers can still figure out
the static vars are never written to).  Another option would be just
add -w to dg-options, another one is const volatile.

Regtested on x86_64-linux and i686-linux, ok for trunk?

2017-11-27  Jakub Jelinek  <jakub@redhat.com>

	PR c/83117
	* gcc.target/x86_64/abi/ms-sysv/gen.cc (make_do_tests_decl): Drop
	const from do_test_{u,v}*.

--- gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/gen.cc.jj	2017-05-22 10:49:45.000000000 +0200
+++ gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/gen.cc	2017-11-27 11:57:14.889570915 +0100
@@ -392,7 +392,7 @@ static void make_do_tests_decl (const ve
 	    continue;
 
 	  comma.reset ();
-	  out << "static __attribute__ ((ms_abi)) long (*const do_test_"
+	  out << "static __attribute__ ((ms_abi)) long (*do_test_"
 	      << (unaligned ? "u" : "")
 	      << (varargs ? "v" : "") << i << ") (";
 

	Jakub

Re: [PATCH] Fix ms-sysv.exp testsuite FAILs (PR c/83117)

[Daniel Santos]

On 11/27/2017 04:34 PM, Jakub Jelinek wrote:
> Hi!
>
> As mentioned in the PR, my C FE rvalue folding patch allows folding
> const variable initializers into the uses of those variables in rvalue
> contexts more than before, and so we get warnings about UB in the test,
> because an unprototyped function is cast to a function type with ellipsis in
> it.
>
> It isn't entirely clear what exactly the test wants to test, as mentioned
> in the PR, this is one of the options how to solve it, by dropping the
> const it can't be optimized in the FEs (the optimizers can still figure out
> the static vars are never written to).  Another option would be just
> add -w to dg-options, another one is const volatile.
>
> Regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2017-11-27  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR c/83117
> 	* gcc.target/x86_64/abi/ms-sysv/gen.cc (make_do_tests_decl): Drop
> 	const from do_test_{u,v}*.
>
> --- gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/gen.cc.jj	2017-05-22 10:49:45.000000000 +0200
> +++ gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/gen.cc	2017-11-27 11:57:14.889570915 +0100
> @@ -392,7 +392,7 @@ static void make_do_tests_decl (const ve
>  	    continue;
>  
>  	  comma.reset ();
> -	  out << "static __attribute__ ((ms_abi)) long (*const do_test_"
> +	  out << "static __attribute__ ((ms_abi)) long (*do_test_"
>  	      << (unaligned ? "u" : "")
>  	      << (varargs ? "v" : "") << i << ") (";
>  
>
> 	Jakub
>

I don't have a problem with removing const, it's only there for
const-correctness and caution.  I just posted to the PR a bit ago and
I'm curious if there is a better approach when using assembly stubs that
are meant to be called in varying ways.  CV would work also, although
there's no real need to refetch the address before each use.

If you don't have a better way to do this then please use this patch.

Thanks!
Daniel

Re: [PATCH] Fix ms-sysv.exp testsuite FAILs (PR c/83117)

[Jeff Law]

On 11/27/2017 03:34 PM, Jakub Jelinek wrote:
> Hi!
> 
> As mentioned in the PR, my C FE rvalue folding patch allows folding
> const variable initializers into the uses of those variables in rvalue
> contexts more than before, and so we get warnings about UB in the test,
> because an unprototyped function is cast to a function type with ellipsis in
> it.
> 
> It isn't entirely clear what exactly the test wants to test, as mentioned
> in the PR, this is one of the options how to solve it, by dropping the
> const it can't be optimized in the FEs (the optimizers can still figure out
> the static vars are never written to).  Another option would be just
> add -w to dg-options, another one is const volatile.
> 
> Regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2017-11-27  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/83117
> 	* gcc.target/x86_64/abi/ms-sysv/gen.cc (make_do_tests_decl): Drop
> 	const from do_test_{u,v}*.
OK.
jeff

Re: [PATCH] Fix ms-sysv.exp testsuite FAILs (PR c/83117)

[Jakub Jelinek]

On Mon, Nov 27, 2017 at 05:02:32PM -0600, Daniel Santos wrote:
> > --- gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/gen.cc.jj	2017-05-22 10:49:45.000000000 +0200
> > +++ gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/gen.cc	2017-11-27 11:57:14.889570915 +0100
> > @@ -392,7 +392,7 @@ static void make_do_tests_decl (const ve
> >  	    continue;
> >  
> >  	  comma.reset ();
> > -	  out << "static __attribute__ ((ms_abi)) long (*const do_test_"
> > +	  out << "static __attribute__ ((ms_abi)) long (*do_test_"
> >  	      << (unaligned ? "u" : "")
> >  	      << (varargs ? "v" : "") << i << ") (";
> 
> I don't have a problem with removing const, it's only there for
> const-correctness and caution.  I just posted to the PR a bit ago and
> I'm curious if there is a better approach when using assembly stubs that
> are meant to be called in varying ways.  CV would work also, although
> there's no real need to refetch the address before each use.
> 
> If you don't have a better way to do this then please use this patch.

I've verified the resulting *.optimized dump as well as assembly is
practically identical without/with the patch, only differences are in
SSA_NAME versions, in assembly the .LCNNNN and .LCFINNNN constants are
different but otherwise it is the same - the functions are emitted in
different orders by cgraph and committed the patch.

Using assembly stubs that are meant to be called in varying ways should
just be avoided in portable programs, you could e.g. in the generator
instead of all those:
extern __attribute__ ((ms_abi)) long do_test_aligned ();
extern __attribute__ ((ms_abi)) long do_test_unaligned ();
static __attribute__ ((ms_abi)) long (*do_test_1) (long a) = (void*)do_test_aligned;
static __attribute__ ((ms_abi)) long (*do_test_v1) (long a, ...) = (void*)do_test_aligned;
static __attribute__ ((ms_abi)) long (*do_test_u1) (long a) = (void*)do_test_unaligned;
static __attribute__ ((ms_abi)) long (*do_test_uv1) (long a, ...) = (void*)do_test_unaligned;
emit:
extern __attribute__ ((ms_abi)) long do_test_1 (long a);
asm (".text; do_test_1: jmp do_test_aligned; .previous");
extern __attribute__ ((ms_abi)) long do_test_v1 (long a, ...);
asm (".text; do_test_v1: jmp do_test_aligned; .previous");
extern __attribute__ ((ms_abi)) long do_test_1 (long a);
asm (".text; do_test_u1: jmp do_test_unaligned; .previous");
extern __attribute__ ((ms_abi)) long do_test_1 (long a, ...);
asm (".text; do_test_uv1: jmp do_test_unaligned; .previous");
or something similar.

	Jakub

Re: [PATCH] Fix ms-sysv.exp testsuite FAILs (PR c/83117)

[Daniel Santos]

On 11/28/2017 05:22 AM, Jakub Jelinek wrote:
> On Mon, Nov 27, 2017 at 05:02:32PM -0600, Daniel Santos wrote:
>>> --- gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/gen.cc.jj	2017-05-22 10:49:45.000000000 +0200
>>> +++ gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/gen.cc	2017-11-27 11:57:14.889570915 +0100
>>> @@ -392,7 +392,7 @@ static void make_do_tests_decl (const ve
>>>  	    continue;
>>>  
>>>  	  comma.reset ();
>>> -	  out << "static __attribute__ ((ms_abi)) long (*const do_test_"
>>> +	  out << "static __attribute__ ((ms_abi)) long (*do_test_"
>>>  	      << (unaligned ? "u" : "")
>>>  	      << (varargs ? "v" : "") << i << ") (";
>> I don't have a problem with removing const, it's only there for
>> const-correctness and caution.  I just posted to the PR a bit ago and
>> I'm curious if there is a better approach when using assembly stubs that
>> are meant to be called in varying ways.  CV would work also, although
>> there's no real need to refetch the address before each use.
>>
>> If you don't have a better way to do this then please use this patch.
> I've verified the resulting *.optimized dump as well as assembly is
> practically identical without/with the patch, only differences are in
> SSA_NAME versions, in assembly the .LCNNNN and .LCFINNNN constants are
> different but otherwise it is the same - the functions are emitted in
> different orders by cgraph and committed the patch.
>
> Using assembly stubs that are meant to be called in varying ways should
> just be avoided in portable programs, you could e.g. in the generator
> instead of all those:
> extern __attribute__ ((ms_abi)) long do_test_aligned ();
> extern __attribute__ ((ms_abi)) long do_test_unaligned ();
> static __attribute__ ((ms_abi)) long (*do_test_1) (long a) = (void*)do_test_aligned;
> static __attribute__ ((ms_abi)) long (*do_test_v1) (long a, ...) = (void*)do_test_aligned;
> static __attribute__ ((ms_abi)) long (*do_test_u1) (long a) = (void*)do_test_unaligned;
> static __attribute__ ((ms_abi)) long (*do_test_uv1) (long a, ...) = (void*)do_test_unaligned;
> emit:
> extern __attribute__ ((ms_abi)) long do_test_1 (long a);
> asm (".text; do_test_1: jmp do_test_aligned; .previous");
> extern __attribute__ ((ms_abi)) long do_test_v1 (long a, ...);
> asm (".text; do_test_v1: jmp do_test_aligned; .previous");
> extern __attribute__ ((ms_abi)) long do_test_1 (long a);
> asm (".text; do_test_u1: jmp do_test_unaligned; .previous");
> extern __attribute__ ((ms_abi)) long do_test_1 (long a, ...);
> asm (".text; do_test_uv1: jmp do_test_unaligned; .previous");
> or something similar.
>
> 	Jakub

Ah hah! That would indeed work. Thanks for the tip.  I have some
improvements to make to this set of tests, mostly tests triggered by
GCC_TEST_RUN_EXPENSIVE, but perhaps I can make this modification as
well.  Come to think of it, attribute naked might work too.

Thanks,
Daniel