Re: [patch] Fix ICE on ACATS test for Aarch64 at -O

[Jeff Law <law@redhat.com>]

On 09/16/2016 04:05 PM, Eric Botcazou wrote:
> Hi,
>
> for the attached reduced testcase, the ICE is:
>
> eric@polaris:~/gnat/bugs/P901-028> ~/build/gcc/aarch64-linux/gcc/gnat1 -quiet
> p.adb -O -I ~/build/gcc/aarch64-linux/gcc/ada/rts
> +===========================GNAT BUG DETECTED==============================+
> | 7.0.0 20160914 (experimental) [trunk revision 240142] (aarch64-linux) GCC
> error:|
> | in expand_shift_1, at expmed.c:2451                                      |
> | Error detected around p.adb:21:29
>
> #0  internal_error (gmsgid=0x22327b7 "in %s, at %s:%d")
>     at /home/eric/svn/gcc/gcc/diagnostic.c:1347
> #1  0x0000000001e2373b in fancy_abort (
>     file=0x1f965f8 "/home/eric/svn/gcc/gcc/expmed.c", line=2451,
>     function=0x1f96ce7 <expand_shift_1(tree_code, machine_mode, rtx_def*,
> rtx_def*, rtx_def*, int)::__FUNCTION__> "expand_shift_1")
>     at /home/eric/svn/gcc/gcc/diagnostic.c:1415
> #2  0x0000000000eb435c in expand_shift_1 (code=RSHIFT_EXPR, mode=OImode,
>     shifted=0x7ffff68e02e8, amount=0x7ffff68bcef0, target=0x0, unsignedp=1)
>     at /home/eric/svn/gcc/gcc/expmed.c:2451
> #3  0x0000000000eb43bd in expand_shift (code=RSHIFT_EXPR, mode=OImode,
>     shifted=0x7ffff68e02e8, amount=255, target=0x0, unsignedp=1)
>     at /home/eric/svn/gcc/gcc/expmed.c:2467
> #4  0x0000000000ebefe3 in emit_store_flag (target=0x7ffff68de780, code=NE,
>     op0=0x7ffff68de798, op1=0x7ffff6c3d400, mode=OImode, unsignedp=1,
>     normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5826
> #5  0x0000000000ebe979 in emit_store_flag (target=0x7ffff68de780, code=NE,
>     op0=0x7ffff68dc138, op1=0x7ffff68dc030, mode=OImode, unsignedp=1,
>     normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5670
> #6  0x0000000000ebf0ab in emit_store_flag_force (target=0x7ffff68de780,
>     code=NE, op0=0x7ffff68dc138, op1=0x7ffff68dc030, mode=OImode, unsignedp=1,
>     normalizep=1) at /home/eric/svn/gcc/gcc/expmed.c:5860
> #7  0x0000000000ef0aba in do_store_flag (ops=0x7fffffffd4d0,
>     target=0x7ffff68de780, mode=QImode) at /home/eric/svn/gcc/gcc/expr.c:11408
> #8  0x0000000000ee6873 in expand_expr_real_2 (ops=0x7fffffffd4d0, target=0x0,
>     tmode=QImode, modifier=EXPAND_NORMAL) at
> /home/eric/svn/gcc/gcc/expr.c:9196
>
> It's an attempt at generating a store-flag sequence with OImode and it fails
> because there are no shift operations supported in that mode.  It turns out
> that emit_store_flag_force knows how to fall back on a branchy sequence in
> that case so the attached patch simply removes the assertion.
>
> Tested on x86-64/Linux, OK for the mainline?
>
>
> 2016-09-16  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* expmed.c (expand_shift_1): Remove assertion and adjust comment.
> 	(expand_shift): Adjust comment.
Is this really safe.  If I look at where we call 
expand_shift/expand_shift_1 I get real worried that we could end up 
using that NULL value in a context where it's not expected.

So while emit_store_flag_force knows what to do upon NULL return, I'm 
not sure the other users of expand_shift/expand_shift_1 do.  For example 
in expand_bit_field_1:

       target = expand_shift (LSHIFT_EXPR, mode, target,
                              GET_MODE_BITSIZE (mode) - bitsize, 
NULL_RTX, 0);
       return expand_shift (RSHIFT_EXPR, mode, target,
                            GET_MODE_BITSIZE (mode) - bitsize, NULL_RTX, 0);

If the first call returned NULL, but the 2nd didn't, then we end up with 
a NULL argument in the RSHIFT_EXPR node we create.

or extract_fixed_bit_field_1:

      if (bitnum)
         {
           /* If the field does not already start at the lsb,
              shift it so it does.  */
           /* Maybe propagate the target for the shift.  */
           rtx subtarget = (target != 0 && REG_P (target) ? target : 0);
           if (tmode != mode)
             subtarget = 0;
           op0 = expand_shift (RSHIFT_EXPR, mode, op0, bitnum, 
subtarget, 1);
         }
       /* Convert the value to the desired mode.  */
       if (mode != tmode)
         op0 = convert_to_mode (tmode, op0, 1);

Can we end up with a NULL op0 which we then pass to convert_to_mode?

Is there any way to address this problem in emit_store_flag_force?

jeff