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

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

[Marek Polacek]

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

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
-- 
2.35.1

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

[Jason Merrill]

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

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

[Marek Polacek]

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

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

[Jason Merrill]

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
> 

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

[Marek Polacek]

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

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

[Jason Merrill]

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
> 

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

[Marek Polacek]

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,
 	      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 */)
 \f
 /* 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;
+}
+
 \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..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
-- 
2.35.1

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

[Jason Merrill]

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

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

[Marek Polacek]

On Thu, Apr 14, 2022 at 11:55:10PM -0400, Jason Merrill wrote:
> On 4/13/22 19:17, Marek Polacek wrote:
> > -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?

Done.
 
> > -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.

Thanks, this is the updated patch, scheduled for GCC 13:

-- >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): Rename to ...
	(validate_constant): ... this.  Take EXPR by reference.  Return bool
	instead of tree.
	(handle_format_arg_attribute): Don't overwrite FORMAT_NUM_EXPR by the
	return value of validate_constant.
	(decode_format_attr): Don't overwrite FORMAT_NUM_EXPR and
	FIRST_ARG_NUM_EXPR by the return value of validate_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/ChangeLog:

	* tree-core.h (struct attribute_spec): Update comment for HANDLER.

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                    | 77 ++++++++++-----------
 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 ++++
 gcc/tree-core.h                             |  6 +-
 11 files changed, 207 insertions(+), 55 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..ea57fde801c 100644
--- a/gcc/c-family/c-format.cc
+++ b/gcc/c-family/c-format.cc
@@ -78,9 +78,9 @@ 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,
-			  int argno, unsigned HOST_WIDE_INT *value,
-			  int flags, bool validated_p);
+static bool validate_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);
 
 static int first_target_format_type;
@@ -172,14 +172,12 @@ 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 validate_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 (!validate_constant (type, atname, *format_num_expr, 0, &format_num, 0,
+			  false))
     {
       *no_add_attrs = true;
       return NULL_TREE;
@@ -301,38 +299,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,
-	      unsigned HOST_WIDE_INT *value, int flags, bool validated_p)
+   N.B. This function modifies EXPR.  */
+
+static bool
+validate_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 +371,13 @@ 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 (!validate_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 (!validate_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 +1149,12 @@ decode_format_type (const char *s, bool *is_raw /* = NULL */)
 \f
 /* 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 +1168,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 +5144,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 +5177,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;
+}
+
 \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..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);
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index f1c2b6413a3..0b675636ea0 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -2115,8 +2115,10 @@ struct attribute_spec {
   bool function_type_required;
   /* Specifies if attribute affects type's identity.  */
   bool affects_type_identity;
-  /* Function to handle this attribute.  NODE points to the node to which
-     the attribute is to be applied.  If a DECL, it should be modified in
+  /* Function to handle this attribute.  NODE points to a tree[3] array,
+     where node[0] is the node to which the attribute is to be applied;
+     node[1] is the last pushed/merged declaration if one exists, and node[2]
+     may be the declaration for node[0].  If a DECL, it should be modified in
      place; if a TYPE, a copy should be created.  NAME is the canonicalized
      name of the attribute i.e. without any leading or trailing underscores.
      ARGS is the TREE_LIST of the arguments (which may be NULL).  FLAGS gives

base-commit: 93dd7f36f2066ec52137178ee52052f293e5e743
-- 
2.35.1