On 03/30/2016 01:25 PM, Jason Merrill wrote:
> On 03/30/2016 12:32 PM, Martin Sebor wrote:
>> On 03/30/2016 09:30 AM, Jason Merrill wrote:
>>> On 03/29/2016 11:57 PM, Martin Sebor wrote:
>>>>> Are we confident that arr[0] won't make it here as
>>>>> POINTER_PLUS_EXPR or
>>>>> some such?
>>>>
>>>> I'm as confident as I can be given that this is my first time
>>>> working in this area.  Which piece of code or what assumption
>>>> in particular are you concerned about?
>>>
>>> I want to be sure that we don't fold these conditions to false.
>>>
>>> constexpr int *ip = 0;
>>> constexpr struct A { int ar[3]; } *ap = 0;
>>>
>>> static_assert(&ip[0] == 0);
>>> static_assert(&(ap->ar[0]) == 0);
>>
>> I see.  Thanks for clarifying.  The asserts pass.  The expressions
>> are folded earlier on (in fact, as we discussed, the second one
>> too early and is accepted even though it's undefined and should be
>> rejected in a constexpr context) and never reach fold_comparison.
>
> Good, then let's add at least the first to one of the tests.

I've enhanced the new constexpr-nullptr-1.C test to verify this.
I added assertions exercising the relational expressions as well
and for sanity compiled the test with CLang.  It turns out that
it rejects the relational expressions with null pointers like
the one below complaining they aren't constant.

   constexpr int i = 0;
   constexpr const int *p = &i;
   constexpr int *q = 0;

   static_assert (q < p, "q < p");

I ended up not using a static_assert for the unspecified subset
even though GCC accepts it.  It seems that they really aren't
valid constant expressions (their results are unspecified for
null pointers) and should be rejected.  Do you agree?  (If you
do, I'll add these cases to c++/70248 that's already tracking
another unspecified case that GCC incorrectly accepts).

>> +           /* Avoid folding references to struct members at offset 0 to
>> +          prevent tests like '&ptr->firstmember == 0' from getting
>> +          eliminated.  When ptr is null, although the -> expression
>> +          is strictly speaking invalid, GCC retains it as a matter
>> +          of QoI.  See PR c/44555. */
>> +           && (TREE_CODE (op0) != ADDR_EXPR
>> +           || TREE_CODE (TREE_OPERAND (op0, 0)) != COMPONENT_REF
>> +           || compare_tree_int (DECL_FIELD_OFFSET ((TREE_OPERAND
>> +                           (TREE_OPERAND (op0, 0), 1))), 0))
>
> Can we look at offset/bitpos here rather than examine the tree structure
> of op0?  get_inner_reference already examined it for us.

Good suggestion, thanks!

>
> Also, it looks like you aren't handling the case with the operands
> switched, i.e. 0 == p and such.

Based on my testing and reading the code I believe the caller
(fold_binary_loc) arranges for the constant argument to always
come second in comparisons.  I've added a comment to the code
to make it clear.

Attached is an updated patch retested on x86_64.

Martin