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

[David Malcolm]

Ping:
  https://gcc.gnu.org/ml/gcc-patches/2017-12/msg01494.html


(FWIW, the only other patch still needing review in the kit is:
  "[v2 of PATCH 03/14] C++: add location_t wrapper nodes during parsing
(minimal impl)"
     https://gcc.gnu.org/ml/gcc-patches/2017-12/msg01594.html  )

On Fri, 2017-12-22 at 14:10 -0500, David Malcolm wrote:
> 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.co
> > m>
> > 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);
>  }