Fix PR79908

On Tue, Mar 14, 2017 at 10:36 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
>
>> On Mar 14, 2017, at 11:07 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
>>>
>>> Your suggestion failed bootstrap in libiberty on vprintf-support.c.  Compilation failed with:
>>>
>>> /home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/xgcc -B/home/wschmidt/gcc/build/gcc-mainline-test2-debug/gcc/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/bin/ -B/home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/lib/ -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/include -isystem /home/wschmidt/gcc/install/gcc-mainline-test2-debug/powerpc64le-unknown-linux-gnu/sys-include    -c -DHAVE_CONFIG_H -g -O2 -gtoggle  -I. -I/home/wschmidt/gcc/gcc-mainline-test2/libiberty/../include  -W -Wall -Wwrite-strings -Wc++-compat -Wstrict-prototypes -Wshadow=local -pedantic  -D_GNU_SOURCE -fPIC /home/wschmidt/gcc/gcc-mainline-test2/libiberty/vprintf-support.c -o pic/vprintf-support.o
>>>
>>> The initial expression being gimplified is ADDR_EXPR (VAR_DECL (ap)).  Gimplification
>>> turns this into MEM_REF (VAR_DECL (D.4274), 0), and the is_gimple_val test fails on that.
>>
>> Reduced test case:
>>
>> typedef __builtin_va_list __gnuc_va_list;
>> typedef __gnuc_va_list va_list;
>>
>> void
>> foo (va_list args)
>> {
>>  va_list ap;
>>  __builtin_va_copy (ap, args);
>>  (void)__builtin_va_arg (ap, int);
>>  __builtin_va_end(ap);
>> }
>>
>
> Perhaps something like this?  It appears to be doing better on bootstrap, and avoids
> failure on both the original problem and the new test case above:
>
> Index: gcc/tree-stdarg.c
> ===================================================================
> --- gcc/tree-stdarg.c   (revision 246109)
> +++ gcc/tree-stdarg.c   (working copy)
> @@ -1058,7 +1058,13 @@ expand_ifn_va_arg_1 (function *fun)
>             gimplify_assign (lhs, expr, &pre);
>           }
>         else
> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
> +         {
> +           enum gimplify_status status;
> +           status = gimplify_expr (&expr, &pre, &post, is_gimple_lvalue,
> +                                   fb_lvalue | fb_mayfail);
> +           if (status == GS_ERROR)
> +             gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_rvalue);
> +         }
>
>         input_location = saved_location;
>         pop_gimplify_context (NULL);

No, I was confused in thinking gimplify_expr would handle the case
properly.  For
just gimplifying side-effects we should use the middle-end
gimplification machinery:



> Bill

Hi,

