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
