Re: [C++ PATCH] For -Wformat purposes use ellipsis args before class to pointer to class conversion (PR c++/84076)

2018-03-09 Thread Jakub Jelinek
On Fri, Mar 09, 2018 at 11:28:47AM -0500, Jason Merrill wrote:
> On Thu, Mar 8, 2018 at 4:26 PM, Jakub Jelinek  wrote:
> > On Thu, Mar 08, 2018 at 04:20:40PM -0500, Jason Merrill wrote:
> >> On Thu, Mar 8, 2018 at 1:08 PM, Jakub Jelinek  wrote:
> >> > The reporter complains that -Wformat incorrectly reports std::string* 
> >> > passed
> >> > to "%s" format rather than std::string, which is what the user did.
> >> >
> >> > This transformation of non-trivial copy init or dtor classes in ellipsis 
> >> > is
> >> > done by convert_arg_to_ellipsis; that function does many changes and all
> >> > but this one look desirable for the 
> >> > -Wnonnull/-Wformat/-Wsentinel/-Wrestrict
> >> > warnings.  We prepare a special argument vector in any case, so this 
> >> > patch
> >> > just arranges to undo what convert_arg_to_ellipsis did for the classes.
> >> > I think -Wnonnull shouldn't care, because when passing such a class by
> >> > value, it will be non-NULL (and -Wnonnull looks only for literal NULLs
> >> > anyway), -Wrestrict only cares about named arguments and -Wsentinel only
> >> > cares about NULL arguments passed to ellipsis.
> >>
> >> I notice that convert_arg_to_ellipsis generates a pointer-typed
> >> expression, whereas convert_for_arg_passing generates a reference.
> >> Does correcting that inconsistency help?
> >
> > No (though I think it is a good idea anyway).
> 
> Perhaps then check_format_types could look through REFERENCE_TYPE,
> since there are no actual expressions of reference type.

It can be done in the caller as well like below, or in c-format.c's
  if (warn_format)
{
  /* FIXME: Rewrite all the internal functions in this file
 to use the ARGARRAY directly instead of constructing this
 temporary list.  */
  tree params = NULL_TREE;
  int i;
  for (i = nargs - 1; i >= 0; i--)
params = tree_cons (NULL_TREE, argarray[i], params);
  check_format_info (, params, arglocs);
}
I'm afraid the c-format.c has so many uses of the params list that it is
hard to tweak it anywhere else.

2018-03-09  Jason Merrill  
Jakub Jelinek  

PR c++/84076
* call.c (convert_arg_to_ellipsis): Instead of cp_build_addr_expr
build ADDR_EXPR with REFERENCE_TYPE.
(build_over_call): For purposes of check_function_arguments, if
argarray[j] is ADDR_EXPR with REFERENCE_TYPE created above, use
its operand rather than the argument itself.

* g++.dg/warn/Wformat-2.C: New test.

--- gcc/cp/call.c.jj2018-03-09 09:01:32.017423737 +0100
+++ gcc/cp/call.c   2018-03-09 18:12:34.973494714 +0100
@@ -7209,7 +7209,7 @@ convert_arg_to_ellipsis (tree arg, tsubs
 "passing objects of non-trivially-copyable "
 "type %q#T through %<...%> is conditionally supported",
 arg_type);
- return cp_build_addr_expr (arg, complain);
+ return build1 (ADDR_EXPR, build_reference_type (arg_type), arg);
}
   /* Build up a real lvalue-to-rvalue conversion in case the
 copy constructor is trivial but not callable.  */
