c, c++: attribute format on a ctor with a vbase [PR101833, PR47634]

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.
	(handle_format_arg_attribute): Pass 0 to get_constant.
	(get_constant): Add new argument adjust_pos, use it.
	(check_function_format): Add fndecl argument.  Pass it to
	decode_format_attr.
	(handle_format_attribute): Get the fndecl from node[2].  Pass it 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                   | 14 ++++---
 gcc/c-family/c-common.cc                    |  4 +-
 gcc/c-family/c-common.h                     |  5 ++-
 gcc/c-family/c-format.cc                    | 46 +++++++++++++--------
 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, 182 insertions(+), 26 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


base-commit: 95533fe4f014c10dd18de649927668aba6117daf

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?

> 	(handle_format_arg_attribute): Pass 0 to get_constant.
> 	(get_constant): Add new argument adjust_pos, use it.
> 	(check_function_format): Add fndecl argument.  Pass it to
> 	decode_format_attr.
> 	(handle_format_attribute): Get the fndecl from node[2].  Pass it 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                   | 14 ++++---
>   gcc/c-family/c-common.cc                    |  4 +-
>   gcc/c-family/c-common.h                     |  5 ++-
>   gcc/c-family/c-format.cc                    | 46 +++++++++++++--------
>   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, 182 insertions(+), 26 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..6e17847ec9e 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -599,12 +599,15 @@ attribute_takes_identifier_p (const_tree attr_id)
>      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.  */
> +   in by callers only for attributes with more than one argument.
> +   ADJUST_POS is used and non-zero in C++ when the function type has
> +   invisible parameters generated by the compiler, such as the in-charge
> +   or VTT parameters.  */
>   
>   tree
>   positional_argument (const_tree fntype, const_tree atname, tree pos,
>   		     tree_code code, int argno /* = 0 */,
> -		     int flags /* = posargflags () */)
> +		     int flags /* = posargflags () */, int adjust_pos /* = 0 */)
>   {
>     if (pos && TREE_CODE (pos) != IDENTIFIER_NODE
>         && TREE_CODE (pos) != FUNCTION_DECL)
> @@ -690,7 +693,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 +710,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);
> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> index d034837bb5b..6f08b55d4a7 100644
> --- a/gcc/c-family/c-common.cc
> +++ b/gcc/c-family/c-common.cc
> @@ -6069,8 +6069,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 (fntype, fndecl, 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..db6ff07db37 100644
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -857,7 +857,7 @@ extern void check_function_arguments_recurse (void (*)
>   					      opt_code);
>   extern bool check_builtin_function_arguments (location_t, vec<location_t>,
>   					      tree, tree, int, tree *);
> -extern void check_function_format (const_tree, tree, int, tree *,
> +extern void check_function_format (const_tree, const_tree, tree, int, tree *,
>   				   vec<location_t> *);
>   extern bool attribute_fallthrough_p (tree);
>   extern tree handle_format_attribute (tree *, tree, tree, int, bool *);
> @@ -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);
> @@ -1494,7 +1495,7 @@ enum posargflags {
>   };
>   
>   extern tree positional_argument (const_tree, const_tree, tree, tree_code,
> -				 int = 0, int = posargflags ());
> +				 int = 0, int = posargflags (), int = 0);
>   
>   extern enum flt_eval_method
>   excess_precision_mode_join (enum flt_eval_method, enum flt_eval_method);
> diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc
> index 98f28c0dcc6..87ae7bb73b0 100644
> --- a/gcc/c-family/c-format.cc
> +++ b/gcc/c-family/c-format.cc
> @@ -70,8 +70,8 @@ static GTY(()) tree local_gimple_ptr_node;
>   static GTY(()) tree local_cgraph_node_ptr_node;
>   static GTY(()) tree locus;
>   
> -static bool decode_format_attr (const_tree, tree, tree, function_format_info *,
> -				bool);
> +static bool decode_format_attr (const_tree, const_tree, tree, tree,
> +				function_format_info *, bool);
>   static format_type decode_format_type (const char *, bool * = NULL);
>   
>   static bool check_format_string (const_tree argument,
> @@ -80,7 +80,7 @@ static bool check_format_string (const_tree argument,
>   				 int expected_format_type);
>   static tree get_constant (const_tree fntype, const_tree atname, tree expr,
>   			  int argno, unsigned HOST_WIDE_INT *value,
> -			  int flags, bool validated_p);
> +			  int flags, bool validated_p, int adjust_pos);
>   static const char *convert_format_name_to_system_name (const char *attr_name);
>   
>   static int first_target_format_type;
> @@ -177,7 +177,7 @@ handle_format_arg_attribute (tree *node, tree atname,
>     unsigned HOST_WIDE_INT format_num = 0;
>   
>     if (tree val = get_constant (type, atname, *format_num_expr, 0, &format_num,
> -			       0, false))
> +			       0, false, 0))
>       *format_num_expr = val;
>     else
>       {
> @@ -305,18 +305,24 @@ check_format_string (const_tree fntype, unsigned HOST_WIDE_INT format_num,
>      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.  */
> +   Returns the converted constant value on success, null otherwise.
> +   ADJUST_POS is used and non-zero in C++ when the function type has
> +   invisible parameters generated by the compiler, such as the in-charge
> +   or VTT parameters.  */
>   
>   static tree
>   get_constant (const_tree fntype, const_tree atname, tree expr, int argno,
> -	      unsigned HOST_WIDE_INT *value, int flags, bool validated_p)
> +	      unsigned HOST_WIDE_INT *value, int flags, bool validated_p,
> +	      int adjust_pos)
>   {
>     /* 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,
> -				      argno, flags))
> +				      argno, flags, adjust_pos))
>       {
> -      *value = TREE_INT_CST_LOW (val);
> +      /* Invisible parameters may have been added by the compiler, so adjust
> +	 the position accordingly.  */
> +      *value = TREE_INT_CST_LOW (val) + adjust_pos;
>         return val;
>       }
>   
> @@ -332,8 +338,8 @@ get_constant (const_tree fntype, const_tree atname, tree expr, int argno,
>      attributes are successfully decoded, false otherwise.  */
>   
>   static bool
> -decode_format_attr (const_tree fntype, tree atname, tree args,
> -		    function_format_info *info, bool validated_p)
> +decode_format_attr (const_tree fntype, const_tree fndecl, tree atname,
> +		    tree args, function_format_info *info, bool validated_p)
>   {
>     tree format_type_id = TREE_VALUE (args);
>     /* Note that TREE_VALUE (args) is changed in place below.  Ditto
> @@ -372,15 +378,19 @@ decode_format_attr (const_tree fntype, tree atname, tree args,
>   	}
>       }
>   
> +  int adjust_pos = maybe_adjust_arg_pos_for_attribute (fndecl);
> +
>     if (tree val = get_constant (fntype, atname, *format_num_expr,
> -			       2, &info->format_num, 0, validated_p))
> +			       2, &info->format_num, 0, validated_p,
> +			       adjust_pos))
>       *format_num_expr = val;
>     else
>       return false;
>   
>     if (tree val = get_constant (fntype, atname, *first_arg_num_expr,
>   			       3, &info->first_arg_num,
> -			       (POSARG_ZERO | POSARG_ELLIPSIS), validated_p))
> +			       (POSARG_ZERO | POSARG_ELLIPSIS), validated_p,
> +			       adjust_pos))
>       *first_arg_num_expr = val;
>     else
>       return false;
> @@ -1160,8 +1170,8 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */)
>      attribute themselves.  */
>   
>   void
> -check_function_format (const_tree fntype, tree attrs, int nargs,
> -		       tree *argarray, vec<location_t> *arglocs)
> +check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
> +		       int nargs, tree *argarray, vec<location_t> *arglocs)
>   {
>     tree a;
>   
> @@ -1174,7 +1184,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 (fntype, fndecl, atname, TREE_VALUE (a), &info,
>   			      /*validated=*/true);
>   	  if (warn_format)
>   	    {
> @@ -5150,10 +5160,11 @@ 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;
> +  const_tree fndecl = node[2];
>     function_format_info info;
>   
>   #ifdef TARGET_FORMAT_TYPES
> @@ -5179,7 +5190,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 (type, fndecl, 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 780a8d89165..ddfeb8ce428 100644
> --- a/gcc/cp/tree.cc
> +++ b/gcc/cp/tree.cc
> @@ -6101,6 +6101,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..d6c9e40756f
> --- /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: 95533fe4f014c10dd18de649927668aba6117daf

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;

Replacing the arguments in the attribute list would lead to problems, because
when we're processing the constructor clone without the additional parameters,
the adjusted argument position would be out of whack at this point.

I've attempted to reduce the number of parameters, but it hardly seemed like
a win, here's the patch I came up with:

diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index 6e17847ec9e..972476fbdf4 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -594,7 +594,7 @@ 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
+   applied to function FNTYPE 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
diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
index 6f08b55d4a7..ffa36673ec0 100644
--- a/gcc/c-family/c-common.cc
+++ b/gcc/c-family/c-common.cc
@@ -6069,7 +6069,7 @@ 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, fndecl, TYPE_ATTRIBUTES (fntype), nargs,
+    check_function_format (fndecl, TYPE_ATTRIBUTES (fntype), nargs,
 			   argarray, arglocs);
 
   if (warn_format)
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index db6ff07db37..b68dc8f7d69 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -857,7 +857,7 @@ extern void check_function_arguments_recurse (void (*)
 					      opt_code);
 extern bool check_builtin_function_arguments (location_t, vec<location_t>,
 					      tree, tree, int, tree *);
-extern void check_function_format (const_tree, const_tree, tree, int, tree *,
+extern void check_function_format (const_tree, tree, int, tree *,
 				   vec<location_t> *);
 extern bool attribute_fallthrough_p (tree);
 extern tree handle_format_attribute (tree *, tree, tree, int, bool *);
diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc
index 87ae7bb73b0..6f0199dfcff 100644
--- a/gcc/c-family/c-format.cc
+++ b/gcc/c-family/c-format.cc
@@ -70,7 +70,7 @@ static GTY(()) tree local_gimple_ptr_node;
 static GTY(()) tree local_cgraph_node_ptr_node;
 static GTY(()) tree locus;
 
-static bool decode_format_attr (const_tree, const_tree, tree, tree,
+static bool decode_format_attr (const_tree, tree, tree,
 				function_format_info *, bool);
 static format_type decode_format_type (const char *, bool * = NULL);
 
@@ -330,17 +330,19 @@ get_constant (const_tree fntype, const_tree atname, tree expr, int argno,
   return NULL_TREE;
 }
 
-/* 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.  */
+/* 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.  FN is
+   either a FUNCTION_TYPE or a FUNCTION_DECL.  Returns true if the attributes
+   are successfully decoded, false otherwise.  */
 
 static bool
-decode_format_attr (const_tree fntype, const_tree fndecl, tree atname,
-		    tree args, function_format_info *info, bool validated_p)
+decode_format_attr (const_tree fn, tree atname, tree args,
+		    function_format_info *info, bool validated_p)
 {
+  const_tree fndecl = TYPE_P (fn) ? NULL_TREE : fn;
+  const_tree fntype = TYPE_P (fn) ? fn : TREE_TYPE (fn);
   tree format_type_id = TREE_VALUE (args);
   /* Note that TREE_VALUE (args) is changed in place below.  Ditto
      for the value of the next element on the list.  */
@@ -1170,7 +1172,7 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */)
    attribute themselves.  */
 
 void
-check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
+check_function_format (const_tree fndecl, tree attrs,
 		       int nargs, tree *argarray, vec<location_t> *arglocs)
 {
   tree a;
@@ -1184,7 +1186,7 @@ check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
 	{
 	  /* Yup; check it.  */
 	  function_format_info info;
-	  decode_format_attr (fntype, fndecl, atname, TREE_VALUE (a), &info,
+	  decode_format_attr (fndecl, atname, TREE_VALUE (a), &info,
 			      /*validated=*/true);
 	  if (warn_format)
 	    {
@@ -5190,7 +5192,7 @@ handle_format_attribute (tree node[3], 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, fndecl, atname, args, &info,
+  if (!decode_format_attr (fndecl ? fndecl : type, atname, args, &info,
 			   /* validated_p = */false))
     {
       *no_add_attrs = true;


Marek

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?  Maybe do that conversion 
directly in decode_format_attr instead?

> Replacing the arguments in the attribute list would lead to problems, because
> when we're processing the constructor clone without the additional parameters,
> the adjusted argument position would be out of whack at this point.
> 
> I've attempted to reduce the number of parameters, but it hardly seemed like
> a win, here's the patch I came up with:
> 
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index 6e17847ec9e..972476fbdf4 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -594,7 +594,7 @@ 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
> +   applied to function FNTYPE 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
> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> index 6f08b55d4a7..ffa36673ec0 100644
> --- a/gcc/c-family/c-common.cc
> +++ b/gcc/c-family/c-common.cc
> @@ -6069,7 +6069,7 @@ 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, fndecl, TYPE_ATTRIBUTES (fntype), nargs,
> +    check_function_format (fndecl, TYPE_ATTRIBUTES (fntype), nargs,
>   			   argarray, arglocs);
>   
>     if (warn_format)
> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> index db6ff07db37..b68dc8f7d69 100644
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -857,7 +857,7 @@ extern void check_function_arguments_recurse (void (*)
>   					      opt_code);
>   extern bool check_builtin_function_arguments (location_t, vec<location_t>,
>   					      tree, tree, int, tree *);
> -extern void check_function_format (const_tree, const_tree, tree, int, tree *,
> +extern void check_function_format (const_tree, tree, int, tree *,
>   				   vec<location_t> *);
>   extern bool attribute_fallthrough_p (tree);
>   extern tree handle_format_attribute (tree *, tree, tree, int, bool *);
> diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc
> index 87ae7bb73b0..6f0199dfcff 100644
> --- a/gcc/c-family/c-format.cc
> +++ b/gcc/c-family/c-format.cc
> @@ -70,7 +70,7 @@ static GTY(()) tree local_gimple_ptr_node;
>   static GTY(()) tree local_cgraph_node_ptr_node;
>   static GTY(()) tree locus;
>   
> -static bool decode_format_attr (const_tree, const_tree, tree, tree,
> +static bool decode_format_attr (const_tree, tree, tree,
>   				function_format_info *, bool);
>   static format_type decode_format_type (const char *, bool * = NULL);
>   
> @@ -330,17 +330,19 @@ get_constant (const_tree fntype, const_tree atname, tree expr, int argno,
>     return NULL_TREE;
>   }
>   
> -/* 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.  */
> +/* 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.  FN is
> +   either a FUNCTION_TYPE or a FUNCTION_DECL.  Returns true if the attributes
> +   are successfully decoded, false otherwise.  */
>   
>   static bool
> -decode_format_attr (const_tree fntype, const_tree fndecl, tree atname,
> -		    tree args, function_format_info *info, bool validated_p)
> +decode_format_attr (const_tree fn, tree atname, tree args,
> +		    function_format_info *info, bool validated_p)
>   {
> +  const_tree fndecl = TYPE_P (fn) ? NULL_TREE : fn;
> +  const_tree fntype = TYPE_P (fn) ? fn : TREE_TYPE (fn);
>     tree format_type_id = TREE_VALUE (args);
>     /* Note that TREE_VALUE (args) is changed in place below.  Ditto
>        for the value of the next element on the list.  */
> @@ -1170,7 +1172,7 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */)
>      attribute themselves.  */
>   
>   void
> -check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
> +check_function_format (const_tree fndecl, tree attrs,
>   		       int nargs, tree *argarray, vec<location_t> *arglocs)
>   {
>     tree a;
> @@ -1184,7 +1186,7 @@ check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
>   	{
>   	  /* Yup; check it.  */
>   	  function_format_info info;
> -	  decode_format_attr (fntype, fndecl, atname, TREE_VALUE (a), &info,
> +	  decode_format_attr (fndecl, atname, TREE_VALUE (a), &info,
>   			      /*validated=*/true);
>   	  if (warn_format)
>   	    {
> @@ -5190,7 +5192,7 @@ handle_format_attribute (tree node[3], 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, fndecl, atname, args, &info,
> +  if (!decode_format_attr (fndecl ? fndecl : type, atname, args, &info,
>   			   /* validated_p = */false))
>       {
>         *no_add_attrs = true;
> 
> 
> Marek
>

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.)
 
> > Replacing the arguments in the attribute list would lead to problems, because
> > when we're processing the constructor clone without the additional parameters,
> > the adjusted argument position would be out of whack at this point.
> > 
> > I've attempted to reduce the number of parameters, but it hardly seemed like
> > a win, here's the patch I came up with:
> > 
> > diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> > index 6e17847ec9e..972476fbdf4 100644
> > --- a/gcc/c-family/c-attribs.cc
> > +++ b/gcc/c-family/c-attribs.cc
> > @@ -594,7 +594,7 @@ 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
> > +   applied to function FNTYPE 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
> > diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
> > index 6f08b55d4a7..ffa36673ec0 100644
> > --- a/gcc/c-family/c-common.cc
> > +++ b/gcc/c-family/c-common.cc
> > @@ -6069,7 +6069,7 @@ 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, fndecl, TYPE_ATTRIBUTES (fntype), nargs,
> > +    check_function_format (fndecl, TYPE_ATTRIBUTES (fntype), nargs,
> >   			   argarray, arglocs);
> >     if (warn_format)
> > diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> > index db6ff07db37..b68dc8f7d69 100644
> > --- a/gcc/c-family/c-common.h
> > +++ b/gcc/c-family/c-common.h
> > @@ -857,7 +857,7 @@ extern void check_function_arguments_recurse (void (*)
> >   					      opt_code);
> >   extern bool check_builtin_function_arguments (location_t, vec<location_t>,
> >   					      tree, tree, int, tree *);
> > -extern void check_function_format (const_tree, const_tree, tree, int, tree *,
> > +extern void check_function_format (const_tree, tree, int, tree *,
> >   				   vec<location_t> *);
> >   extern bool attribute_fallthrough_p (tree);
> >   extern tree handle_format_attribute (tree *, tree, tree, int, bool *);
> > diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc
> > index 87ae7bb73b0..6f0199dfcff 100644
> > --- a/gcc/c-family/c-format.cc
> > +++ b/gcc/c-family/c-format.cc
> > @@ -70,7 +70,7 @@ static GTY(()) tree local_gimple_ptr_node;
> >   static GTY(()) tree local_cgraph_node_ptr_node;
> >   static GTY(()) tree locus;
> > -static bool decode_format_attr (const_tree, const_tree, tree, tree,
> > +static bool decode_format_attr (const_tree, tree, tree,
> >   				function_format_info *, bool);
> >   static format_type decode_format_type (const char *, bool * = NULL);
> > @@ -330,17 +330,19 @@ get_constant (const_tree fntype, const_tree atname, tree expr, int argno,
> >     return NULL_TREE;
> >   }
> > -/* 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.  */
> > +/* 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.  FN is
> > +   either a FUNCTION_TYPE or a FUNCTION_DECL.  Returns true if the attributes
> > +   are successfully decoded, false otherwise.  */
> >   static bool
> > -decode_format_attr (const_tree fntype, const_tree fndecl, tree atname,
> > -		    tree args, function_format_info *info, bool validated_p)
> > +decode_format_attr (const_tree fn, tree atname, tree args,
> > +		    function_format_info *info, bool validated_p)
> >   {
> > +  const_tree fndecl = TYPE_P (fn) ? NULL_TREE : fn;
> > +  const_tree fntype = TYPE_P (fn) ? fn : TREE_TYPE (fn);
> >     tree format_type_id = TREE_VALUE (args);
> >     /* Note that TREE_VALUE (args) is changed in place below.  Ditto
> >        for the value of the next element on the list.  */
> > @@ -1170,7 +1172,7 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */)
> >      attribute themselves.  */
> >   void
> > -check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
> > +check_function_format (const_tree fndecl, tree attrs,
> >   		       int nargs, tree *argarray, vec<location_t> *arglocs)
> >   {
> >     tree a;
> > @@ -1184,7 +1186,7 @@ check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
> >   	{
> >   	  /* Yup; check it.  */
> >   	  function_format_info info;
> > -	  decode_format_attr (fntype, fndecl, atname, TREE_VALUE (a), &info,
> > +	  decode_format_attr (fndecl, atname, TREE_VALUE (a), &info,
> >   			      /*validated=*/true);
> >   	  if (warn_format)
> >   	    {
> > @@ -5190,7 +5192,7 @@ handle_format_attribute (tree node[3], 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, fndecl, atname, args, &info,
> > +  if (!decode_format_attr (fndecl ? fndecl : type, atname, args, &info,
> >   			   /* validated_p = */false))
> >       {
> >         *no_add_attrs = true;
> > 

Marek

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?

>>> Replacing the arguments in the attribute list would lead to problems, because
>>> when we're processing the constructor clone without the additional parameters,
>>> the adjusted argument position would be out of whack at this point.
>>>
>>> I've attempted to reduce the number of parameters, but it hardly seemed like
>>> a win, here's the patch I came up with:
>>>
>>> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
>>> index 6e17847ec9e..972476fbdf4 100644
>>> --- a/gcc/c-family/c-attribs.cc
>>> +++ b/gcc/c-family/c-attribs.cc
>>> @@ -594,7 +594,7 @@ 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
>>> +   applied to function FNTYPE 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
>>> diff --git a/gcc/c-family/c-common.cc b/gcc/c-family/c-common.cc
>>> index 6f08b55d4a7..ffa36673ec0 100644
>>> --- a/gcc/c-family/c-common.cc
>>> +++ b/gcc/c-family/c-common.cc
>>> @@ -6069,7 +6069,7 @@ 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, fndecl, TYPE_ATTRIBUTES (fntype), nargs,
>>> +    check_function_format (fndecl, TYPE_ATTRIBUTES (fntype), nargs,
>>>    			   argarray, arglocs);
>>>      if (warn_format)
>>> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
>>> index db6ff07db37..b68dc8f7d69 100644
>>> --- a/gcc/c-family/c-common.h
>>> +++ b/gcc/c-family/c-common.h
>>> @@ -857,7 +857,7 @@ extern void check_function_arguments_recurse (void (*)
>>>    					      opt_code);
>>>    extern bool check_builtin_function_arguments (location_t, vec<location_t>,
>>>    					      tree, tree, int, tree *);
>>> -extern void check_function_format (const_tree, const_tree, tree, int, tree *,
>>> +extern void check_function_format (const_tree, tree, int, tree *,
>>>    				   vec<location_t> *);
>>>    extern bool attribute_fallthrough_p (tree);
>>>    extern tree handle_format_attribute (tree *, tree, tree, int, bool *);
>>> diff --git a/gcc/c-family/c-format.cc b/gcc/c-family/c-format.cc
>>> index 87ae7bb73b0..6f0199dfcff 100644
>>> --- a/gcc/c-family/c-format.cc
>>> +++ b/gcc/c-family/c-format.cc
>>> @@ -70,7 +70,7 @@ static GTY(()) tree local_gimple_ptr_node;
>>>    static GTY(()) tree local_cgraph_node_ptr_node;
>>>    static GTY(()) tree locus;
>>> -static bool decode_format_attr (const_tree, const_tree, tree, tree,
>>> +static bool decode_format_attr (const_tree, tree, tree,
>>>    				function_format_info *, bool);
>>>    static format_type decode_format_type (const char *, bool * = NULL);
>>> @@ -330,17 +330,19 @@ get_constant (const_tree fntype, const_tree atname, tree expr, int argno,
>>>      return NULL_TREE;
>>>    }
>>> -/* 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.  */
>>> +/* 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.  FN is
>>> +   either a FUNCTION_TYPE or a FUNCTION_DECL.  Returns true if the attributes
>>> +   are successfully decoded, false otherwise.  */
>>>    static bool
>>> -decode_format_attr (const_tree fntype, const_tree fndecl, tree atname,
>>> -		    tree args, function_format_info *info, bool validated_p)
>>> +decode_format_attr (const_tree fn, tree atname, tree args,
>>> +		    function_format_info *info, bool validated_p)
>>>    {
>>> +  const_tree fndecl = TYPE_P (fn) ? NULL_TREE : fn;
>>> +  const_tree fntype = TYPE_P (fn) ? fn : TREE_TYPE (fn);
>>>      tree format_type_id = TREE_VALUE (args);
>>>      /* Note that TREE_VALUE (args) is changed in place below.  Ditto
>>>         for the value of the next element on the list.  */
>>> @@ -1170,7 +1172,7 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */)
>>>       attribute themselves.  */
>>>    void
>>> -check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
>>> +check_function_format (const_tree fndecl, tree attrs,
>>>    		       int nargs, tree *argarray, vec<location_t> *arglocs)
>>>    {
>>>      tree a;
>>> @@ -1184,7 +1186,7 @@ check_function_format (const_tree fntype, const_tree fndecl, tree attrs,
>>>    	{
>>>    	  /* Yup; check it.  */
>>>    	  function_format_info info;
>>> -	  decode_format_attr (fntype, fndecl, atname, TREE_VALUE (a), &info,
>>> +	  decode_format_attr (fndecl, atname, TREE_VALUE (a), &info,
>>>    			      /*validated=*/true);
>>>    	  if (warn_format)
>>>    	    {
>>> @@ -5190,7 +5192,7 @@ handle_format_attribute (tree node[3], 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, fndecl, atname, args, &info,
>>> +  if (!decode_format_attr (fndecl ? fndecl : type, atname, args, &info,
>>>    			   /* validated_p = */false))
>>>        {
>>>          *no_add_attrs = true;
>>>
> 
> Marek
>