From: Jason Merrill <jason@redhat.com>
To: Marek Polacek <polacek@redhat.com>
Cc: Joseph Myers <joseph@codesourcery.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] c, c++: attribute format on a ctor with a vbase [PR101833, PR47634]
Date: Mon, 11 Apr 2022 16:39:22 -0400 [thread overview]
Message-ID: <c0acee37-1e3a-37e5-26e5-ae362beef90a@redhat.com> (raw)
In-Reply-To: <YlCLK5VXMLv7Brj7@redhat.com>
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
>
next prev parent reply other threads:[~2022-04-11 20:39 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-01 19:14 Marek Polacek
2022-04-06 20:55 ` Jason Merrill
2022-04-08 19:21 ` Marek Polacek
2022-04-11 20:39 ` Jason Merrill [this message]
2022-04-12 18:38 ` Marek Polacek
2022-04-12 20:01 ` Jason Merrill
2022-04-13 23:17 ` [PATCH v2] " Marek Polacek
2022-04-15 3:55 ` Jason Merrill
2022-04-21 19:53 ` [PATCH v3] " Marek Polacek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c0acee37-1e3a-37e5-26e5-ae362beef90a@redhat.com \
--to=jason@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=joseph@codesourcery.com \
--cc=polacek@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).