Re: [v2 of PATCH 13/14] c-format.c: handle location wrappers
On Wed, Dec 20, 2017 at 12:33 PM, David Malcolm 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 (
>> > STRING_CST)
>> > turning it into a:
>> >NOP_EXPR (
>> > ADDR_EXPR (
>> >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
Re: [v2 of PATCH 13/14] c-format.c: handle location wrappers
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 (
> > STRING_CST)
> > turning it into a:
> >NOP_EXPR (
> > ADDR_EXPR (
> >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)
Re: [v2 of PATCH 13/14] c-format.c: handle location wrappers
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 (
STRING_CST)
turning it into a:
NOP_EXPR (
ADDR_EXPR (
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
[v2 of PATCH 13/14] c-format.c: handle location wrappers
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 (
STRING_CST)
turning it into a:
NOP_EXPR (
ADDR_EXPR (
STRING_CST))
which without a STRIP_NOPS leads to a bail-out here:
1596if (TREE_CODE (format_tree) != ADDR_EXPR)
1597 {
1598res->number_non_literal++;
1599return;
1600 }
and thus -Wformat-security not recognizing string literals.
Successfully bootstrapped®rtested 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(-)
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++;
--
1.8.5.3
