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

[Jason Merrill via Gcc-patches]

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