Re: [PATCH][gcc] Allow functions without C-style ellipsis to use format attribute

[Martin Sebor <msebor@gmail.com>]

On 6/27/21 10:24 PM, Tuan Le Quang via Gcc-patches wrote:
> Hi,
> 
> Currently, format attribute can be used to do type-checking for arguments
> with respect to  a format string. However, only functions with a C-style
> ellipsis can use it.
> Supporting this attribute for non-variadic functions(functions without a
> C-style ellipsis) gives nice convenience especially when writing code in
> C++, we can use it for C++ variadic template functions like this
> 
> template<Args...args>
> __attribute__((format(printf, 1, 2))) void myPrint (const char * fmt,
> Args...args)

The main benefit of variadic functions templates over C vararg
functions is that they make use of the type system for type safety.
I'm not sure I see applying attribute format to them as a very
compelling use case.  (I'd expect the format string in a variadic
function template to use generic conversion specifiers, say %@ or
some such, and only let the caller specify things like flags, width
and precision but not type conversion specifiers).  Is there one
where relying on the type system isn't good enough?

> This patch will introduce these changes:
> 1. It is no longer an error simply to have a function with the format
> attribute but no C-style variadic arguments

I'm a little on the fence about this.  On the one hand it seems
unexpected to apply format checking to ordinary (non-variadic)
functions.  On the other, I can't think of anything wrong with it
and it even seems like it could be useful to specify a format for
a fixed number of arguments of fixed types.  Do you have an actual
use case for it or did it just fall out of the varaidic template
implementation?

> 2. Functions are subjected to warnings/errors as before, except errors
> mentioned in point 1 about not being variadic. For example, when a
> non-variadic function has wrong arguments, e.g
> __attribute__((format(printf, 1, 1))) or when being type-checked.
> 
> Note that behaviours of C-style variadic functions do not change, errors
> and warnings are given as before.
> 
> This patch does it by:
> 1.   Relaxing several conditions for format attribute:
>       -  Will only use POSARG_ELLIPSIS flag to call `get_constant` when
> getting attribute arguments of a variadic function
>       -  Relax the check for the last argument of the attribute (will not
> require an ellipsis argument)
>       -  (Before this patch) After passing the above check, current gcc will
> call `get_constant` to get the function parameter that the third attribute
> argument is pointing to. If POSARG_ELLIPSIS is set, `get_constant` will
> look for `...`. If not, `get_constant` will look for a C-style string. Note
> that POSARG_ELLIPSIS is set automatically for getting the third attribute
> argument.
>          (After this patch) POSARG_ELLIPSIS is set only when the function
> has C-style '...'. Now, if POSARG_ELLIPSIS is not set, `get_constant` will
> not check whether the third argument of format attribute points to a
> C-style string.
> 2.   Modifying expected outcome of a testcase in objc testsuite, where we
> expect a warning instead of an error
> 3.   Adding 2 test files
> 
> Successully bootstrapped and regression tested on x86_64-pc-linux-gnu.
> 
> Signed-off-by: Le Quang Tuan <lequangtuan6b@gmail.com>
> 
> gcc/c-family/ChangeLog:
> 
> * c-attribs.c (positional_argument): allow third argument of format
> attribute to point to parameters of any type if the function is not C-style
> variadic
> * c-format.c (decode_format_attr): read third argument with POSARG_ELLIPSIS
> only if the function has has a variable argument
> (handle_format_attribute): relax explicit checks for non-variadic functions
> 
> gcc/testsuite/ChangeLog:
> 
> * gcc.dg/format/attr-3.c: modify comment
> * objc.dg/attributes/method-format-1.m: errors do not hold anymore, a
> warning is given instead
> * g++.dg/warn/format9.C: New test with usage of variadic templates.
> * gcc.dg/format/attr-9.c: New test.
> 
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index 6bf492afcc0..7a17ce671de 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -714,6 +714,11 @@ positional_argument (const_tree fntype, const_tree
> atname, tree pos,
>     return NULL_TREE;
>    }
> 
> +  /* For format attribute with argno >= 3, we don't expect any type
> +   */
> +  if (argno >= 3 && strcmp (IDENTIFIER_POINTER(atname), "format") == 0 &&
> !(flags & POSARG_ELLIPSIS ) )
> +    return pos;

Hardcoding knowledge of individual attributes in this function doesn't
seem very robust.  Avoiding that is the purpose of the flags argument.
I'd suggest adding a bit to the posargflags enum.

Also, at this point, (flags & POSARG_ELLIPSIS) should be zero as
a result of the test above (not shown) so repeating the test shouldn't
be necessary.

Martin