Message ID | 1539037463-61406-1-git-send-email-dmalcolm@redhat.com |
---|---|
State | New |
Headers | show |
Series | Folding and check_function_arguments | expand |
On 10/8/18 6:24 PM, David Malcolm wrote: > On Mon, 2018-10-08 at 10:37 -0400, Jason Merrill wrote: >> On Thu, Oct 4, 2018 at 10:12 AM David Malcolm <dmalcolm@redhat.com> >> wrote: >>> >>> -Wformat in the C++ FE doesn't work as well as it could: >>> (a) it doesn't report precise locations within the string literal, >>> and >>> (b) it doesn't underline arguments for those arguments >>> !CAN_HAVE_LOCATION_P, >>> despite having location wrapper nodes. >>> >>> For example: >>> >>> Wformat-ranges.C:32:10: warning: format '%s' expects argument of >>> type 'char*', but argument 2 has type 'int' [-Wformat=] >>> 32 | printf("hello %s", 42); >>> | ^~~~~~~~~~ >>> >>> (a) is due to not wiring up the langhook for extracting substring >>> locations. >>> >>> This patch uses the one in c-family; it also fixes string >>> literal >>> parsing so that it records string concatenations (needed for >>> extracting substring locations from concatenated strings). >>> >>> (b) is due to the call to maybe_constant_value here: >>> fargs[j] = maybe_constant_value (argarray[j]); >>> within build_over_call. >> >> Maybe we should remove that in favor of fold_for_warn in >> check_function_arguments. >> >> Jason > > This patch eliminates the arglocs array I introduced to build_over_call > in r264887, and eliminates the call to maybe_constant_value when building > "fargs" (thus retaining location wrapper nodes). > > Instead, this patch requires that any checks within > check_function_arguments that need folded arguments do their own folding. > > Of the various checks: > (a) check_function_nonnull already calls fold_for_warn, > (b) check_function_format doesn't need folding > (c) check_function_sentinel needs fold_for_warn in one place, which the > patch adds, and > (d) check_function_restrict needs per-argument folding, which the patch > adds. Given that it scans before and after resetting TREE_VISITED on > each argument, it seemed best to make a copy of the array, folding each > argument from the outset, rather than repeatedly calling fold_for_warn; > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. > > OK for trunk? OK, thanks. Jason
On Thu, 25 Oct 2018, Jason Merrill wrote: > > > > > > Maybe we should remove that in favor of fold_for_warn in > > > check_function_arguments. David, I think your patch also fixes PR 86567. David, Jason, could you comment on doing something similar (using fold_for_warn instead of maybe_constant_value) to solve other issues where generation of new tree uids under maybe_constant_value called in warning context changes code generation, in particular PR 86586? Thanks. Alexander
On Mon, Oct 29, 2018 at 7:07 AM Alexander Monakov <amonakov@ispras.ru> wrote: > On Thu, 25 Oct 2018, Jason Merrill wrote: > > > > > > > > Maybe we should remove that in favor of fold_for_warn in > > > > check_function_arguments. > > David, I think your patch also fixes PR 86567. > > David, Jason, could you comment on doing something similar (using fold_for_warn > instead of maybe_constant_value) to solve other issues where generation of new > tree uids under maybe_constant_value called in warning context changes code > generation, in particular PR 86586? I don't see how it would help; this change does the same folding, just a bit later. Jason Jason
On Mon, 29 Oct 2018, Jason Merrill wrote: > On Mon, Oct 29, 2018 at 7:07 AM Alexander Monakov <amonakov@ispras.ru> wrote: > > On Thu, 25 Oct 2018, Jason Merrill wrote: > > > > > > > > > > Maybe we should remove that in favor of fold_for_warn in > > > > > check_function_arguments. > > > > David, I think your patch also fixes PR 86567. > > > > David, Jason, could you comment on doing something similar (using fold_for_warn > > instead of maybe_constant_value) to solve other issues where generation of new > > tree uids under maybe_constant_value called in warning context changes code > > generation, in particular PR 86586? > > I don't see how it would help; this change does the same folding, just > a bit later. If David's patch causes GCC to perform that folding only after main compilation flow has already instantiated templates, that should help: folding for warning wouldn't try to instantiate anything not already instantiated, and thus wouldn't cause allocation of new uids, I think? (I did check that it helps on the std::vector testcase given in the PR) I see that my question about fold_for_warn doesn't make sense though. Thanks. Alexander
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index c0198e1..11fa360 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -5297,7 +5297,7 @@ check_function_sentinel (const_tree fntype, int nargs, tree *argarray) } /* Validate the sentinel. */ - sentinel = argarray[nargs - 1 - pos]; + sentinel = fold_for_warn (argarray[nargs - 1 - pos]); if ((!POINTER_TYPE_P (TREE_TYPE (sentinel)) || !integer_zerop (sentinel)) /* Although __null (in C++) is only an integer we allow it @@ -5316,11 +5316,16 @@ check_function_sentinel (const_tree fntype, int nargs, tree *argarray) static bool check_function_restrict (const_tree fndecl, const_tree fntype, - int nargs, tree *argarray) + int nargs, tree *unfolded_argarray) { int i; tree parms = TYPE_ARG_TYPES (fntype); + /* Call fold_for_warn on all of the arguments. */ + auto_vec<tree> argarray (nargs); + for (i = 0; i < nargs; i++) + argarray.quick_push (fold_for_warn (unfolded_argarray[i])); + if (fndecl && TREE_CODE (fndecl) == FUNCTION_DECL) { @@ -5357,7 +5362,7 @@ check_function_restrict (const_tree fndecl, const_tree fntype, if (POINTER_TYPE_P (type) && TYPE_RESTRICT (type) && !TYPE_READONLY (TREE_TYPE (type))) - warned |= warn_for_restrict (i, argarray, nargs); + warned |= warn_for_restrict (i, argarray.address (), nargs); } for (i = 0; i < nargs; i++) @@ -5604,7 +5609,11 @@ attribute_fallthrough_p (tree attr) /* Check for valid arguments being passed to a function with FNTYPE. There are NARGS arguments in the array ARGARRAY. LOC should be used for diagnostics. Return true if either -Wnonnull or -Wrestrict has - been issued. */ + been issued. + + The arguments in ARGARRAY may not have been folded yet (e.g. for C++, + to preserve location wrappers); checks that require folded arguments + should call fold_for_warn on them. */ bool check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, diff --git a/gcc/cp/call.c b/gcc/cp/call.c index 747f837..51da771 100644 --- a/gcc/cp/call.c +++ b/gcc/cp/call.c @@ -8188,7 +8188,6 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) { tree *fargs = (!nargs ? argarray : (tree *) alloca (nargs * sizeof (tree))); - auto_vec<location_t> arglocs (nargs); for (j = 0; j < nargs; j++) { /* For -Wformat undo the implicit passing by hidden reference @@ -8197,12 +8196,11 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) && TYPE_REF_P (TREE_TYPE (argarray[j]))) fargs[j] = TREE_OPERAND (argarray[j], 0); else - fargs[j] = maybe_constant_value (argarray[j]); - arglocs.quick_push (EXPR_LOC_OR_LOC (argarray[j], input_location)); + fargs[j] = argarray[j]; } warned_p = check_function_arguments (input_location, fn, TREE_TYPE (fn), - nargs, fargs, &arglocs); + nargs, fargs, NULL); } if (DECL_INHERITED_CTOR (fn))