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

[Martin Sebor via Gcc-patches]

On 7/5/21 12:38 AM, Tuan Le Quang wrote:

Hi Martin,

Thank you for your quick response.

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

One case we have is wrapping a C API in a C++ one. Hence, the underlying API is C and we must specify types for format arguments. It means that generic conversion is not possible.

 > Do you have an actual
 > use case for it or did it just fall out of the varaidic template
 > implementation?

Yes, it falls out of the variadic template. It is the only way that helps variadic templates use format attribute. And I also feel the same, it might be useful sometimes.

Also, your suggestion on the code is helpful! I have drafted a new patch here. I have bootstrapped and regression tested it on x86_64-pc-linux-gnu

You've answered my questions about the design (thank you) and I don't
have any objections to the idea, but I'm not in a position to approve
the patch.  I would suggest to get Jason's input on extending
attribute format to variadic function templates, and Joseph's on
extending it to ordinary (non-variadic) functions.  I've CC'd both.

Thanks
Martin

Regards,
Tuan

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-common.h (enum posargflags): add flag POSARG_ANY for the third argument of format attribute * 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.
* 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..a46d882feba 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c

@@ -714,6 +714,9 @@ positional_argument (const_tree fntype, const_tree atname, tree pos,

    return NULL_TREE;
   }

+      if (flags & POSARG_ANY)
+    return pos;
+
        /* Where the expected code is STRING_CST accept any pointer
   expected by attribute format (this includes possibly qualified
   char pointers and, for targets like Darwin, also pointers to
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index be4b29a017b..391cfa685d4 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1462,7 +1462,10 @@ enum posargflags {
    POSARG_ZERO = 1,
    /* Consider positional attribute argument value valid if it refers
       to the ellipsis (i.e., beyond the last typed argument).  */
-  POSARG_ELLIPSIS = 2
+  POSARG_ELLIPSIS = 2,
+  /* Consider positional attribute argument value valid if it refers
+     to an argument of any type */
+  POSARG_ANY = 4
  };

  extern tree positional_argument (const_tree, const_tree, tree, tree_code,
diff --git a/gcc/c-family/c-format.c b/gcc/c-family/c-format.c
index bda3b18fcd0..0cef3152828 100644
--- a/gcc/c-family/c-format.c
+++ b/gcc/c-family/c-format.c

@@ -380,9 +380,16 @@ decode_format_attr (const_tree fntype, tree atname, tree args,

    else
      return false;

+  bool has_variable_arg = !type_argument_type(fntype, type_num_arguments(fntype) + 1);

+  int extra_flag = 0;
+  if (has_variable_arg)
+    extra_flag = POSARG_ELLIPSIS;
+  else
+    extra_flag = POSARG_ANY;
+
    if (tree val = get_constant (fntype, atname, *first_arg_num_expr,
         3, &info->first_arg_num,
-       (POSARG_ZERO | POSARG_ELLIPSIS), validated_p))
+       (POSARG_ZERO | extra_flag), validated_p))
      *first_arg_num_expr = val;
    else
      return false;

