| Message ID | YldZ+ba1M5GEyggl@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] c, c++: attribute format on a ctor with a vbase [PR101833, PR47634] | expand |
On 4/13/22 19:17, Marek Polacek wrote: > On Tue, Apr 12, 2022 at 04:01:14PM -0400, Jason Merrill wrote: >> On 4/12/22 14:38, Marek Polacek wrote: >>> On Mon, Apr 11, 2022 at 04:39:22PM -0400, Jason Merrill wrote: >>>> On 4/8/22 15:21, Marek Polacek wrote: >>>>> On Wed, Apr 06, 2022 at 04:55:54PM -0400, Jason Merrill wrote: >>>>>> On 4/1/22 15:14, Marek Polacek wrote: >>>>>>> Attribute format takes three arguments: archetype, string-index, and >>>>>>> first-to-check. The last two specify the position in the function >>>>>>> parameter list. r63030 clarified that "Since non-static C++ methods have >>>>>>> an implicit this argument, the arguments of such methods should be counted >>>>>>> from two, not one, when giving values for string-index and first-to-check." >>>>>>> Therefore one has to write >>>>>>> >>>>>>> struct D { >>>>>>> D(const char *, ...) __attribute__((format(printf, 2, 3))); >>>>>>> }; >>>>>>> >>>>>>> However -- and this is the problem in this PR -- ctors with virtual >>>>>>> bases also get two additional parameters: the in-charge parameter and >>>>>>> the VTT parameter (added in maybe_retrofit_in_chrg). In fact we'll end up >>>>>>> with two clones of the ctor: an in-charge and a not-in-charge version (see >>>>>>> build_cdtor_clones). That means that the argument position the user >>>>>>> specified in the attribute argument will refer to different arguments, >>>>>>> depending on which constructor we're currently dealing with. This can >>>>>>> cause a range of problems: wrong errors, confusing warnings, or crashes. >>>>>>> >>>>>>> This patch corrects that; for C we don't have to do anything, and in C++ >>>>>>> we can use num_artificial_parms_for. It would be wrong to rewrite the >>>>>>> attributes the user supplied, so I've added an extra parameter called >>>>>>> adjust_pos. >>>>>>> >>>>>>> Attribute format_arg is not affected, because it requires that the >>>>>>> function returns "const char *" which will never be the case for cdtors. >>>>>>> >>>>>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? >>>>>>> >>>>>>> PR c++/101833 >>>>>>> PR c++/47634 >>>>>>> >>>>>>> gcc/c-family/ChangeLog: >>>>>>> >>>>>>> * c-attribs.cc (positional_argument): Add new argument adjust_pos, >>>>>>> use it. >>>>>>> * c-common.cc (check_function_arguments): Pass fndecl to >>>>>>> check_function_format. >>>>>>> * c-common.h (check_function_format): Adjust declaration. >>>>>>> (maybe_adjust_arg_pos_for_attribute): Add. >>>>>>> (positional_argument): Adjust declaration. >>>>>>> * c-format.cc (decode_format_attr): Add fndecl argument. Pass it to >>>>>>> maybe_adjust_arg_pos_for_attribute. Adjust calls to get_constant. >>>>>> >>>>>> I wonder about, instead of adding another parameter, allowing the current >>>>>> fntype parameter to be the fndecl when we have one. >>>>>> >>>>>> And then that gets passed down into positional_argument, so we can call >>>>>> maybe_adjust_arg_pos_for_attribute there, and adjust the return value >>>>>> appropriately so we don't need the extra adjustment in get_constant? >>>>> >>>>> Unfortunately I can't do that. positional_argument can't return the >>>>> adjusted position, because get_constant returns it and in decode_format_attr >>>>> it's used to rewrite the arguments in the attribute list: >>>>> >>>>> tree *format_num_expr = &TREE_VALUE (TREE_CHAIN (args)); >>>>> tree *first_arg_num_expr = &TREE_VALUE (TREE_CHAIN (TREE_CHAIN (args))); >>>>> ... >>>>> if (tree val = get_constant (fntype, atname, *format_num_expr, >>>>> 2, &info->format_num, 0, validated_p, >>>>> adjust_pos)) >>>>> *format_num_expr = val; >>>> >>>> Could we not do that? Currently isn't it just overwriting the value with >>>> the same value after default_conversion? >>> >>> I think it is. >>> >>>> Maybe do that conversion directly in decode_format_attr instead? >>> >>> I'm afraid I can't move the default_conversion call out of positional_argument >>> because positional_argument is called from a lot of handle_*_attribute >>> functions, and each of those would have to call default_conversion, which >>> we don't want to do. (Failure to call default_conversion would break e.g. >>> g++.dg/cpp0x/constexpr-attribute2.C.) >> >> Maybe pass in the pointer where we want to store the converted value? > > ... or use pass-by-reference so that I don't have to adjust the call sites > of positional_argument, since we never pass a NULL_TREE. That makes the > patch a bit smaller. > > Now maybe_adjust_arg_pos_for_attribute is only called in positional_argument > and the adjustment is done only there. It's still somewhat messier than I > hoped so I'm happy to defer to GCC 13. Thanks, > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/11.3? > > -- >8 -- > Attribute format takes three arguments: archetype, string-index, and > first-to-check. The last two specify the position in the function > parameter list. r63030 clarified that "Since non-static C++ methods have > an implicit this argument, the arguments of such methods should be counted > from two, not one, when giving values for string-index and first-to-check." > Therefore one has to write > > struct D { > D(const char *, ...) __attribute__((format(printf, 2, 3))); > }; > > However -- and this is the problem in this PR -- ctors with virtual > bases also get two additional parameters: the in-charge parameter and > the VTT parameter (added in maybe_retrofit_in_chrg). In fact we'll end up > with two clones of the ctor: an in-charge and a not-in-charge version (see > build_cdtor_clones). That means that the argument position the user > specified in the attribute argument will refer to different arguments, > depending on which constructor we're currently dealing with. This can > cause a range of problems: wrong errors, confusing warnings, or crashes. > > This patch corrects that; for C we don't have to do anything, and in C++ > we can use num_artificial_parms_for. It would be wrong to rewrite the > attributes the user supplied, so I've changed POS to be passed by > reference so that we don't have to change all the call sites of > positional_argument and we still get the default_conversion adjustment. > > Attribute format_arg is not affected, because it requires that the > function returns "const char *" which will never be the case for cdtors. > > PR c++/101833 > PR c++/47634 > > gcc/c-family/ChangeLog: > > * c-attribs.cc (positional_argument): Pass POS by reference. Deal > with FN being either a function declaration or function type. Use > maybe_adjust_arg_pos_for_attribute. > * c-common.cc (check_function_arguments): Maybe pass FNDECL down to > check_function_format. > * c-common.h (maybe_adjust_arg_pos_for_attribute): Declare. > (positional_argument): Adjust. > * c-format.cc (get_constant): Take EXPR by reference. Return bool > instead of tree. > (handle_format_arg_attribute): Don't overwrite FORMAT_NUM_EXPR by the > return value of get_constant. > (decode_format_attr): Don't overwrite FORMAT_NUM_EXPR and > FIRST_ARG_NUM_EXPR by the return value of get_constant. > (check_function_format): Adjust a parameter name. > (handle_format_attribute): Maybe pass FNDECL down to decode_format_attr. > > gcc/c/ChangeLog: > > * c-objc-common.cc (maybe_adjust_arg_pos_for_attribute): New. > > gcc/cp/ChangeLog: > > * tree.cc (maybe_adjust_arg_pos_for_attribute): New. > > gcc/testsuite/ChangeLog: > > * g++.dg/ext/attr-format-arg1.C: New test. > * g++.dg/ext/attr-format1.C: New test. > * g++.dg/ext/attr-format2.C: New test. > * g++.dg/ext/attr-format3.C: New test. > --- > gcc/c-family/c-attribs.cc | 33 ++++++---- > gcc/c-family/c-common.cc | 4 +- > gcc/c-family/c-common.h | 3 +- > gcc/c-family/c-format.cc | 69 ++++++++++----------- > gcc/c/c-objc-common.cc | 9 +++ > gcc/cp/tree.cc | 19 ++++++ > gcc/testsuite/g++.dg/ext/attr-format-arg1.C | 26 ++++++++ > gcc/testsuite/g++.dg/ext/attr-format1.C | 32 ++++++++++ > gcc/testsuite/g++.dg/ext/attr-format2.C | 38 ++++++++++++ > gcc/testsuite/g++.dg/ext/attr-format3.C | 15 +++++ > 10 files changed, 198 insertions(+), 50 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ext/attr-format-arg1.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-format1.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-format2.C > create mode 100644 gcc/testsuite/g++.dg/ext/attr-format3.C > > diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc > index 111a33f405a..bcc316fa259 100644 > --- a/gcc/c-family/c-attribs.cc > +++ b/gcc/c-family/c-attribs.cc > @@ -594,18 +594,23 @@ attribute_takes_identifier_p (const_tree attr_id) > } > > /* Verify that argument value POS at position ARGNO to attribute NAME > - applied to function TYPE refers to a function parameter at position > - POS and the expected type CODE. Treat CODE == INTEGER_TYPE as > - matching all C integral types except bool. If successful, return > - POS after default conversions, if any. Otherwise, issue appropriate > - warnings and return null. A non-zero 1-based ARGNO should be passed > - in by callers only for attributes with more than one argument. */ > + applied to function FN (which is either a function declaration or function > + type) refers to a function parameter at position POS and the expected type > + CODE. Treat CODE == INTEGER_TYPE as matching all C integral types except > + bool. If successful, return POS after default conversions (and possibly > + adjusted by ADJUST_POS). Otherwise, issue appropriate warnings and return > + null. A non-zero 1-based ARGNO should be passed in by callers only for > + attributes with more than one argument. > + > + N.B. This function modifies POS. */ > > tree > -positional_argument (const_tree fntype, const_tree atname, tree pos, > +positional_argument (const_tree fn, const_tree atname, tree &pos, > tree_code code, int argno /* = 0 */, > int flags /* = posargflags () */) > { > + const_tree fndecl = TYPE_P (fn) ? NULL_TREE : fn; > + const_tree fntype = TYPE_P (fn) ? fn : TREE_TYPE (fn); > if (pos && TREE_CODE (pos) != IDENTIFIER_NODE > && TREE_CODE (pos) != FUNCTION_DECL) > pos = default_conversion (pos); > @@ -682,6 +687,11 @@ positional_argument (const_tree fntype, const_tree atname, tree pos, > if (!prototype_p (fntype)) > return pos; > > + /* ADJUST_POS is non-zero in C++ when the function type has invisible > + parameters generated by the compiler, such as the in-charge or VTT > + parameters. */ > + const int adjust_pos = maybe_adjust_arg_pos_for_attribute (fndecl); > + > /* Verify that the argument position does not exceed the number > of formal arguments to the function. When POSARG_ELLIPSIS > is set, ARGNO may be beyond the last argument of a vararg > @@ -690,7 +700,7 @@ positional_argument (const_tree fntype, const_tree atname, tree pos, > if (!nargs > || !tree_fits_uhwi_p (pos) > || ((flags & POSARG_ELLIPSIS) == 0 > - && !IN_RANGE (tree_to_uhwi (pos), 1, nargs))) > + && !IN_RANGE (tree_to_uhwi (pos) + adjust_pos, 1, nargs))) > { > > if (argno < 1) > @@ -707,8 +717,9 @@ positional_argument (const_tree fntype, const_tree atname, tree pos, > } > > /* Verify that the type of the referenced formal argument matches > - the expected type. */ > - unsigned HOST_WIDE_INT ipos = tree_to_uhwi (pos); > + the expected type. Invisible parameters may have been added by > + the compiler, so adjust the position accordingly. */ > + unsigned HOST_WIDE_INT ipos = tree_to_uhwi (pos) + adjust_pos; > > /* Zero was handled above. */ > gcc_assert (ipos != 0); > @@ -791,7 +802,7 @@ positional_argument (const_tree fntype, const_tree atname, tree pos, > return NULL_TREE; > } > > - return pos; > + return build_int_cst (TREE_TYPE (pos), ipos); > } > > /* Return the first of DECL or TYPE attributes installed in NODE if it's > diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc > index 70f55f3a346..9fb664c9872 100644 > --- a/gcc/c-family/c-common.cc > +++ b/gcc/c-family/c-common.cc > @@ -6071,8 +6071,8 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, > /* Check for errors in format strings. */ > > if (warn_format || warn_suggest_attribute_format) > - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, > - arglocs); > + check_function_format (fndecl ? fndecl : fntype, TYPE_ATTRIBUTES (fntype), nargs, > + argarray, arglocs); > > if (warn_format) > check_function_sentinel (fntype, nargs, argarray); > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h > index 52a85bfb783..f10be9bd67e 100644 > --- a/gcc/c-family/c-common.h > +++ b/gcc/c-family/c-common.h > @@ -1049,6 +1049,7 @@ extern tree finish_label_address_expr (tree, location_t); > extern tree lookup_label (tree); > extern tree lookup_name (tree); > extern bool lvalue_p (const_tree); > +extern int maybe_adjust_arg_pos_for_attribute (const_tree); > > extern bool vector_targets_convertible_p (const_tree t1, const_tree t2); > extern bool vector_types_convertible_p (const_tree t1, const_tree t2, bool emit_lax_note); > @@ -1493,7 +1494,7 @@ enum posargflags { > POSARG_ELLIPSIS = 2 > }; > > -extern tree positional_argument (const_tree, const_tree, tree, tree_code, > +extern tree positional_argument (const_tree, const_tree, tree &, tree_code, > int = 0, int = posargflags ()); > > extern enum flt_eval_method > diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc > index 98f28c0dcc6..393dda462a4 100644 > --- a/gcc/c-family/c-format.cc > +++ b/gcc/c-family/c-format.cc > @@ -78,7 +78,7 @@ static bool check_format_string (const_tree argument, > unsigned HOST_WIDE_INT format_num, > int flags, bool *no_add_attrs, > int expected_format_type); > -static tree get_constant (const_tree fntype, const_tree atname, tree expr, > +static bool get_constant (const_tree fn, const_tree atname, tree &expr, > int argno, unsigned HOST_WIDE_INT *value, > int flags, bool validated_p); > static const char *convert_format_name_to_system_name (const char *attr_name); > @@ -172,14 +172,11 @@ handle_format_arg_attribute (tree *node, tree atname, > tree args, int flags, bool *no_add_attrs) > { > tree type = *node; > - /* Note that TREE_VALUE (args) is changed in place below. */ > + /* Note that TREE_VALUE (args) is changed in the get_constant call. */ > tree *format_num_expr = &TREE_VALUE (args); > unsigned HOST_WIDE_INT format_num = 0; > > - if (tree val = get_constant (type, atname, *format_num_expr, 0, &format_num, > - 0, false)) > - *format_num_expr = val; > - else > + if (!get_constant (type, atname, *format_num_expr, 0, &format_num, 0, false)) > { > *no_add_attrs = true; > return NULL_TREE; > @@ -301,38 +298,39 @@ check_format_string (const_tree fntype, unsigned HOST_WIDE_INT format_num, > /* Under the control of FLAGS, verify EXPR is a valid constant that > refers to a positional argument ARGNO having a string type (char* > or, for targets like Darwin, a pointer to struct CFString) to > - a function type FNTYPE declared with attribute ATNAME. > - If valid, store the constant's integer value in *VALUE and return > - the value. > - If VALIDATED_P is true assert the validation is successful. > - Returns the converted constant value on success, null otherwise. */ > + a function FN declared with attribute ATNAME. If valid, store the > + constant's integer value in *VALUE and return true. If VALIDATED_P > + is true assert the validation is successful. > > -static tree > -get_constant (const_tree fntype, const_tree atname, tree expr, int argno, > + N.B. This function modifies EXPR. */ > + > +static bool > +get_constant (const_tree fn, const_tree atname, tree &expr, int argno, I think this function could use a new name to reflect its new behavior; maybe validate_constant? > unsigned HOST_WIDE_INT *value, int flags, bool validated_p) > { > /* Require the referenced argument to have a string type. For targets > like Darwin, also accept pointers to struct CFString. */ > - if (tree val = positional_argument (fntype, atname, expr, STRING_CST, > + if (tree val = positional_argument (fn, atname, expr, STRING_CST, > argno, flags)) > { > *value = TREE_INT_CST_LOW (val); > - return val; > + return true; > } > > gcc_assert (!validated_p); > - return NULL_TREE; > + return false; > } > > /* Decode the arguments to a "format" attribute into a > function_format_info structure. It is already known that the list > is of the right length. If VALIDATED_P is true, then these > attributes have already been validated and must not be erroneous; > - if false, it will give an error message. Returns true if the > - attributes are successfully decoded, false otherwise. */ > + if false, it will give an error message. FN is either a function > + declaration or function type. Returns true if the attributes are > + successfully decoded, false otherwise. */ > > static bool > -decode_format_attr (const_tree fntype, tree atname, tree args, > +decode_format_attr (const_tree fn, tree atname, tree args, > function_format_info *info, bool validated_p) > { > tree format_type_id = TREE_VALUE (args); > @@ -372,17 +370,12 @@ decode_format_attr (const_tree fntype, tree atname, tree args, > } > } > > - if (tree val = get_constant (fntype, atname, *format_num_expr, > - 2, &info->format_num, 0, validated_p)) > - *format_num_expr = val; > - else > + if (!get_constant (fn, atname, *format_num_expr, 2, &info->format_num, > + 0, validated_p)) > return false; > > - if (tree val = get_constant (fntype, atname, *first_arg_num_expr, > - 3, &info->first_arg_num, > - (POSARG_ZERO | POSARG_ELLIPSIS), validated_p)) > - *first_arg_num_expr = val; > - else > + if (!get_constant (fn, atname, *first_arg_num_expr, 3, &info->first_arg_num, > + (POSARG_ZERO | POSARG_ELLIPSIS), validated_p)) > return false; > > if (info->first_arg_num != 0 && info->first_arg_num <= info->format_num) > @@ -1154,13 +1147,12 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) > > /* Check the argument list of a call to printf, scanf, etc. > ATTRS are the attributes on the function type. There are NARGS argument > - values in the array ARGARRAY. > - Also, if -Wsuggest-attribute=format, > - warn for calls to vprintf or vscanf in functions with no such format > - attribute themselves. */ > + values in the array ARGARRAY. FN is either a function declaration or > + function type. Also, if -Wsuggest-attribute=format, warn for calls to > + vprintf or vscanf in functions with no such format attribute themselves. */ > > void > -check_function_format (const_tree fntype, tree attrs, int nargs, > +check_function_format (const_tree fn, tree attrs, int nargs, > tree *argarray, vec<location_t> *arglocs) > { > tree a; > @@ -1174,7 +1166,7 @@ check_function_format (const_tree fntype, tree attrs, int nargs, > { > /* Yup; check it. */ > function_format_info info; > - decode_format_attr (fntype, atname, TREE_VALUE (a), &info, > + decode_format_attr (fn, atname, TREE_VALUE (a), &info, > /*validated=*/true); > if (warn_format) > { > @@ -5150,10 +5142,14 @@ convert_format_name_to_system_name (const char *attr_name) > /* Handle a "format" attribute; arguments as in > struct attribute_spec.handler. */ > tree > -handle_format_attribute (tree *node, tree atname, tree args, > +handle_format_attribute (tree node[3], tree atname, tree args, > int flags, bool *no_add_attrs) > { > const_tree type = *node; > + /* NODE[2] may be NULL, and it also may be a PARM_DECL for function > + pointers. */ > + const_tree fndecl = ((node[2] && TREE_CODE (node[2]) == FUNCTION_DECL) > + ? node[2] : NULL_TREE); I notice that the comment for the handler member of attribute_spec could use an update to note that the node argument now points to an array of 3. With those changes, OK for GCC 13. > function_format_info info; > > #ifdef TARGET_FORMAT_TYPES > @@ -5179,7 +5175,8 @@ handle_format_attribute (tree *node, tree atname, tree args, > if (TREE_CODE (TREE_VALUE (args)) == IDENTIFIER_NODE) > TREE_VALUE (args) = canonicalize_attr_name (TREE_VALUE (args)); > > - if (!decode_format_attr (type, atname, args, &info, /* validated_p = */false)) > + if (!decode_format_attr (fndecl ? fndecl : type, atname, args, &info, > + /* validated_p = */false)) > { > *no_add_attrs = true; > return NULL_TREE; > diff --git a/gcc/c/c-objc-common.cc b/gcc/c/c-objc-common.cc > index 97850ada2c8..70e10a98e33 100644 > --- a/gcc/c/c-objc-common.cc > +++ b/gcc/c/c-objc-common.cc > @@ -394,3 +394,12 @@ c_get_alias_set (tree t) > > return c_common_get_alias_set (t); > } > + > +/* In C there are no invisible parameters like in C++ (this, in-charge, VTT, > + etc.). */ > + > +int > +maybe_adjust_arg_pos_for_attribute (const_tree) > +{ > + return 0; > +} > diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc > index 63164bee638..6c77b9c19b5 100644 > --- a/gcc/cp/tree.cc > +++ b/gcc/cp/tree.cc > @@ -6118,6 +6118,25 @@ maybe_warn_zero_as_null_pointer_constant (tree expr, location_t loc) > } > return false; > } > + > +/* FNDECL is a function declaration whose type may have been altered by > + adding extra parameters such as this, in-charge, or VTT. When this > + takes place, the positional arguments supplied by the user (as in the > + 'format' attribute arguments) may refer to the wrong argument. This > + function returns an integer indicating how many arguments should be > + skipped. */ > + > +int > +maybe_adjust_arg_pos_for_attribute (const_tree fndecl) > +{ > + if (!fndecl) > + return 0; > + int n = num_artificial_parms_for (fndecl); > + /* The manual states that it's the user's responsibility to account > + for the implicit this parameter. */ > + return n > 0 ? n - 1 : 0; > +} > + > > /* Release memory we no longer need after parsing. */ > void > diff --git a/gcc/testsuite/g++.dg/ext/attr-format-arg1.C b/gcc/testsuite/g++.dg/ext/attr-format-arg1.C > new file mode 100644 > index 00000000000..a7ad0f9ca33 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ext/attr-format-arg1.C > @@ -0,0 +1,26 @@ > +// PR c++/101833 > +// { dg-do compile } > +// { dg-options "-Wall" } > + > +struct B { }; > + > +struct V : virtual B { > + const char *fmt (int, const char *) __attribute__((format_arg(3))); > +}; > + > +struct D : B { > + const char *fmt (int, const char *) __attribute__((format_arg(3))); > +}; > + > +extern void fmt (const char *, ...) __attribute__((format(printf, 1, 2))); > + > +void > +g () > +{ > + V v; > + fmt (v.fmt (1, "%d"), 1); > + fmt (v.fmt (1, "%d"), 1lu); // { dg-warning "expects argument of type" } > + D d; > + fmt (d.fmt (1, "%d"), 1); > + fmt (d.fmt (1, "%d"), 1lu); // { dg-warning "expects argument of type" } > +} > diff --git a/gcc/testsuite/g++.dg/ext/attr-format1.C b/gcc/testsuite/g++.dg/ext/attr-format1.C > new file mode 100644 > index 00000000000..1b8464ed6ac > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ext/attr-format1.C > @@ -0,0 +1,32 @@ > +// PR c++/47634 > +// { dg-do compile } > + > +class Base { > +public: > + Base() { } > +}; > + > +class VDerived : public virtual Base { > +public: > + VDerived(int x, const char * f, ...) __attribute__((format(printf, 3, 4))); > +}; > + > +class Derived : public Base { > +public: > + Derived(int x, const char * f, ...) __attribute__((format(printf, 3, 4))); > +}; > + > +VDerived::VDerived(int, const char *, ...) > +{ > +} > + > +Derived::Derived(int, const char *, ...) > +{ > +} > + > +int > +main(int, char **) > +{ > + throw VDerived(1, "%s %d", "foo", 1); > + throw Derived(1, "%s %d", "bar", 1); > +} > diff --git a/gcc/testsuite/g++.dg/ext/attr-format2.C b/gcc/testsuite/g++.dg/ext/attr-format2.C > new file mode 100644 > index 00000000000..7e6eec58047 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ext/attr-format2.C > @@ -0,0 +1,38 @@ > +// PR c++/101833 > +// { dg-do compile } > +// { dg-options "-Wall" } > + > +struct B { }; > + > +struct V : virtual B { > + V(int, const char *, ...) __attribute__((format(printf, 3, 4))); > +}; > + > +struct D : B { > + D(int, const char *, ...) __attribute__((format(printf, 3, 4))); > +}; > + > +struct D2 : B { > + template<typename T> > + D2(T, const char *, ...) __attribute__((format(printf, 3, 4))); > +}; > + > +struct V2 : virtual B { > + template<typename T> > + V2(T, const char *, ...) __attribute__((format(printf, 3, 4))); > +}; > + > +struct X { > + template<typename T> > + X(T, ...) __attribute__((format(printf, 2, 3))); > +}; > + > +V v(1, "%s %d", "foo", 1); > +D d(1, "%s %d", "foo", 1); > +D2 d2(1, "%s %d", "foo", 1); > +V2 v2(1, "%s %d", "foo", 1); > + > +// Test that it actually works. > +V e1(1, "%d", 1L); // { dg-warning "expects argument of type" } > +D e2(1, "%d", 1L); // { dg-warning "expects argument of type" } > +X e3("%d", 1L); // { dg-warning "expects argument of type" } > diff --git a/gcc/testsuite/g++.dg/ext/attr-format3.C b/gcc/testsuite/g++.dg/ext/attr-format3.C > new file mode 100644 > index 00000000000..60a672cd389 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ext/attr-format3.C > @@ -0,0 +1,15 @@ > +// PR c++/101833 > +// { dg-do compile } > +// { dg-options "-Wall" } > + > +class Base {}; > + > +struct VDerived : virtual Base { > + VDerived(int, int, const char *, ...) __attribute__((format(printf, 2, 3))); // { dg-error ".format. attribute argument 2 value .2. refers to parameter type .int." } > + VDerived(int, const char *, ...) __attribute__((format(printf, 5, 6))); // { dg-warning ".format. attribute argument 2 value .5. exceeds" } > +} a(1, "%s %d", "foo", 1); > + > +struct Derived : Base { > + Derived(int, int, const char *, ...) __attribute__((format(printf, 2, 3))); // { dg-error ".format. attribute argument 2 value .2. refers to parameter type .int." } > + Derived(int, const char *, ...) __attribute__((format(printf, 5, 6))); // { dg-warning ".format. attribute argument 2 value .5. exceeds" } > +} b(1, "%s %d", "foo", 1); > > base-commit: 33ba46663cdf2c9e995265eb838585488f81e907
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc index 111a33f405a..bcc316fa259 100644 --- a/gcc/c-family/c-attribs.cc +++ b/gcc/c-family/c-attribs.cc @@ -594,18 +594,23 @@ attribute_takes_identifier_p (const_tree attr_id) } /* Verify that argument value POS at position ARGNO to attribute NAME - applied to function TYPE refers to a function parameter at position - POS and the expected type CODE. Treat CODE == INTEGER_TYPE as - matching all C integral types except bool. If successful, return - POS after default conversions, if any. Otherwise, issue appropriate - warnings and return null. A non-zero 1-based ARGNO should be passed - in by callers only for attributes with more than one argument. */ + applied to function FN (which is either a function declaration or function + type) refers to a function parameter at position POS and the expected type + CODE. Treat CODE == INTEGER_TYPE as matching all C integral types except + bool. If successful, return POS after default conversions (and possibly + adjusted by ADJUST_POS). Otherwise, issue appropriate warnings and return + null. A non-zero 1-based ARGNO should be passed in by callers only for + attributes with more than one argument. + + N.B. This function modifies POS. */ tree -positional_argument (const_tree fntype, const_tree atname, tree pos, +positional_argument (const_tree fn, const_tree atname, tree &pos, tree_code code, int argno /* = 0 */, int flags /* = posargflags () */) { + const_tree fndecl = TYPE_P (fn) ? NULL_TREE : fn; + const_tree fntype = TYPE_P (fn) ? fn : TREE_TYPE (fn); if (pos && TREE_CODE (pos) != IDENTIFIER_NODE && TREE_CODE (pos) != FUNCTION_DECL) pos = default_conversion (pos); @@ -682,6 +687,11 @@ positional_argument (const_tree fntype, const_tree atname, tree pos, if (!prototype_p (fntype)) return pos; + /* ADJUST_POS is non-zero in C++ when the function type has invisible + parameters generated by the compiler, such as the in-charge or VTT + parameters. */ + const int adjust_pos = maybe_adjust_arg_pos_for_attribute (fndecl); + /* Verify that the argument position does not exceed the number of formal arguments to the function. When POSARG_ELLIPSIS is set, ARGNO may be beyond the last argument of a vararg @@ -690,7 +700,7 @@ positional_argument (const_tree fntype, const_tree atname, tree pos, if (!nargs || !tree_fits_uhwi_p (pos) || ((flags & POSARG_ELLIPSIS) == 0 - && !IN_RANGE (tree_to_uhwi (pos), 1, nargs))) + && !IN_RANGE (tree_to_uhwi (pos) + adjust_pos, 1, nargs))) { if (argno < 1) @@ -707,8 +717,9 @@ positional_argument (const_tree fntype, const_tree atname, tree pos, } /* Verify that the type of the referenced formal argument matches - the expected type. */ - unsigned HOST_WIDE_INT ipos = tree_to_uhwi (pos); + the expected type. Invisible parameters may have been added by + the compiler, so adjust the position accordingly. */ + unsigned HOST_WIDE_INT ipos = tree_to_uhwi (pos) + adjust_pos; /* Zero was handled above. */ gcc_assert (ipos != 0); @@ -791,7 +802,7 @@ positional_argument (const_tree fntype, const_tree atname, tree pos, return NULL_TREE; } - return pos; + return build_int_cst (TREE_TYPE (pos), ipos); } /* Return the first of DECL or TYPE attributes installed in NODE if it's diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc index 70f55f3a346..9fb664c9872 100644 --- a/gcc/c-family/c-common.cc +++ b/gcc/c-family/c-common.cc @@ -6071,8 +6071,8 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, /* Check for errors in format strings. */ if (warn_format || warn_suggest_attribute_format) - check_function_format (fntype, TYPE_ATTRIBUTES (fntype), nargs, argarray, - arglocs); + check_function_format (fndecl ? fndecl : fntype, TYPE_ATTRIBUTES (fntype), nargs, + argarray, arglocs); if (warn_format) check_function_sentinel (fntype, nargs, argarray); diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 52a85bfb783..f10be9bd67e 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1049,6 +1049,7 @@ extern tree finish_label_address_expr (tree, location_t); extern tree lookup_label (tree); extern tree lookup_name (tree); extern bool lvalue_p (const_tree); +extern int maybe_adjust_arg_pos_for_attribute (const_tree); extern bool vector_targets_convertible_p (const_tree t1, const_tree t2); extern bool vector_types_convertible_p (const_tree t1, const_tree t2, bool emit_lax_note); @@ -1493,7 +1494,7 @@ enum posargflags { POSARG_ELLIPSIS = 2 }; -extern tree positional_argument (const_tree, const_tree, tree, tree_code, +extern tree positional_argument (const_tree, const_tree, tree &, tree_code, int = 0, int = posargflags ()); extern enum flt_eval_method diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc index 98f28c0dcc6..393dda462a4 100644 --- a/gcc/c-family/c-format.cc +++ b/gcc/c-family/c-format.cc @@ -78,7 +78,7 @@ static bool check_format_string (const_tree argument, unsigned HOST_WIDE_INT format_num, int flags, bool *no_add_attrs, int expected_format_type); -static tree get_constant (const_tree fntype, const_tree atname, tree expr, +static bool get_constant (const_tree fn, const_tree atname, tree &expr, int argno, unsigned HOST_WIDE_INT *value, int flags, bool validated_p); static const char *convert_format_name_to_system_name (const char *attr_name); @@ -172,14 +172,11 @@ handle_format_arg_attribute (tree *node, tree atname, tree args, int flags, bool *no_add_attrs) { tree type = *node; - /* Note that TREE_VALUE (args) is changed in place below. */ + /* Note that TREE_VALUE (args) is changed in the get_constant call. */ tree *format_num_expr = &TREE_VALUE (args); unsigned HOST_WIDE_INT format_num = 0; - if (tree val = get_constant (type, atname, *format_num_expr, 0, &format_num, - 0, false)) - *format_num_expr = val; - else + if (!get_constant (type, atname, *format_num_expr, 0, &format_num, 0, false)) { *no_add_attrs = true; return NULL_TREE; @@ -301,38 +298,39 @@ check_format_string (const_tree fntype, unsigned HOST_WIDE_INT format_num, /* Under the control of FLAGS, verify EXPR is a valid constant that refers to a positional argument ARGNO having a string type (char* or, for targets like Darwin, a pointer to struct CFString) to - a function type FNTYPE declared with attribute ATNAME. - If valid, store the constant's integer value in *VALUE and return - the value. - If VALIDATED_P is true assert the validation is successful. - Returns the converted constant value on success, null otherwise. */ + a function FN declared with attribute ATNAME. If valid, store the + constant's integer value in *VALUE and return true. If VALIDATED_P + is true assert the validation is successful. -static tree -get_constant (const_tree fntype, const_tree atname, tree expr, int argno, + N.B. This function modifies EXPR. */ + +static bool +get_constant (const_tree fn, const_tree atname, tree &expr, int argno, unsigned HOST_WIDE_INT *value, int flags, bool validated_p) { /* Require the referenced argument to have a string type. For targets like Darwin, also accept pointers to struct CFString. */ - if (tree val = positional_argument (fntype, atname, expr, STRING_CST, + if (tree val = positional_argument (fn, atname, expr, STRING_CST, argno, flags)) { *value = TREE_INT_CST_LOW (val); - return val; + return true; } gcc_assert (!validated_p); - return NULL_TREE; + return false; } /* Decode the arguments to a "format" attribute into a function_format_info structure. It is already known that the list is of the right length. If VALIDATED_P is true, then these attributes have already been validated and must not be erroneous; - if false, it will give an error message. Returns true if the - attributes are successfully decoded, false otherwise. */ + if false, it will give an error message. FN is either a function + declaration or function type. Returns true if the attributes are + successfully decoded, false otherwise. */ static bool -decode_format_attr (const_tree fntype, tree atname, tree args, +decode_format_attr (const_tree fn, tree atname, tree args, function_format_info *info, bool validated_p) { tree format_type_id = TREE_VALUE (args); @@ -372,17 +370,12 @@ decode_format_attr (const_tree fntype, tree atname, tree args, } } - if (tree val = get_constant (fntype, atname, *format_num_expr, - 2, &info->format_num, 0, validated_p)) - *format_num_expr = val; - else + if (!get_constant (fn, atname, *format_num_expr, 2, &info->format_num, + 0, validated_p)) return false; - if (tree val = get_constant (fntype, atname, *first_arg_num_expr, - 3, &info->first_arg_num, - (POSARG_ZERO | POSARG_ELLIPSIS), validated_p)) - *first_arg_num_expr = val; - else + if (!get_constant (fn, atname, *first_arg_num_expr, 3, &info->first_arg_num, + (POSARG_ZERO | POSARG_ELLIPSIS), validated_p)) return false; if (info->first_arg_num != 0 && info->first_arg_num <= info->format_num) @@ -1154,13 +1147,12 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */) /* Check the argument list of a call to printf, scanf, etc. ATTRS are the attributes on the function type. There are NARGS argument - values in the array ARGARRAY. - Also, if -Wsuggest-attribute=format, - warn for calls to vprintf or vscanf in functions with no such format - attribute themselves. */ + values in the array ARGARRAY. FN is either a function declaration or + function type. Also, if -Wsuggest-attribute=format, warn for calls to + vprintf or vscanf in functions with no such format attribute themselves. */ void -check_function_format (const_tree fntype, tree attrs, int nargs, +check_function_format (const_tree fn, tree attrs, int nargs, tree *argarray, vec<location_t> *arglocs) { tree a; @@ -1174,7 +1166,7 @@ check_function_format (const_tree fntype, tree attrs, int nargs, { /* Yup; check it. */ function_format_info info; - decode_format_attr (fntype, atname, TREE_VALUE (a), &info, + decode_format_attr (fn, atname, TREE_VALUE (a), &info, /*validated=*/true); if (warn_format) { @@ -5150,10 +5142,14 @@ convert_format_name_to_system_name (const char *attr_name) /* Handle a "format" attribute; arguments as in struct attribute_spec.handler. */ tree -handle_format_attribute (tree *node, tree atname, tree args, +handle_format_attribute (tree node[3], tree atname, tree args, int flags, bool *no_add_attrs) { const_tree type = *node; + /* NODE[2] may be NULL, and it also may be a PARM_DECL for function + pointers. */ + const_tree fndecl = ((node[2] && TREE_CODE (node[2]) == FUNCTION_DECL) + ? node[2] : NULL_TREE); function_format_info info; #ifdef TARGET_FORMAT_TYPES @@ -5179,7 +5175,8 @@ handle_format_attribute (tree *node, tree atname, tree args, if (TREE_CODE (TREE_VALUE (args)) == IDENTIFIER_NODE) TREE_VALUE (args) = canonicalize_attr_name (TREE_VALUE (args)); - if (!decode_format_attr (type, atname, args, &info, /* validated_p = */false)) + if (!decode_format_attr (fndecl ? fndecl : type, atname, args, &info, + /* validated_p = */false)) { *no_add_attrs = true; return NULL_TREE; diff --git a/gcc/c/c-objc-common.cc b/gcc/c/c-objc-common.cc index 97850ada2c8..70e10a98e33 100644 --- a/gcc/c/c-objc-common.cc +++ b/gcc/c/c-objc-common.cc @@ -394,3 +394,12 @@ c_get_alias_set (tree t) return c_common_get_alias_set (t); } + +/* In C there are no invisible parameters like in C++ (this, in-charge, VTT, + etc.). */ + +int +maybe_adjust_arg_pos_for_attribute (const_tree) +{ + return 0; +} diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc index 63164bee638..6c77b9c19b5 100644 --- a/gcc/cp/tree.cc +++ b/gcc/cp/tree.cc @@ -6118,6 +6118,25 @@ maybe_warn_zero_as_null_pointer_constant (tree expr, location_t loc) } return false; } + +/* FNDECL is a function declaration whose type may have been altered by + adding extra parameters such as this, in-charge, or VTT. When this + takes place, the positional arguments supplied by the user (as in the + 'format' attribute arguments) may refer to the wrong argument. This + function returns an integer indicating how many arguments should be + skipped. */ + +int +maybe_adjust_arg_pos_for_attribute (const_tree fndecl) +{ + if (!fndecl) + return 0; + int n = num_artificial_parms_for (fndecl); + /* The manual states that it's the user's responsibility to account + for the implicit this parameter. */ + return n > 0 ? n - 1 : 0; +} + /* Release memory we no longer need after parsing. */ void diff --git a/gcc/testsuite/g++.dg/ext/attr-format-arg1.C b/gcc/testsuite/g++.dg/ext/attr-format-arg1.C new file mode 100644 index 00000000000..a7ad0f9ca33 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/attr-format-arg1.C @@ -0,0 +1,26 @@ +// PR c++/101833 +// { dg-do compile } +// { dg-options "-Wall" } + +struct B { }; + +struct V : virtual B { + const char *fmt (int, const char *) __attribute__((format_arg(3))); +}; + +struct D : B { + const char *fmt (int, const char *) __attribute__((format_arg(3))); +}; + +extern void fmt (const char *, ...) __attribute__((format(printf, 1, 2))); + +void +g () +{ + V v; + fmt (v.fmt (1, "%d"), 1); + fmt (v.fmt (1, "%d"), 1lu); // { dg-warning "expects argument of type" } + D d; + fmt (d.fmt (1, "%d"), 1); + fmt (d.fmt (1, "%d"), 1lu); // { dg-warning "expects argument of type" } +} diff --git a/gcc/testsuite/g++.dg/ext/attr-format1.C b/gcc/testsuite/g++.dg/ext/attr-format1.C new file mode 100644 index 00000000000..1b8464ed6ac --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/attr-format1.C @@ -0,0 +1,32 @@ +// PR c++/47634 +// { dg-do compile } + +class Base { +public: + Base() { } +}; + +class VDerived : public virtual Base { +public: + VDerived(int x, const char * f, ...) __attribute__((format(printf, 3, 4))); +}; + +class Derived : public Base { +public: + Derived(int x, const char * f, ...) __attribute__((format(printf, 3, 4))); +}; + +VDerived::VDerived(int, const char *, ...) +{ +} + +Derived::Derived(int, const char *, ...) +{ +} + +int +main(int, char **) +{ + throw VDerived(1, "%s %d", "foo", 1); + throw Derived(1, "%s %d", "bar", 1); +} diff --git a/gcc/testsuite/g++.dg/ext/attr-format2.C b/gcc/testsuite/g++.dg/ext/attr-format2.C new file mode 100644 index 00000000000..7e6eec58047 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/attr-format2.C @@ -0,0 +1,38 @@ +// PR c++/101833 +// { dg-do compile } +// { dg-options "-Wall" } + +struct B { }; + +struct V : virtual B { + V(int, const char *, ...) __attribute__((format(printf, 3, 4))); +}; + +struct D : B { + D(int, const char *, ...) __attribute__((format(printf, 3, 4))); +}; + +struct D2 : B { + template<typename T> + D2(T, const char *, ...) __attribute__((format(printf, 3, 4))); +}; + +struct V2 : virtual B { + template<typename T> + V2(T, const char *, ...) __attribute__((format(printf, 3, 4))); +}; + +struct X { + template<typename T> + X(T, ...) __attribute__((format(printf, 2, 3))); +}; + +V v(1, "%s %d", "foo", 1); +D d(1, "%s %d", "foo", 1); +D2 d2(1, "%s %d", "foo", 1); +V2 v2(1, "%s %d", "foo", 1); + +// Test that it actually works. +V e1(1, "%d", 1L); // { dg-warning "expects argument of type" } +D e2(1, "%d", 1L); // { dg-warning "expects argument of type" } +X e3("%d", 1L); // { dg-warning "expects argument of type" } diff --git a/gcc/testsuite/g++.dg/ext/attr-format3.C b/gcc/testsuite/g++.dg/ext/attr-format3.C new file mode 100644 index 00000000000..60a672cd389 --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/attr-format3.C @@ -0,0 +1,15 @@ +// PR c++/101833 +// { dg-do compile } +// { dg-options "-Wall" } + +class Base {}; + +struct VDerived : virtual Base { + VDerived(int, int, const char *, ...) __attribute__((format(printf, 2, 3))); // { dg-error ".format. attribute argument 2 value .2. refers to parameter type .int." } + VDerived(int, const char *, ...) __attribute__((format(printf, 5, 6))); // { dg-warning ".format. attribute argument 2 value .5. exceeds" } +} a(1, "%s %d", "foo", 1); + +struct Derived : Base { + Derived(int, int, const char *, ...) __attribute__((format(printf, 2, 3))); // { dg-error ".format. attribute argument 2 value .2. refers to parameter type .int." } + Derived(int, const char *, ...) __attribute__((format(printf, 5, 6))); // { dg-warning ".format. attribute argument 2 value .5. exceeds" } +} b(1, "%s %d", "foo", 1);