@@ -8018,7 +8018,15 @@ build_over_call (struct z_candidate *can
   tree *fargs = (!nargs ? argarray
: (tree *) alloca (nargs * sizeof (tree)));
   for (j = 0; j < nargs; j++)
-   fargs[j] = maybe_constant_value (argarray[j]);
+   {
+ /* For -Wformat undo the implicit passing by hidden reference
+done by convert_arg_to_ellipsis.  */
+ if (TREE_CODE (argarray[j]) == ADDR_EXPR
+ && TREE_CODE (TREE_TYPE (argarray[j])) == REFERENCE_TYPE)
+   fargs[j] = TREE_OPERAND (argarray[j], 0);
+ else
+   fargs[j] = maybe_constant_value (argarray[j]);
+   }
 
   warned_p = check_function_arguments (input_location, fn, TREE_TYPE (fn),
   nargs, fargs, NULL);
--- gcc/testsuite/g++.dg/warn/Wformat-2.C.jj2018-03-09 17:59:57.098113181 
+0100
+++ gcc/testsuite/g++.dg/warn/Wformat-2.C   2018-03-09 17:59:57.098113181 
+0100
@@ -0,0 +1,17 @@
+// PR c++/84076
+// { dg-do compile }
+// { dg-options "-Wformat" }
+
+struct S { ~S (); };
+struct T { T (); T (const T &); };
+
+void
+foo ()
+{
+  S s;
+  T t;
+  __builtin_printf ("%s\n", s);// { dg-warning "format '%s' expects 
argument of type 'char\\*', but argument 2 has type 'S'" }
+  __builtin_printf ("%s\n", t);// { dg-warning "format '%s' expects 
argument of type 'char\\*', but argument 2 has type 'T'" }
+  __builtin_printf ("%s\n", );// { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'S\\*'" }
+  __builtin_printf ("%s\n", );// { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'T\\*'" }
+}


   

Re: [C++ PATCH] For -Wformat purposes use ellipsis args before class to pointer to class conversion (PR c++/84076)

2018-03-09 Thread Jason Merrill
OK.

On Fri, Mar 9, 2018 at 12:21 PM, Jakub Jelinek  wrote:
> On Fri, Mar 09, 2018 at 11:28:47AM -0500, Jason Merrill wrote:
>> On Thu, Mar 8, 2018 at 4:26 PM, Jakub Jelinek  wrote:
>> > On Thu, Mar 08, 2018 at 04:20:40PM -0500, Jason Merrill wrote:
>> >> On Thu, Mar 8, 2018 at 1:08 PM, Jakub Jelinek  wrote:
>> >> > The reporter complains that -Wformat incorrectly reports std::string* 
>> >> > passed
>> >> > to "%s" format rather than std::string, which is what the user did.
>> >> >
>> >> > This transformation of non-trivial copy init or dtor classes in 
>> >> > ellipsis is
>> >> > done by convert_arg_to_ellipsis; that function does many changes and all
>> >> > but this one look desirable for the 
>> >> > -Wnonnull/-Wformat/-Wsentinel/-Wrestrict
>> >> > warnings.  We prepare a special argument vector in any case, so this 
>> >> > patch
>> >> > just arranges to undo what convert_arg_to_ellipsis did for the classes.
>> >> > I think -Wnonnull shouldn't care, because when passing such a class by
>> >> > value, it will be non-NULL (and -Wnonnull looks only for literal NULLs
>> >> > anyway), -Wrestrict only cares about named arguments and -Wsentinel only
>> >> > cares about NULL arguments passed to ellipsis.
>> >>
>> >> I notice that convert_arg_to_ellipsis generates a pointer-typed
>> >> expression, whereas convert_for_arg_passing generates a reference.
>> >> Does correcting that inconsistency help?
>> >
>> > No (though I think it is a good idea anyway).
>>
>> Perhaps then check_format_types could look through REFERENCE_TYPE,
>> since there are no actual expressions of reference type.
>
> It can be done in the caller as well like below, or in c-format.c's
>   if (warn_format)
> {
>   /* FIXME: Rewrite all the internal functions in this file
>  to use the ARGARRAY directly instead of constructing this
>  temporary list.  */
>   tree params = NULL_TREE;
>   int i;
>   for (i = nargs - 1; i >= 0; i--)
> params = tree_cons (NULL_TREE, argarray[i], params);
>   check_format_info (, params, arglocs);
> }
> I'm afraid the c-format.c has so many uses of the params list that it is
> hard to tweak it anywhere else.
>
> 2018-03-09  Jason Merrill  
> Jakub Jelinek  
>
> PR c++/84076
> * call.c (convert_arg_to_ellipsis): Instead of cp_build_addr_expr
> build ADDR_EXPR with REFERENCE_TYPE.
> (build_over_call): For purposes of check_function_arguments, if
> argarray[j] is ADDR_EXPR with REFERENCE_TYPE created above, use
> its operand rather than the argument itself.
>
> * g++.dg/warn/Wformat-2.C: New test.
>
> --- gcc/cp/call.c.jj2018-03-09 09:01:32.017423737 +0100
> +++ gcc/cp/call.c   2018-03-09 18:12:34.973494714 +0100
> @@ -7209,7 +7209,7 @@ convert_arg_to_ellipsis (tree arg, tsubs
>  "passing objects of non-trivially-copyable "
>  "type %q#T through %<...%> is conditionally supported",
>  arg_type);
> - return cp_build_addr_expr (arg, complain);
> + return build1 (ADDR_EXPR, build_reference_type (arg_type), arg);
> }
>/* Build up a real lvalue-to-rvalue conversion in case the
>  copy constructor is trivial but not callable.  */
> @@ -8018,7 +8018,15 @@ build_over_call (struct z_candidate *can
>tree *fargs = (!nargs ? argarray
> : (tree *) alloca (nargs * sizeof (tree)));
>for (j = 0; j < nargs; j++)
> -   fargs[j] = maybe_constant_value (argarray[j]);
> +   {
> + /* For -Wformat undo the implicit passing by hidden reference
> +done by convert_arg_to_ellipsis.  */
> + if (TREE_CODE (argarray[j]) == ADDR_EXPR
> + && TREE_CODE (TREE_TYPE (argarray[j])) == REFERENCE_TYPE)
> +   fargs[j] = TREE_OPERAND (argarray[j], 0);
> + else
> +   fargs[j] = maybe_constant_value (argarray[j]);
> +   }
>
>warned_p = check_function_arguments (input_location, fn, TREE_TYPE 
> (fn),
>nargs, fargs, NULL);
> --- gcc/testsuite/g++.dg/warn/Wformat-2.C.jj2018-03-09 17:59:57.098113181 
> +0100
> +++ gcc/testsuite/g++.dg/warn/Wformat-2.C   2018-03-09 17:59:57.098113181 
> +0100
> @@ -0,0 +1,17 @@
> +// PR c++/84076
> +// { dg-do compile }
> +// { dg-options "-Wformat" }
> +
> +struct S { ~S (); };
> +struct T { T (); T (const T &); };
> +
> +void
> +foo ()
> +{
> +  S s;
> +  T t;
> +  __builtin_printf ("%s\n", s);// { dg-warning "format '%s' expects 
> argument of type 'char\\*', but argument 2 has type 'S'" }
> +  __builtin_printf ("%s\n", t);// { dg-warning "format '%s' expects 
> argument of type 'char\\*', but argument 2 has type 'T'" }
> +  

Re: [C++ PATCH] For -Wformat purposes use ellipsis args before class to pointer to class conversion (PR c++/84076)

2018-03-09 Thread Jason Merrill
On Thu, Mar 8, 2018 at 4:26 PM, Jakub Jelinek  wrote:
> On Thu, Mar 08, 2018 at 04:20:40PM -0500, Jason Merrill wrote:
>> On Thu, Mar 8, 2018 at 1:08 PM, Jakub Jelinek  wrote:
>> > The reporter complains that -Wformat incorrectly reports std::string* 
>> > passed
>> > to "%s" format rather than std::string, which is what the user did.
>> >
>> > This transformation of non-trivial copy init or dtor classes in ellipsis is
>> > done by convert_arg_to_ellipsis; that function does many changes and all
>> > but this one look desirable for the 
>> > -Wnonnull/-Wformat/-Wsentinel/-Wrestrict
>> > warnings.  We prepare a special argument vector in any case, so this patch
>> > just arranges to undo what convert_arg_to_ellipsis did for the classes.
>> > I think -Wnonnull shouldn't care, because when passing such a class by
>> > value, it will be non-NULL (and -Wnonnull looks only for literal NULLs
>> > anyway), -Wrestrict only cares about named arguments and -Wsentinel only
>> > cares about NULL arguments passed to ellipsis.
>>
>> I notice that convert_arg_to_ellipsis generates a pointer-typed
>> expression, whereas convert_for_arg_passing generates a reference.
>> Does correcting that inconsistency help?
>
> No (though I think it is a good idea anyway).

Perhaps then check_format_types could look through REFERENCE_TYPE,
since there are no actual expressions of reference type.

Jason


Re: [C++ PATCH] For -Wformat purposes use ellipsis args before class to pointer to class conversion (PR c++/84076)

2018-03-08 Thread Jakub Jelinek
On Thu, Mar 08, 2018 at 04:20:40PM -0500, Jason Merrill wrote:
> On Thu, Mar 8, 2018 at 1:08 PM, Jakub Jelinek  wrote:
> > The reporter complains that -Wformat incorrectly reports std::string* passed
> > to "%s" format rather than std::string, which is what the user did.
> >
> > This transformation of non-trivial copy init or dtor classes in ellipsis is
> > done by convert_arg_to_ellipsis; that function does many changes and all
> > but this one look desirable for the -Wnonnull/-Wformat/-Wsentinel/-Wrestrict
> > warnings.  We prepare a special argument vector in any case, so this patch
> > just arranges to undo what convert_arg_to_ellipsis did for the classes.
> > I think -Wnonnull shouldn't care, because when passing such a class by
> > value, it will be non-NULL (and -Wnonnull looks only for literal NULLs
> > anyway), -Wrestrict only cares about named arguments and -Wsentinel only
> > cares about NULL arguments passed to ellipsis.
> 
> I notice that convert_arg_to_ellipsis generates a pointer-typed
> expression, whereas convert_for_arg_passing generates a reference.
> Does correcting that inconsistency help?

No (though I think it is a good idea anyway).
With this patch the original testcase reports:
pr84076.C:7:16: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘std::__cxx11::string&’ {aka 
‘std::__cxx11::basic_string&’} [-Wformat=]
where the user expects that without the & characters and without the patch
it has * instead of &.
With the patch I've posted it is:
pr84076.C:7:16: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘std::__cxx11::string’ {aka 
‘std::__cxx11::basic_string’} [-Wformat=]

Similarly, the testcase included in the patch with just your patch:
pr84076.C: In function ‘void foo()’:
pr84076.C:13:21: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘S&’ [-Wformat=]
   __builtin_printf ("%s\n", s); // { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'S'" }
 ^~
pr84076.C:14:21: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘T&’ [-Wformat=]
   __builtin_printf ("%s\n", t); // { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'T'" }
 ^~
pr84076.C:15:21: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘S*’ [-Wformat=]
   __builtin_printf ("%s\n", );// { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'S\\*'" }
 ^~  ~~
pr84076.C:16:21: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘T*’ [-Wformat=]
   __builtin_printf ("%s\n", );// { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'T\\*'" }
 ^~  ~~
and with both patches:
pr84076.C: In function ‘void foo()’:
pr84076.C:13:21: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘S’ [-Wformat=]
   __builtin_printf ("%s\n", s); // { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'S'" }
 ^~  ~
pr84076.C:14:21: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘T’ [-Wformat=]
   __builtin_printf ("%s\n", t); // { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'T'" }
 ^~  ~
pr84076.C:15:21: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘S*’ [-Wformat=]
   __builtin_printf ("%s\n", );// { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'S\\*'" }
 ^~  ~~
pr84076.C:16:21: warning: format ‘%s’ expects argument of type ‘char*’, but 
argument 2 has type ‘T*’ [-Wformat=]
   __builtin_printf ("%s\n", );// { dg-warning "format '%s' expects argument 
of type 'char\\*', but argument 2 has type 'T\\*'" }
 ^~  ~~

> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index f83d51f3457..617ce18f8cd 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -7209,7 +7209,7 @@ convert_arg_to_ellipsis (tree arg, tsubst_flags_t 
> complain)
>"passing objects of non-trivially-copyable "
>"type %q#T through %<...%> is conditionally supported",
>arg_type);
> -   return cp_build_addr_expr (arg, complain);
> +   return build1 (ADDR_EXPR, build_reference_type (arg_type), arg);
>   }
>/* Build up a real lvalue-to-rvalue conversion in case the
>copy constructor is trivial but not callable.  */