@@ -5193,11 +5200,11 @@ handle_format_attribute (tree *node, tree atname, tree args,

    tree arg_type;

    /* Verify that first_arg_num points to the last arg,
-     the ...  */
+     if the last arg is  ... */
    FOREACH_FUNCTION_ARGS (type, arg_type, iter)
      arg_num++;

-  if (arg_num != info.first_arg_num)
+  if (arg_num != info.first_arg_num && !type_argument_type(type, arg_num))
      {
        if (!(flags & (int) ATTR_FLAG_BUILT_IN))
   error ("argument to be formatted is not %<...%>");

diff --git a/gcc/testsuite/g++.dg/warn/format9.C b/gcc/testsuite/g++.dg/warn/format9.C

new file mode 100644
index 00000000000..39b615859fc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/format9.C
@@ -0,0 +1,16 @@
+// Test format attribute used with variadic templates
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wformat" }
+
+template<typename...Args>

+__attribute__((format(printf, 1, 2))) void fa (const char * fmt, Args...args)

+{
+    return;
+}
+
+int main()
+{
+    fa ("%d%d", 5, 7);
+    fa ("%d%d%d", 5, 6, 7);
+    fa ("%s%d", 3, 4); // { dg-warning "format" "printf warning" }
+}

diff --git a/gcc/testsuite/gcc.dg/format/attr-3.c b/gcc/testsuite/gcc.dg/format/attr-3.c

index 1b275c5aba8..64990c43472 100644
--- a/gcc/testsuite/gcc.dg/format/attr-3.c
+++ b/gcc/testsuite/gcc.dg/format/attr-3.c

@@ -55,7 +55,7 @@ extern void fg2 () __attribute__((format(gnu_attr_printf, 1, 1))); /* { dg-error  extern void fg3 () __attribute__((format(gnu_attr_printf, 2, 1))); /* { dg-error "follows" "bad number order" } */

  /* The format string argument must be a string type, and the arguments to
-   be formatted must be the "...".  */
+   be formatted must be the "..." if the function is variadic  */

 extern void fh0 (int, ...) __attribute__((format(gnu_attr_printf, 1, 2))); /* { dg-error ".format. attribute argument 2 value .1. refers to parameter type .int." "format int string" } */  extern void fh1 (signed char *, ...) __attribute__((format(gnu_attr_printf, 1, 2))); /* { dg-error ".format. attribute argument 2 value .1. refers to parameter type .signed char \\\*." "signed char string" } */  extern void fh2 (unsigned char *, ...) __attribute__((format(gnu_attr_printf, 1, 2))); /* { dg-error ".format. attribute argument 2 value .1. refers to parameter type .unsigned char \\\*." "unsigned char string" } */ diff --git a/gcc/testsuite/gcc.dg/format/attr-9.c b/gcc/testsuite/gcc.dg/format/attr-9.c

new file mode 100644
index 00000000000..3f6788f291c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/format/attr-9.c
@@ -0,0 +1,21 @@

+/* Test for format attributes: test usage of the attribute for non-variadic functions */ +/* Origin: Tuan Le <lequangtua...@gmail.com <mailto:lequangtua...@gmail.com>> */

+/* { dg-do compile } */
+/* { dg-options "-std=gnu99 -Wformat" } */
+
+#define DONT_GNU_PROTOTYPE
+#include "format.h"
+
+/* Usage of the attributes */

+extern void fa (const char *, int, const char *) __attribute__((format(gnu_attr_printf, 1, 2))); +extern void fb (const char *, int, int, int) __attribute__((format(gnu_attr_printf, 1, 3)));

+
+/* Some bad uses */

+extern void fc (const char *, int) __attribute__((format(gnu_attr_printf, 1, 1))); /* { dg-error "format string argument follows the arguments to be formatted" } */

+
+void
+foo (const char *s, int *p)
+{
+    fa ("%d%s", 5, "hello");

+    fa ("%d %d", 5, "hello"); /* { dg-warning "format" "attribute format __printf__" } */

+}

diff --git a/gcc/testsuite/objc.dg/attributes/method-format-1.m b/gcc/testsuite/objc.dg/attributes/method-format-1.m

index d3bb997b2aa..443f9c73120 100644
--- a/gcc/testsuite/objc.dg/attributes/method-format-1.m
+++ b/gcc/testsuite/objc.dg/attributes/method-format-1.m
@@ -20,8 +20,8 @@

 - (void) log2: (int)level  message: (const char *) my_format, ...  __attribute__ ((format (printf, 2)));    /* { dg-error "wrong" } */  + (void) debug2: (const char *) my_format, ...  __attribute__ ((format (printf))); /* { dg-error "wrong" } */  - (void) debug2: (const char *) my_format, ...  __attribute__ ((format (printf))); /* { dg-error "wrong" } */ -+ (void) alert: (const char *) my_format __attribute__ ((format (printf, 1, 2))); /* { dg-error "does not refer to a variable argument list" } */ -- (void) alert: (const char *) my_format __attribute__ ((format (printf, 1, 2))); /* { dg-error "does not refer to a variable argument list" } */ ++ (void) alert: (const char *) my_format __attribute__ ((format (printf, 1, 2))); /* { dg-warning "exceeds the number of function parameters" } */ +- (void) alert: (const char *) my_format __attribute__ ((format (printf, 1, 2))); /* { dg-warning "exceeds the number of function parameters" } */

  @end

  void test (LogObject *object)
--
2.17.1

Vào Th 4, 30 thg 6, 2021 vào lúc 03:56 Martin Sebor <mse...@gmail.com <mailto:mse...@gmail.com>> đã viết:

    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 <lequangtua...@gmail.com
    <mailto:lequangtua...@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