| Message ID | 20180308180811.GV5867@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] For -Wformat purposes use ellipsis args before class to pointer to class conversion (PR c++/84076) | expand |
On Thu, Mar 8, 2018 at 1:08 PM, Jakub Jelinek <jakub@redhat.com> 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. */
On Thu, Mar 08, 2018 at 04:20:40PM -0500, Jason Merrill wrote: > On Thu, Mar 8, 2018 at 1:08 PM, Jakub Jelinek <jakub@redhat.com> 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<char>&’} [-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<char>’} [-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", &s);// { 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", &t);// { 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", &s);// { 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", &t);// { 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
On Thu, Mar 8, 2018 at 4:26 PM, Jakub Jelinek <jakub@redhat.com> 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 <jakub@redhat.com> 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
On Fri, Mar 09, 2018 at 11:28:47AM -0500, Jason Merrill wrote: > On Thu, Mar 8, 2018 at 4:26 PM, Jakub Jelinek <jakub@redhat.com> 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 <jakub@redhat.com> 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 (&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 <jason@redhat.com> Jakub Jelinek <jakub@redhat.com> 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.jj 2018-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.jj 2018-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", &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\\*'" } +} Jakub
OK. On Fri, Mar 9, 2018 at 12:21 PM, Jakub Jelinek <jakub@redhat.com> 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 <jakub@redhat.com> 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 <jakub@redhat.com> 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 (&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 <jason@redhat.com> > Jakub Jelinek <jakub@redhat.com> > > 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.jj 2018-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.jj 2018-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", &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\\*'" } > +} > > > Jakub
--- gcc/cp/call.c.jj 2018-03-07 22:51:58.698478669 +0100 +++ gcc/cp/call.c 2018-03-08 10:58:58.157373982 +0100 @@ -7967,6 +7967,8 @@ build_over_call (struct z_candidate *can } /* Ellipsis */ + int ellipsis_first = j; + unsigned ellipsis_bias = arg_index - j; int magic = magic_varargs_p (fn); for (; arg_index < vec_safe_length (args); ++arg_index) { @@ -8014,7 +8016,22 @@ 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 the purposes of check_function_arguments, undo the implicit + passing of classes with non-trivial copy ctor or dtor in ellipsis + by invisible reference. */ + if (j >= ellipsis_first && POINTER_TYPE_P (TREE_TYPE (argarray[j]))) + { + tree orig_type = TREE_TYPE ((*args)[ellipsis_bias + j]); + if (type_has_nontrivial_copy_init (orig_type) + || TYPE_HAS_NONTRIVIAL_DESTRUCTOR (orig_type)) + { + fargs[j] = (*args)[ellipsis_bias + j]; + continue; + } + } + 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.jj 2018-03-08 11:05:27.269422301 +0100 +++ gcc/testsuite/g++.dg/warn/Wformat-2.C 2018-03-08 11:04:52.915418031 +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", &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\\*'" } +}