On 02/09/2015 01:05 PM, Jakub Jelinek wrote:
> On Mon, Feb 09, 2015 at 12:40:05PM +0100, Andreas Krebbel wrote:
>> On 02/09/2015 12:29 PM, Jakub Jelinek wrote:
>>> On Mon, Feb 09, 2015 at 10:50:34AM +0100, Andreas Krebbel wrote:
>>>> Hi,
>>>>
>>>> the attached patch fixes a critical problem in the va_start expansion
>>>> code in the S/390 backend. The problem exists since GCC 4.0.
>>>>
>>>> Ok to commit to 4.9 branch and mainline?
>>>
>>> No. This isn't a backend problem, but a bug in the tree-stdarg.c pass,
>>> so should be fixed there, for all targets, rather than just worked around by
>>> pessimizing unnecessarily one target.
>>
>> I think for the overflow area pointer we would need a different flag
>> indicating whether a pointer to
>> the va_list structure escapes or not. To my understanding it is not correct
>> to only use the
>> va_list_gpr_size/va_list_fpr_size fields for that purpose. These only refer
>> to the va_arg expansions
>> in the current function.
>
> No. The flag for the escapes is
> if (va_list_escapes)
> {
> fun->va_list_gpr_size = VA_LIST_MAX_GPR_SIZE;
> fun->va_list_fpr_size = VA_LIST_MAX_FPR_SIZE;
> }
> and has been that way since forever. Escape case is like no stdarg pass run
> at all (-O0), it is a case where you don't know anything about it.
Ok. I missed that. Thanks!
-Andreas-
>
> What I mean as a fix is something like untested following patch, though that
> only fixes the case on the !va_list_simple_ptr targets like x86_64 or s390*,
> but not say on i?86. Even on i?86 it should say that it escapes, because it
> does, thus something is still wrong in the stdarg pass.
>
> As you can see, the updated testcase fails even on x86_64-linux.
>
> --- gcc/tree-stdarg.c.jj 2015-01-09 21:59:44.000000000 +0100
> +++ gcc/tree-stdarg.c 2015-02-09 12:47:12.020789728 +0100
> @@ -856,22 +856,23 @@ pass_stdarg::execute (function *fun)
> /* For va_list_simple_ptr, we have to check PHI nodes too. We treat
> them as assignments for the purpose of escape analysis. This is
> not needed for non-simple va_list because virtual phis don't perform
> - any real data movement. */
> - if (va_list_simple_ptr)
> - {
> - tree lhs, rhs;
> - use_operand_p uop;
> - ssa_op_iter soi;
> + any real data movement. For !va_list_simple_ptr, check PHI nodes for
> + taking address of the va_list vars. */
> + tree lhs, rhs;
> + use_operand_p uop;
> + ssa_op_iter soi;
>
> - for (gphi_iterator i = gsi_start_phis (bb); !gsi_end_p (i);
> - gsi_next (&i))
> - {
> - gphi *phi = i.phi ();
> - lhs = PHI_RESULT (phi);
> + for (gphi_iterator i = gsi_start_phis (bb); !gsi_end_p (i);
> + gsi_next (&i))
> + {
> + gphi *phi = i.phi ();
> + lhs = PHI_RESULT (phi);
>
> - if (virtual_operand_p (lhs))
> - continue;
> + if (virtual_operand_p (lhs))
> + continue;
>
> + if (va_list_simple_ptr)
> + {
> FOR_EACH_PHI_ARG (uop, phi, soi, SSA_OP_USE)
> {
> rhs = USE_FROM_PTR (uop);
> @@ -894,6 +895,22 @@ pass_stdarg::execute (function *fun)
> }
> }
> }
> + else
> + {
> + for (unsigned j = 0; !va_list_escapes
> + && j < gimple_phi_num_args (phi); ++j)
> + if (walk_tree (gimple_phi_arg_def_ptr (phi, j),
> + find_va_list_reference, &wi, NULL))
> + {
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fputs ("va_list escapes in ", dump_file);
> + print_gimple_stmt (dump_file, phi, 0, dump_flags);
> + fputc ('\n', dump_file);
> + }
> + va_list_escapes = true;
> + }
> + }
> }
>
> for (gimple_stmt_iterator i = gsi_start_bb (bb);
> @@ -916,8 +933,8 @@ pass_stdarg::execute (function *fun)
>
> if (is_gimple_assign (stmt))
> {
> - tree lhs = gimple_assign_lhs (stmt);
> - tree rhs = gimple_assign_rhs1 (stmt);
> + lhs = gimple_assign_lhs (stmt);
> + rhs = gimple_assign_rhs1 (stmt);
>
> if (va_list_simple_ptr)
> {
> --- gcc/testsuite/gcc.dg/tree-ssa/stdarg-7.c.jj 2015-02-09
> 12:54:44.222284401 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/stdarg-7.c 2015-02-09 12:58:10.406875647
> +0100
> @@ -0,0 +1,22 @@
> +/* PR target/64979 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-stdarg" } */
> +
> +#include <stdarg.h>
> +
> +void bar (int x, va_list *ap);
> +
> +void
> +foo (int x, ...)
> +{
> + va_list ap;
> + int n;
> +
> + va_start (ap, x);
> + n = va_arg (ap, int);
> + bar (x, (va_list *) ((n == 0) ? ((void *) 0) : &ap));
> + va_end (ap);
> +}
> +
> +/* { dg-final { scan-tree-dump "foo: va_list escapes 1, needs to save all
> GPR units and all FPR units" "stdarg" } } */
> +/* { dg-final { cleanup-tree-dump "stdarg" } } */
> --- gcc/testsuite/gcc.c-torture/execute/pr64979.c.jj 2015-02-09
> 12:54:01.867984625 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr64979.c 2015-02-09
> 12:55:12.998808652 +0100
> @@ -0,0 +1,36 @@
> +/* PR target/64979 */
> +
> +#include <stdarg.h>
> +
> +void __attribute__((noinline, noclone))
> +bar (int x, va_list *ap)
> +{
> + if (ap)
> + {
> + int i;
> + for (i = 0; i < 10; i++)
> + if (i != va_arg (*ap, int))
> + __builtin_abort ();
> + if (va_arg (*ap, double) != 0.5)
> + __builtin_abort ();
> + }
> +}
> +
> +void __attribute__((noinline, noclone))
> +foo (int x, ...)
> +{
> + va_list ap;
> + int n;
> +
> + va_start (ap, x);
> + n = va_arg (ap, int);
> + bar (x, (va_list *) ((n == 0) ? ((void *) 0) : &ap));
> + va_end (ap);
> +}
> +
> +int
> +main ()
> +{
> + foo (100, 1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0.5);
> + return 0;
> +}
>
>
> Jakub
>
