On Mon, Apr 27, 2015 at 5:04 PM, Tom de Vries <tom_devr...@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_devr...@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.
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