> On Mar 17, 2017, at 6:44 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> No, I was confused in thinking gimplify_expr would handle the case
> properly.  For
> just gimplifying side-effects we should use the middle-end
> gimplification machinery:
> 
> Index: tree-stdarg.c
> ===================================================================
> --- tree-stdarg.c       (revision 246188)
> +++ tree-stdarg.c       (working copy)
> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.
> #include "gimple-iterator.h"
> #include "gimple-walk.h"
> #include "gimplify.h"
> +#include "gimplify-me.h"
> #include "tree-into-ssa.h"
> #include "tree-cfg.h"
> #include "tree-stdarg.h"
> @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun)
>            gimplify_assign (lhs, expr, &pre);
>          }
>        else
> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
> +         force_gimple_operand (expr, &pre, false, NULL_TREE);
> 
>        input_location = saved_location;
>        pop_gimplify_context (NULL);
> 
> -       gimple_seq_add_seq (&pre, post);
> +       gimple_seq_add_seq_without_update (&pre, post);
>        update_modified_stmts (pre);
> 
>        /* Add the sequence after IFN_VA_ARG.  This splits the bb right
> @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun)
>        gimple_find_sub_bbs (pre, &i);
> 
>        /* Remove the IFN_VA_ARG gimple_call.  It's the last stmt in the
> -          bb.  */
> +          bb if we added any stmts.  */
>        unlink_stmt_vdef (stmt);
>        release_ssa_name_fn (fun, gimple_vdef (stmt));
>        gsi_remove (&i, true);
> -       gcc_assert (gsi_end_p (i));
> 
>        /* We're walking here into the bbs which contain the expansion of
>           IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs

Looks good, but for some reason I hit a segfault in the linker building 
gengtype when I tried to bootstrap with this.  I assume it's something
latent and unrelated, but will need to investigate.

Bill

> On Mar 17, 2017, at 9:52 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
> 
> Hi,
> 
>> On Mar 17, 2017, at 6:44 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>> 
>> No, I was confused in thinking gimplify_expr would handle the case
>> properly.  For
>> just gimplifying side-effects we should use the middle-end
>> gimplification machinery:
>> 
>> Index: tree-stdarg.c
>> ===================================================================
>> --- tree-stdarg.c       (revision 246188)
>> +++ tree-stdarg.c       (working copy)
>> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.
>> #include "gimple-iterator.h"
>> #include "gimple-walk.h"
>> #include "gimplify.h"
>> +#include "gimplify-me.h"
>> #include "tree-into-ssa.h"
>> #include "tree-cfg.h"
>> #include "tree-stdarg.h"
>> @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun)
>>           gimplify_assign (lhs, expr, &pre);
>>         }
>>       else
>> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>> +         force_gimple_operand (expr, &pre, false, NULL_TREE);
>> 
>>       input_location = saved_location;
>>       pop_gimplify_context (NULL);
>> 
>> -       gimple_seq_add_seq (&pre, post);
>> +       gimple_seq_add_seq_without_update (&pre, post);
>>       update_modified_stmts (pre);
>> 
>>       /* Add the sequence after IFN_VA_ARG.  This splits the bb right
>> @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun)
>>       gimple_find_sub_bbs (pre, &i);
>> 
>>       /* Remove the IFN_VA_ARG gimple_call.  It's the last stmt in the
>> -          bb.  */
>> +          bb if we added any stmts.  */
>>       unlink_stmt_vdef (stmt);
>>       release_ssa_name_fn (fun, gimple_vdef (stmt));
>>       gsi_remove (&i, true);
>> -       gcc_assert (gsi_end_p (i));
>> 
>>       /* We're walking here into the bbs which contain the expansion of
>>          IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs
> 
> Looks good, but for some reason I hit a segfault in the linker building 
> gengtype when I tried to bootstrap with this.  I assume it's something
> latent and unrelated, but will need to investigate.

Ah, I misread the build log.  The link of gengtype succeeds, but gengtype
has apparently been miscompiled (stage2) as it tries to call strlen() with
an address of zero.  So something wrong with this patch.  Looks like the
miscompilation is of libiberty_vprintf_buffer_size in libiberty/vprintf-support.c.

I looked at the before and after dumps and we see that VA_ARGs with
no lhs are just removed from the code, instead of expanding the advance
of the va pointer.

Before:

<L16> [1.39%]:                                                                  
  VA_ARG (&ap, 0B, 0B);                                                         
  goto <bb 22> (<L31>); [100.00%]                                               
                                                                                
<L23> [1.39%]:                                                                  
  VA_ARG (&ap, 0B, 0B);                                                         
  total_width_89 = total_width_17 + 337;                                        
  goto <bb 22> (<L31>); [100.00%]                                               

After:

<L16> [1.39%]:
  goto <bb 22> (<L31>); [100.00%]

<L23> [1.39%]:
  total_width_89 = total_width_17 + 337;
  goto <bb 22> (<L31>); [100.00%]

Thanks,
Bill

On Fri, Mar 17, 2017 at 6:27 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
>
>> On Mar 17, 2017, at 9:52 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
>>
>> Hi,
>>
>>> On Mar 17, 2017, at 6:44 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>>
>>> No, I was confused in thinking gimplify_expr would handle the case
>>> properly.  For
>>> just gimplifying side-effects we should use the middle-end
>>> gimplification machinery:
>>>
>>> Index: tree-stdarg.c
>>> ===================================================================
>>> --- tree-stdarg.c       (revision 246188)
>>> +++ tree-stdarg.c       (working copy)
>>> @@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.
>>> #include "gimple-iterator.h"
>>> #include "gimple-walk.h"
>>> #include "gimplify.h"
>>> +#include "gimplify-me.h"
>>> #include "tree-into-ssa.h"
>>> #include "tree-cfg.h"
>>> #include "tree-stdarg.h"
>>> @@ -1058,12 +1059,12 @@ expand_ifn_va_arg_1 (function *fun)
>>>           gimplify_assign (lhs, expr, &pre);
>>>         }
>>>       else
>>> -         gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue);
>>> +         force_gimple_operand (expr, &pre, false, NULL_TREE);
>>>
>>>       input_location = saved_location;
>>>       pop_gimplify_context (NULL);
>>>
>>> -       gimple_seq_add_seq (&pre, post);
>>> +       gimple_seq_add_seq_without_update (&pre, post);
>>>       update_modified_stmts (pre);
>>>
>>>       /* Add the sequence after IFN_VA_ARG.  This splits the bb right
>>> @@ -1072,11 +1073,10 @@ expand_ifn_va_arg_1 (function *fun)
>>>       gimple_find_sub_bbs (pre, &i);
>>>
>>>       /* Remove the IFN_VA_ARG gimple_call.  It's the last stmt in the
>>> -          bb.  */
>>> +          bb if we added any stmts.  */
>>>       unlink_stmt_vdef (stmt);
>>>       release_ssa_name_fn (fun, gimple_vdef (stmt));
>>>       gsi_remove (&i, true);
>>> -       gcc_assert (gsi_end_p (i));
>>>
>>>       /* We're walking here into the bbs which contain the expansion of
>>>          IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs
>>
>> Looks good, but for some reason I hit a segfault in the linker building
>> gengtype when I tried to bootstrap with this.  I assume it's something
>> latent and unrelated, but will need to investigate.
>
> Ah, I misread the build log.  The link of gengtype succeeds, but gengtype
> has apparently been miscompiled (stage2) as it tries to call strlen() with
> an address of zero.  So something wrong with this patch.  Looks like the
> miscompilation is of libiberty_vprintf_buffer_size in libiberty/vprintf-support.c.
>
> I looked at the before and after dumps and we see that VA_ARGs with
> no lhs are just removed from the code, instead of expanding the advance
> of the va pointer.
>
> Before:
>
> <L16> [1.39%]:
>   VA_ARG (&ap, 0B, 0B);
>   goto <bb 22> (<L31>); [100.00%]
>
> <L23> [1.39%]:
>   VA_ARG (&ap, 0B, 0B);
>   total_width_89 = total_width_17 + 337;
>   goto <bb 22> (<L31>); [100.00%]
>
> After:
>
> <L16> [1.39%]:
>   goto <bb 22> (<L31>); [100.00%]
>
> <L23> [1.39%]:
>   total_width_89 = total_width_17 + 337;
>   goto <bb 22> (<L31>); [100.00%]

Hmm, I think force_gimple_oeprand overwrites what is in &pre so can
you try with using a temporary sequence for force_gimple_operand and
appending that to pre afterwards instead?

> Thanks,
> Bill

On Mar 20, 2017, at 3:26 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> Hmm, I think force_gimple_oeprand overwrites what is in &pre so can
> you try with using a temporary sequence for force_gimple_operand and
> appending that to pre afterwards instead?

Indeed, that solves the problem.  I'll prepare the patch for
review.  Thanks!

Bill