diff mbox

RFA: PATCH to match.pd for c++/68385

Message ID 564F7B57.4060703@redhat.com
State New
Headers show

Commit Message

Jason Merrill Nov. 20, 2015, 7:58 p.m. UTC 
In this bug, we hit the (A & sign-bit) != 0 -> A < 0 transformation. 
Because of delayed folding, the operands aren't fully folded yet, so we 
have NOP_EXPRs around INTEGER_CSTs, and so calling wi::only_sign_bit_p 
ICEs.  We've been seeing several similar bugs, where code calls 
integer_zerop and therefore assumes that they have an INTEGER_CST, but 
in fact integer_zerop does STRIP_NOPS.

This patch changes the pattern to only match if the operand is actually 
an INTEGER_CST.  Alternatively we could call tree_strip_nop_conversions 
on the operand, but I would expect that to have issues when the 
conversion changes the signedness of the type.

OK if testing passes?

Comments

Jason Merrill Nov. 20, 2015, 8:38 p.m. UTC | #1 
On 11/20/2015 02:58 PM, Jason Merrill wrote:
> OK if testing passes?

Which it did.

Jason
Richard Biener Nov. 21, 2015, 6:30 a.m. UTC | #2 
On November 20, 2015 8:58:15 PM GMT+01:00, Jason Merrill <jason@redhat.com> wrote:
>In this bug, we hit the (A & sign-bit) != 0 -> A < 0 transformation. 
>Because of delayed folding, the operands aren't fully folded yet, so we
>
>have NOP_EXPRs around INTEGER_CSTs, and so calling wi::only_sign_bit_p 
>ICEs.  We've been seeing several similar bugs, where code calls 
>integer_zerop and therefore assumes that they have an INTEGER_CST, but 
>in fact integer_zerop does STRIP_NOPS.
>
>This patch changes the pattern to only match if the operand is actually
>
>an INTEGER_CST.  Alternatively we could call tree_strip_nop_conversions
>
>on the operand, but I would expect that to have issues when the 
>conversion changes the signedness of the type.
>
>OK if testing passes?

What happens if we remove the nops stripping from integer_zerop?  Do other integer predicates strip nops?

Richard.
Marc Glisse Nov. 21, 2015, 6:57 p.m. UTC | #3 
On Sat, 21 Nov 2015, Richard Biener wrote:

> On November 20, 2015 8:58:15 PM GMT+01:00, Jason Merrill <jason@redhat.com> wrote:
>> In this bug, we hit the (A & sign-bit) != 0 -> A < 0 transformation.
>> Because of delayed folding, the operands aren't fully folded yet, so we
>>
>> have NOP_EXPRs around INTEGER_CSTs, and so calling wi::only_sign_bit_p
>> ICEs.  We've been seeing several similar bugs, where code calls
>> integer_zerop and therefore assumes that they have an INTEGER_CST, but
>> in fact integer_zerop does STRIP_NOPS.
>>
>> This patch changes the pattern to only match if the operand is actually
>>
>> an INTEGER_CST.  Alternatively we could call tree_strip_nop_conversions
>>
>> on the operand, but I would expect that to have issues when the
>> conversion changes the signedness of the type.
>>
>> OK if testing passes?
>
> What happens if we remove the nops stripping from integer_zerop?

I had the same reaction.

> Do other integer predicates strip nops?

Yes, they do.

I believe I added one or two of those, and the reason I added STRIP_NOPS 
is because they started as a copy-paste of integer_zerop...
Richard Biener Nov. 23, 2015, 10:12 a.m. UTC | #4 
On Sat, Nov 21, 2015 at 7:57 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Sat, 21 Nov 2015, Richard Biener wrote:
>
>> On November 20, 2015 8:58:15 PM GMT+01:00, Jason Merrill
>> <jason@redhat.com> wrote:
>>>
>>> In this bug, we hit the (A & sign-bit) != 0 -> A < 0 transformation.
>>> Because of delayed folding, the operands aren't fully folded yet, so we
>>>
>>> have NOP_EXPRs around INTEGER_CSTs, and so calling wi::only_sign_bit_p
>>> ICEs.  We've been seeing several similar bugs, where code calls
>>> integer_zerop and therefore assumes that they have an INTEGER_CST, but
>>> in fact integer_zerop does STRIP_NOPS.
>>>
>>> This patch changes the pattern to only match if the operand is actually
>>>
>>> an INTEGER_CST.  Alternatively we could call tree_strip_nop_conversions
>>>
>>> on the operand, but I would expect that to have issues when the
>>> conversion changes the signedness of the type.
>>>
>>> OK if testing passes?
>>
>>
>> What happens if we remove the nops stripping from integer_zerop?
>
>
> I had the same reaction.
>
>> Do other integer predicates strip nops?
>
>
> Yes, they do.
>
> I believe I added one or two of those, and the reason I added STRIP_NOPS is
> because they started as a copy-paste of integer_zerop...

Ok...

Jason, from looking at the PRs backtrace I see the C++ FE does things like

      if (complain & tf_warning)
        warn_logical_operator (loc, code, boolean_type_node,
                               code_orig_arg1, fold (arg1),
                               code_orig_arg2, fold (arg2));

but that's in principle a no-no, if arg1s operands are not folded.
Delayed folding needs
to happen recursively, bottom-up.  Folders generally do not expect
unfolded operands
like (int) 1.

There is c-common.c:c_fully_fold () which does this properly but with

  /* This function is not relevant to C++ because C++ folds while
     parsing, and may need changes to be correct for C++ when C++
     stops folding while parsing.  */
  if (c_dialect_cxx ())
    gcc_unreachable ();

not sure if the C++ FE can re-use this for the diagnostic cases.

Richard.



> --
> Marc Glisse
diff mbox

Patch

commit e7b45ed6775c88c6d48c5863738ba0db2e38fc5e
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Nov 20 14:40:35 2015 -0500

    	PR c++/68385
    
    	* match.pd: Don't assume that integer_pow2p implies INTEGER_CST.

diff --git a/gcc/match.pd b/gcc/match.pd
index e86cc8b..1981ae7 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -2232,6 +2232,7 @@  DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
        && (TYPE_PRECISION (TREE_TYPE (@0))
 	   == GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (@0))))
        && element_precision (@2) >= element_precision (@0)
+       && TREE_CODE (@1) == INTEGER_CST
        && wi::only_sign_bit_p (@1, element_precision (@0)))
    (with { tree stype = signed_type_for (TREE_TYPE (@0)); }
     (ncmp (convert:stype @0) { build_zero_cst (stype); })))))