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

[Jason Merrill <jason@redhat.com>]

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;
> +}
> +
>   \f
>   /* 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