[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
  • Untitled series #281
Related show

Commit Message

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

-- 
1.8.5.3

Comments

David Malcolm Jan. 5, 2018, 5:35 p.m. | #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. | #2
On Fri, 5 Jan 2018, David Malcolm wrote:

> Ping:

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


OK.

-- 
Joseph S. Myers
joseph@codesourcery.com
Jason Merrill Jan. 5, 2018, 8:21 p.m. | #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

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