Jakub


Re: [C++ PATCH] For -Wformat purposes use ellipsis args before class to pointer to class conversion (PR c++/84076)

2018-03-08 Thread Jason Merrill
On Thu, Mar 8, 2018 at 1:08 PM, Jakub Jelinek  wrote:
> The reporter complains that -Wformat incorrectly reports std::string* passed
> to "%s" format rather than std::string, which is what the user did.
>
> This transformation of non-trivial copy init or dtor classes in ellipsis is
> done by convert_arg_to_ellipsis; that function does many changes and all
> but this one look desirable for the -Wnonnull/-Wformat/-Wsentinel/-Wrestrict
> warnings.  We prepare a special argument vector in any case, so this patch
> just arranges to undo what convert_arg_to_ellipsis did for the classes.
> I think -Wnonnull shouldn't care, because when passing such a class by
> value, it will be non-NULL (and -Wnonnull looks only for literal NULLs
> anyway), -Wrestrict only cares about named arguments and -Wsentinel only
> cares about NULL arguments passed to ellipsis.

I notice that convert_arg_to_ellipsis generates a pointer-typed
expression, whereas convert_for_arg_passing generates a reference.
Does correcting that inconsistency help?
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f83d51f3457..617ce18f8cd 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -7209,7 +7209,7 @@ convert_arg_to_ellipsis (tree arg, tsubst_flags_t complain)
 		 "passing objects of non-trivially-copyable "
 		 "type %q#T through %<...%> is conditionally supported",
 		 arg_type);
-	  return cp_build_addr_expr (arg, complain);
+	  return build1 (ADDR_EXPR, build_reference_type (arg_type), arg);
 	}
   /* Build up a real lvalue-to-rvalue conversion in case the
 	 copy constructor is trivial but not callable.  */