Folding and check_function_arguments

Message ID 1539037463-61406-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • Folding and check_function_arguments
Related show

Commit Message

David Malcolm Oct. 8, 2018, 10:24 p.m.
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?

gcc/c-family/ChangeLog:
	PR c++/56856
	* c-common.c (check_function_sentinel): Call fold_for_warn on the
	argument.
	(check_function_restrict): Rename param "argarray" to
	"unfolded_argarray", and make a copy named "argarray", calling
	fold_for_warn on each argument.
	(check_function_arguments): Add note about responsibility for
	folding the arguments.

gcc/cp/ChangeLog:
	PR c++/56856
	* call.c (build_over_call): Eliminate the "arglocs" array, and the
	call to maybe_constant_value when building "fargs".
---
 gcc/c-family/c-common.c | 17 +++++++++++++----
 gcc/cp/call.c           |  6 ++----
 2 files changed, 15 insertions(+), 8 deletions(-)

Comments

Jason Merrill Oct. 25, 2018, 5:58 p.m. | #1
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
Alexander Monakov Oct. 29, 2018, 11:07 a.m. | #2
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
Jason Merrill Oct. 29, 2018, 5:47 p.m. | #3
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
Alexander Monakov Oct. 29, 2018, 6:45 p.m. | #4
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

Patch

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