Re: [PATCH] c++/67376 Comparison with pointer to past-the-end, of array fails inside constant expression

[Jeff Law <law@redhat.com>]

On 03/17/2016 03:16 PM, Martin Sebor wrote:
>>>   static tree cxx_eval_constant_expression (const constexpr_ctx *, tree,
>>> -                      bool, bool *, bool *, tree * = NULL);
>>> +                      bool, bool *, bool *, bool * = NULL,
>>> +                                          tree * = NULL);
>> I didn't look deeply, but do you end up fixing all (most) of the callers
>> of cxx_eval_constant_expression?  If so, then you don't need the default
>> initialization.
>
> Thanks for the comments.  The patch only modifies about 10 out
> of the 70 or so calls to the function in the file and the default
> argument helps avoid making the rest of the changes.  (I have some
> ideas for improving the APIs of these functions that I'd like to
> run by Jason when we're done with the 6.0 work.)
OK.  Then let's keep the default initialization.


>
> The difficulty I've run into with detecting these problems in later
> phases is that some invalid expressions have already been simplified
> by the front end.  The example that applies here (even though this
> is still the front end) is this:
Yea.  I was hoping that the delayed folding work would be helping in 
getting a more faithful representation out of the front-ends.

>
>    constexpr int* p = 0;
>    constexpr bool b0 = &p[0] == 0;   // accepted
>    constexpr bool b1 = &p[1] == 0;   // rejected
>
> Both b0 and b1 are invalid and should be diagnosed, but only b1
> is.  b1 isn't because because by the time we see its initializer
> in constexpr.c it's been transformed into the equivalent of "b1
> = (int*)ps" (though we don't see the cast which would also make
> it invalid).
>
> But if we can avoid these early simplifying transformations and
> retain a more faithful representation of the original source then
> doing the checking later will likely be simpler and result in
> detecting more problems with greater consistency and less effort.
Do we know where the folding is happening for this case and is it 
something we can reasonably defer?    ie, is this just a case we missed 
as part of the deferred folding work and hence should have its own 
distinct BZ to track?

>> Hmm, I thought we already had code to do this somewhere.   It looks like
>> it's moved around quite a bit.  I think you want to be using
>> symtab_node::nonzero_address to determine if a given symbol must bind to
>> a nonzero address.
>
> Thanks for the hint.  I had looked for existing functions but
> couldn't find one that worked.  decl_with_nonnull_addr_p() in
> c-common.c looked promising but it's not accessible here and
> it doesn't do the right thing when HAS_DECL_ASSEMBLER_NAME_P()
> is false (it ICEs).
Yea, I found the same mis-mash of bits that didn't look directly usable 
for the problem you're tackling.  What's odd is I would have sworn that 
we had code to do exactly what you wanted, but I wasn't able to find it, 
either as a distinct routine or open-coded.

>
> In the end, I added a new function, maybe_nonzero_address(),
> that calls symtab_node::nonzero_address(), and that I factored
> out of tree_single_nonzero_warnv_p() that I was then able to
> use in fold_comparison().
Sounds good.

>
> I've made a few other small adjustments to the patch to avoid
> one false positive, and a few test cases, and tweak the expected
> diagnostics now that Marek has fixed 70194.
>
> I've also wrote myself a small sed script to replace blocks of
> 8 spaces with tabs and ran the patch through it.  I'll integrate
> it into my workflow so I hopefully don't have to worry about this
> ever again.
I'll try to take a look at the updated patch shortly.  It may still hit 
too much of the C++ front-end for me to be comfortable reviewing -- 
we'll see.

jeff