diff mbox series

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

Message ID 1513969858-13170-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series None | expand

Commit Message

David Malcolm Dec. 22, 2017, 7:10 p.m. UTC 
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(-)

Comments

David Malcolm Jan. 5, 2018, 5:35 p.m. UTC | #1 
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);
>  }
Joseph Myers Jan. 5, 2018, 5:48 p.m. UTC | #2 
On Fri, 5 Jan 2018, David Malcolm wrote:

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

OK.
Jason Merrill Jan. 5, 2018, 8:21 p.m. UTC | #3 
On 12/22/2017 02:10 PM, David Malcolm wrote:
> 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.

Right, differences in top-level qualifiers don't matter.  A scalar 
prvalue can't be const.

Jason
diff mbox series

Patch

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);
 }