Re: [PATCH][PR65818][bootstrap,hppa] Return side-effect free result in gimplify_va_arg_internal

[Tom de Vries <Tom_deVries@mentor.com>]

[-- Attachment #1: Type: text/plain, Size: 5191 bytes --]

On 28-04-15 12:34, Richard Biener wrote:
> On Mon, Apr 27, 2015 at 5:04 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 27-04-15 15:40, Richard Biener wrote:
>>>
>>> On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries <Tom_deVries@mentor.com>
>>> wrote:
>>>>
>>>> On 27-04-15 10:17, Richard Biener wrote:
>>>>>>
>>>>>>
>>>>>> This patch fixes that by gimplifying the address expression of the
>>>>>> mem-ref
>>>>>>>
>>>>>>> returned by the target hook (borrowing code from gimplify_expr, case
>>>>>>> MEM_REF).
>>>>>>>
>>>>>>> Bootstrapped and reg-tested on x86_64.
>>>>>>>
>>>>>>> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11.
>>>>>>>
>>>>>>> OK for trunk?
>>>>>
>>>>>
>>>>> Hmm, that assert looks suspicious...
>>>>>
>>>>> Can't you simply always do
>>>>>
>>>>>      gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>>>>
>>>>
>>>>
>>>> It's a bit counter-intuitive for me, using is_gimple_lvalue for something
>>>> (the result of va_arg) we use as rvalue.
>>>
>>>
>>> Yeah, choosing that was done because you could assert it's a MEM_REF
>>> and tree-stdarg eventually builds a WITH_SIZE_EXPR around it.
>>>
>>> It would possibly be easier to simply "inline" gimplify_va_arg_internal
>>> at its use and only gimplify the assignment.  Though in that case the
>>> original code probably worked - just in the lhs == NULL case it didn't,
>>> which hints at a better place for the fix - in expand_ifn_va_arg_1 do
>>>
>>>    if (lhs != NULL_TREE)
>>>      {
>>> ...
>>>      }
>>>    else
>>>      gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
>>>
>>> So ... if you can re-try that one it's pre-approved.
>>>
>>
>> Using that proposed patch, we run into the following problem.
>>
>> Consider this testcase with ignored-result-va_arg:
>> ...
>> #include <stdarg.h>
>>
>> void
>> f2 (int i, ...)
>> {
>>    va_list ap;
>>    va_start (ap, i);
>>    va_arg (ap, long);
>>    va_end (ap);
>> }
>> ...
>>
>> after gimplify_va_arg_internal we have:
>> ...
>> (gdb) call debug_generic_expr (expr)
>> *(long int * {ref-all}) addr.2
>> ...
>>
>> In a bit more detail:
>> ...
>> (gdb) call debug_tree (expr)
>>   <mem_ref 0x7ffff65ea1b8
>>      type <integer_type 0x7ffff64a77e0 long int DI
>>          size <integer_cst 0x7ffff64a3ca8 constant 64>
>>          unit size <integer_cst 0x7ffff64a3cc0 constant 8>
>>          align 64 symtab 0 alias set -1 canonical type 0x7ffff64a77e0
>> precision 64 min <integer_cst 0x7ffff64a3f30 -9223372036854775808> max
>> <integer_cst 0x7ffff64a3f48 9223372036854775807>
>>          pointer_to_this <pointer_type 0x7ffff65e0498>>
>>
>>      arg 0 <nop_expr 0x7ffff65cbb60
>>          type <pointer_type 0x7ffff65e0498 type <integer_type 0x7ffff64a77e0
>> long int>
>>              public static unsigned DI size <integer_cst 0x7ffff64a3ca8 64>
>> unit size <integer_cst 0x7ffff64a3cc0 8>
>>              align 64 symtab 0 alias set -1 canonical type 0x7ffff65e0498>
>>
>>          arg 0 <var_decl 0x7ffff65e32d0 addr.2 type <pointer_type
>> 0x7ffff64c2150>
>>              used unsigned ignored DI file stdarg-1.c line 4 col 1 size
>> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
>>              align 64 context <function_decl 0x7ffff65c21b0 f2>>>
>>      arg 1 <integer_cst 0x7ffff65e64e0 type <pointer_type 0x7ffff65e0498>
>> constant 0>>
>> ...
>>
>> During 'gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either)' we ICE
>> here:
>> ...
>> 8886      gcc_assert ((*gimple_test_f) (*expr_p));
>> ...
>>
>> At this point expr is:
>> ...
>> (gdb) call debug_generic_expr (*expr_p)
>> *addr.2
>> ...
>>
>> In more detail:
>> ...
>> (gdb) call debug_tree (*expr_p)
>>   <mem_ref 0x7ffff65ea1e0
>>      type <void_type 0x7ffff64c2000 void VOID
>>          align 8 symtab 0 alias set -1 canonical type 0x7ffff64c2000
>>          pointer_to_this <pointer_type 0x7ffff64c2150>>
>>
>>      arg 0 <var_decl 0x7ffff65e32d0 addr.2
>>          type <pointer_type 0x7ffff64c2150 type <void_type 0x7ffff64c2000
>> void>
>>              sizes-gimplified public unsigned DI
>>              size <integer_cst 0x7ffff64a3ca8 constant 64>
>>              unit size <integer_cst 0x7ffff64a3cc0 constant 8>
>>              align 64 symtab 0 alias set -1 canonical type 0x7ffff64c2150
>>              pointer_to_this <pointer_type 0x7ffff64c9bd0>>
>>          used unsigned ignored DI file stdarg-1.c line 4 col 1 size
>> <integer_cst 0x7ffff64a3ca8 64> unit size <integer_cst 0x7ffff64a3cc0 8>
>>          align 64 context <function_decl 0x7ffff65c21b0 f2>>
>>      arg 1 <integer_cst 0x7ffff64c10c0 type <pointer_type 0x7ffff64c2150>
>> constant 0>>
>> ...
>>
>> The memref is now VOID type. And that's not a gimple_val:
>> ...
>> (gdb) p is_gimple_val (*expr_p)
>> $1 = false
>> ...
>
> On which target?  I can't seem to reproduce on x86_64 or i?86.  I also
> can't see how this
> could happen.
>

Reproduced using attached patch on top of r222537, for target x86_64, with and 
without -m32.

Thanks,
- Tom

> Richard.
>
>> Attached patch uses is_gimple_lvalue, but in expand_ifn_va_arg_1.
>>
>> I'll bootstrap and reg-test on x86_64 and commit, unless further comments of
>> course.
>>
>> Thanks,
>> - Tom


[-- Attachment #2: 0001-Reproduce-ICE-in-gimplify_expr-gcc_assert-gimple_tes.patch --]
[-- Type: text/x-patch, Size: 875 bytes --]

Reproduce ICE in 'gimplify_expr:
  gcc_assert ((*gimple_test_f) (*expr_p))'

diff --git a/gcc/testsuite/gcc.dg/reproduce.c b/gcc/testsuite/gcc.dg/reproduce.c
new file mode 100644
index 0000000..c0c8e0b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/reproduce.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#include <stdarg.h>
+
+void
+f2 (int i, ...)
+{
+  va_list ap;
+  va_start (ap, i);
+  va_arg (ap, long);
+  va_end (ap);
+}
diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
index 1356374..829cbc1 100644
--- a/gcc/tree-stdarg.c
+++ b/gcc/tree-stdarg.c
@@ -1079,7 +1079,7 @@ expand_ifn_va_arg_1 (function *fun)
 	    gimplify_assign (lhs, expr, &pre);
 	  }
 	else
-	  gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
+	  gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either);
 
 	pop_gimplify_context (NULL);
 
-- 
1.9.1