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

[David Malcolm <dmalcolm at redhat dot com>]

On Thu, 2017-12-21 at 00:00 -0500, Jason Merrill wrote:
> 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.

Thanks.

You describe the types needing to match "closely enough" (as opposed
to *exactly*), and that may be the key here: what I didn't say in my
message above is that the decl is "const" whereas the value isn't.

To identify where a DECL_INITIAL can have a different type to its decl
(and thus fold_for_warn "changing type"), I tried adding this assertion:

  --- a/gcc/cp/typeck2.c
  +++ b/gcc/cp/typeck2.c
  @@ -857,6 +857,7 @@ store_init_value (tree decl, tree init, vec<tree, va_gc>** cleanups, int flags)
     /* If the value is a constant, just put it in DECL_INITIAL.  If DECL
        is an automatic variable, the middle end will turn this into a
        dynamic initialization later.  */
  +  gcc_assert (TREE_TYPE (value) == TREE_TYPE (decl));
     DECL_INITIAL (decl) = value;
     return NULL_TREE;
   }

The assertion fires when a "const" decl is initialized with a value of
a non-const type; e.g. in g++.dg/warn/Wformat-1.C:

  const char *const msg = "abc";

but I can also reproduce it with:

  const int i = 42;

What happens is that the type of the decl has the "readonly_flag" set,
whereas the type of the value has that flag clear.

For the string case, convert_for_initialization finishes here:

8894	  return convert_for_assignment (type, rhs, errtype, fndecl, parmnum,
8895					 complain, flags);

and convert_for_assignment finishes by calling:

8801	  return perform_implicit_conversion_flags (strip_top_quals (type), rhs,
8802						    complain, flags);

The "strip_top_quals" in that latter call strips away the
"readonly_flag" from type (the decl's type), giving the type of the
rhs, and hence this does nothing.

So we end up with a decl of type
  const char * const
being initialized with a value of type
  const char *
(or a decl of type "const int" being initialized with a value of
type "int").

So the types are slightly inconsistent (const vs non-const), but given
your wording of types needing to match "closely enough" it's not clear
to me that it's a problem - if I'm reading things right, it's coming from
that strip_top_quals in the implicit conversion in convert_for_assignment.

This also happens with a pristine build of trunk.

Rather than attempting to change decl_constant_value or the folding code,
the following patch simply updates the:

  if (VAR_P (format_tree))
    {
      /* Pull out a constant value if the front end didn't.  */
      format_tree = decl_constant_value (format_tree);
      STRIP_NOPS (format_tree);
    }

that you added in r219964 to fix PR c++/64629 to instead be:

  format_tree = fold_for_warn (format_tree);
  STRIP_NOPS (format_tree);

and adds a little test coverage for the pointer addition case.

Successfully bootstrapped&regrtested on x86_64-pc-linux-gnu, as
part of the kit.

OK for trunk once the rest of the kit is approved?


gcc/c-family/ChangeLog:
	* c-format.c (check_format_arg): Convert VAR_P check to a
	fold_for_warn.

gcc/testsuite/ChangeLog:
	* g++.dg/warn/Wformat-1.C: Add tests of pointer arithmetic on
	format strings.
---
 gcc/c-family/c-format.c               | 10 ++++------
 gcc/testsuite/g++.dg/warn/Wformat-1.C |  2 ++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c
index 164d035..1fcac2f 100644
--- a/gcc/c-family/c-format.c
+++ b/gcc/c-family/c-format.c
@@ -1536,12 +1536,10 @@ check_format_arg (void *ctx, tree format_tree,
 
   location_t fmt_param_loc = EXPR_LOC_OR_LOC (format_tree, input_location);
 
-  if (VAR_P (format_tree))
-    {
-      /* Pull out a constant value if the front end didn't.  */
-      format_tree = decl_constant_value (format_tree);
-      STRIP_NOPS (format_tree);
-    }
+  /* Pull out a constant value if the front end didn't, and handle location
+     wrappers.  */
+  format_tree = fold_for_warn (format_tree);
+  STRIP_NOPS (format_tree);
 
   if (integer_zerop (format_tree))
     {
diff --git a/gcc/testsuite/g++.dg/warn/Wformat-1.C b/gcc/testsuite/g++.dg/warn/Wformat-1.C
index 6094a9c..f2e772a 100644
--- a/gcc/testsuite/g++.dg/warn/Wformat-1.C
+++ b/gcc/testsuite/g++.dg/warn/Wformat-1.C
@@ -7,4 +7,6 @@ foo (void)
 {
   const char *const msg = "abc";
   bar (1, msg);
+  bar (1, msg + 1);
+  bar (1, 1 + msg);
 }
-- 
1.8.5.3