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

Message ID 1513528183-16972-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • Untitled series #191
Related show

Commit Message

David Malcolm Dec. 17, 2017, 4:29 p.m.
On Mon, 2017-12-11 at 18:45 -0500, Jason Merrill wrote:
> On 11/10/2017 04:45 PM, David Malcolm wrote:

> > gcc/c-family/ChangeLog:

> >        * c-format.c (check_format_arg): Strip any location wrapper

> > around

> >        format_tree.

> > ---

> >    gcc/c-family/c-format.c | 9 ++++++++-

> >    1 file changed, 8 insertions(+), 1 deletion(-)

> >

> > diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c

> > index 164d035..6b436ec 100644

> > --- a/gcc/c-family/c-format.c

> > +++ b/gcc/c-family/c-format.c

> > @@ -1536,6 +1536,8 @@ check_format_arg (void *ctx, tree

> > format_tree,

> >

> >      location_t fmt_param_loc = EXPR_LOC_OR_LOC (format_tree,

> > input_location);

> >

> > +  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.

>

> Jason


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))
which without a STRIP_NOPS leads to a bail-out here:
  1596	  if (TREE_CODE (format_tree) != ADDR_EXPR)
  1597	    {
  1598	      res->number_non_literal++;
  1599	      return;
  1600	    }
and thus -Wformat-security not recognizing string literals.

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

Is this OK for trunk, assuming the rest of the kit is approved?

gcc/c-family/ChangeLog:
	* c-format.c (check_format_arg): Strip any location wrapper around
	format_tree.
---
 gcc/c-family/c-format.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
1.8.5.3

Comments

Jason Merrill Dec. 19, 2017, 7:55 p.m. | #1
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.

> +  format_tree = fold_for_warn (format_tree);

> +

>     if (VAR_P (format_tree))

>       {

>         /* Pull out a constant value if the front end didn't.  */


I was suggesting dropping the if (VAR_P... block, since pulling out the 
constant value should now be covered by fold_for_warn.

> -    format_tree = TREE_OPERAND (format_tree, 0);

> +    {

> +      format_tree = fold_for_warn (TREE_OPERAND (format_tree, 0));

> +    }


Why the added braces?

Jason
David Malcolm Dec. 20, 2017, 5:33 p.m. | #2
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

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

Thanks
Dave



> > +  format_tree = fold_for_warn (format_tree);

> > +

> >     if (VAR_P (format_tree))

> >       {

> >         /* Pull out a constant value if the front end didn't.  */

> 

> I was suggesting dropping the if (VAR_P... block, since pulling out

> the 

> constant value should now be covered by fold_for_warn.

> 

> > -    format_tree = TREE_OPERAND (format_tree, 0);

> > +    {

> > +      format_tree = fold_for_warn (TREE_OPERAND (format_tree, 0));

> > +    }

> 

> Why the added braces?


(I messed up; will be fixed in the next version)
Jason Merrill Dec. 21, 2017, 5 a.m. | #3
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

Patch

diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c
index 164d035..47aca55 100644
--- a/gcc/c-family/c-format.c
+++ b/gcc/c-family/c-format.c
@@ -1536,6 +1536,8 @@  check_format_arg (void *ctx, tree format_tree,
 
   location_t fmt_param_loc = EXPR_LOC_OR_LOC (format_tree, input_location);
 
+  format_tree = fold_for_warn (format_tree);
+
   if (VAR_P (format_tree))
     {
       /* Pull out a constant value if the front end didn't.  */
@@ -1591,13 +1593,14 @@  check_format_arg (void *ctx, tree format_tree,
 	}
       offset = int_cst_value (arg1);
     }
+  STRIP_NOPS (format_tree);
   if (TREE_CODE (format_tree) != ADDR_EXPR)
     {
       res->number_non_literal++;
       return;
     }
   res->format_string_loc = EXPR_LOC_OR_LOC (format_tree, input_location);
-  format_tree = TREE_OPERAND (format_tree, 0);
+  format_tree = fold_for_warn (TREE_OPERAND (format_tree, 0));
   if (format_types[info->format_type].flags 
       & (int) FMT_FLAG_PARSE_ARG_CONVERT_EXTERNAL)
     {
@@ -1634,7 +1637,9 @@  check_format_arg (void *ctx, tree format_tree,
   if (TREE_CODE (format_tree) == ARRAY_REF
       && tree_fits_shwi_p (TREE_OPERAND (format_tree, 1))
       && (offset += tree_to_shwi (TREE_OPERAND (format_tree, 1))) >= 0)
-    format_tree = TREE_OPERAND (format_tree, 0);
+    {
+      format_tree = fold_for_warn (TREE_OPERAND (format_tree, 0));
+    }
   if (offset < 0)
     {
       res->number_non_literal++;