Re: [v2 of PATCH 13/14] c-format.c: handle location wrappers

[Jason Merrill <jason at redhat dot com>]

On Wed, Dec 20, 2017 at 12:33 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2017-12-19 at 14:55 -0500, Jason Merrill wrote:
>> On 12/17/2017 11:29 AM, David Malcolm wrote:
>> > On Mon, 2017-12-11 at 18:45 -0500, Jason Merrill wrote:
>> > > On 11/10/2017 04:45 PM, David Malcolm wrote:
>> > > > +  STRIP_ANY_LOCATION_WRAPPER (format_tree);
>> > > > +
>> > > >       if (VAR_P (format_tree))
>> > > >         {
>> > > >           /* Pull out a constant value if the front end
>> > > > didn't.  */
>> > >
>> > > It seems like we want fold_for_warn here instead of the special
>> > > variable
>> > > handling.  That probably makes sense for the other places you
>> > > change in
>> > > this patch, too.
>> >
>> > Here's an updated version of the patch which uses fold_for_warn,
>> > rather than STRIP_ANY_LOCATION_WRAPPER.
>> >
>> > In one place it was necessary to add a STRIP_NOPS, since the
>> > fold_for_warn can add a cast around a:
>> >    ADDR_EXPR <POINTER_TYPE(ARRAY_TYPE(char))> (
>> >      STRING_CST)
>> > turning it into a:
>> >    NOP_EXPR<POINTER_TYPE(char))> (
>> >      ADDR_EXPR <POINTER_TYPE(ARRAY_TYPE(char))> (
>> >        STRING_CST))
>>
>> Hmm, that seems like a bug.  fold_for_warn shouldn't change the type of
>> the result.
>
> In a similar vein, is the result of decl_constant_value (decl) required
> to be the same type as that of the decl?
>
> What's happening for this testcase (g++.dg/warn/Wformat-1.C) is that we
> have a VAR_DECL with a DECL_INITIAL, but the types of the two don't
> quite match up.
>
> decl_constant_value returns an unshare_expr clone of the DECL_INITIAL,
> and this is what's returned from fold_for_warn.
>
> Am I right in thinking
>
> (a) that the bug here is that a DECL_INITIAL ought to have the same
>     type as its decl, and so there's a missing cast where that gets
>     set up, or

This seems right.

> (b) should decl_constant_value handle such cases, and introduce a cast
>     if it uncovers them?

decl_constant_value should probably assert that the types match closely enough.

Jason