Message ID | 1513528183-16972-1-git-send-email-dmalcolm@redhat.com |
---|---|
State | New |
Headers | show |
Series | None | expand |
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
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)
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
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++;