diff mbox series

handle attribute format on functions without prototype (PR 93812)

Message ID 42f51c31-dc1b-26de-4b8e-37b76e45e554@gmail.com
State New
Headers show
Series handle attribute format on functions without prototype (PR 93812) | expand

Commit Message

Martin Sebor Feb. 26, 2020, 10:51 p.m. UTC
GCC accepts attribute format even on functions without a prototype.
However, when such a function is subsequently redeclared with
a prototype the attribute validation runs into trouble.  To avoid
the ICE I considered a) dropping the attribute on functions declared
without a prototype (like Clang does) and b) dropping the attribute
only when the function is subsequently redeclared with a prototype.
Since (b) is slightly safer in terms of retaining potentially useful
functionality the attached patch implements it and verifies that
the attribute is still accepted on functions without a prototype
and has the expected effect.

Tested on x86_64-linux.  I propose this for both GCC 10 and 9.

Martin

PS The solution causes a redunant warning for each instance of
a redeclaration with a prototype of a function without a prototype
but with attribute format.  Since this seems like a rare problem
I don't think it's worth going to the trouble of making sure only
one warning.

PPS I don't think it's intentional that GCC accepts attribute format
on functions without a prototype, first because it normally requires
that the format argument be a char*, and second because I could find
no tests for the "feature" in the test suite.  It would make sense
to me reject such declarations but I don't think now is a good time
for such a change.

Comments

Jeff Law Feb. 28, 2020, 9:28 p.m. UTC | #1
On Wed, 2020-02-26 at 15:51 -0700, Martin Sebor wrote:
> GCC accepts attribute format even on functions without a prototype.
> However, when such a function is subsequently redeclared with
> a prototype the attribute validation runs into trouble.  To avoid
> the ICE I considered a) dropping the attribute on functions declared
> without a prototype (like Clang does) and b) dropping the attribute
> only when the function is subsequently redeclared with a prototype.
> Since (b) is slightly safer in terms of retaining potentially useful
> functionality the attached patch implements it and verifies that
> the attribute is still accepted on functions without a prototype
> and has the expected effect.
> 
> Tested on x86_64-linux.  I propose this for both GCC 10 and 9.
> 
> Martin
> 
> PS The solution causes a redunant warning for each instance of
> a redeclaration with a prototype of a function without a prototype
> but with attribute format.  Since this seems like a rare problem
> I don't think it's worth going to the trouble of making sure only
> one warning.
> 
> PPS I don't think it's intentional that GCC accepts attribute format
> on functions without a prototype, first because it normally requires
> that the format argument be a char*, and second because I could find
> no tests for the "feature" in the test suite.  It would make sense
> to me reject such declarations but I don't think now is a good time
> for such a change.

> PR c/93812 - ICE on redeclaration of an attribute format function without
> protoype
> 
> gcc/c/ChangeLog:
> 
> 	PR c/93812
> 	* c-typeck.c (build_functype_attribute_variant): New function.
> 	(composite_type): Call it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c/93812
> 	* gcc.dg/format/proto.c: New test.
OK.  

If you want to drop like Clang for gcc-11, that's fine with me, but we'll
definitely want to warn when we drop and document the behavior change.

Thanks,
Jeff
diff mbox series

Patch

PR c/93812 - ICE on redeclaration of an attribute format function without protoype

gcc/c/ChangeLog:

	PR c/93812
	* c-typeck.c (build_functype_attribute_variant): New function.
	(composite_type): Call it.

gcc/testsuite/ChangeLog:

	PR c/93812
	* gcc.dg/format/proto.c: New test.

diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 8df0849bd8b..308fcffcfb0 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -353,7 +353,28 @@  c_vla_type_p (const_tree t)
     return true;
   return false;
 }
-
+
+/* If NTYPE is a type of a non-variadic function with a prototype
+   and OTYPE is a type of a function without a prototype and ATTRS
+   contains attribute format, diagnosess and removes it from ATTRS.
+   Returns the result of build_type_attribute_variant of NTYPE and
+   the (possibly) modified ATTRS.  */
+
+static tree
+build_functype_attribute_variant (tree ntype, tree otype, tree attrs)
+{
+  if (!prototype_p (otype)
+      && prototype_p (ntype)
+      && lookup_attribute ("format", attrs))
+    {
+      warning_at (input_location, OPT_Wattributes,
+		  "%qs attribute cannot be applied to a function that "
+		  "does not take variable arguments", "format");
+      attrs = remove_attribute ("format", attrs);
+    }
+  return build_type_attribute_variant (ntype, attrs);
+
+}
 /* Return the composite type of two compatible types.
 
    We assume that comptypes has already been done and returned
@@ -504,9 +525,9 @@  composite_type (tree t1, tree t2)
 
 	/* Save space: see if the result is identical to one of the args.  */
 	if (valtype == TREE_TYPE (t1) && !TYPE_ARG_TYPES (t2))
-	  return build_type_attribute_variant (t1, attributes);
+	  return build_functype_attribute_variant (t1, t2, attributes);
 	if (valtype == TREE_TYPE (t2) && !TYPE_ARG_TYPES (t1))
-	  return build_type_attribute_variant (t2, attributes);
+	  return build_functype_attribute_variant (t2, t1, attributes);
 
 	/* Simple way if one arg fails to specify argument types.  */
 	if (TYPE_ARG_TYPES (t1) == NULL_TREE)
diff --git a/gcc/testsuite/gcc.dg/format/proto.c b/gcc/testsuite/gcc.dg/format/proto.c
new file mode 100644
index 00000000000..b2050c9de5b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/format/proto.c
@@ -0,0 +1,89 @@ 
+/* PR c/93812 - ICE on redeclaration of an attribute format function without
+   protoype
+   It's not clear that attribute format should be accepted on functions
+   without a prototype.  If it's decided that it shouldn't be the tests
+   here will need to be adjusted.
+   { dg-do compile }
+   { dg-options "-Wall" } */
+
+#define FMT(n1, n2) __attribute__((__format__(__printf__, n1, n2)))
+
+// Exercise function declarations.
+FMT (1, 2) void print1 ();
+
+FMT (2, 3) void print2 ();
+           void print2 ();
+
+FMT (3, 4) void print3 ();
+FMT (3, 4) void print3 ();
+
+FMT (1, 2) void print4 ();
+           void print4 (void);              // { dg-warning "'format' attribute cannot be applied to a function that does not take variable arguments" }
+
+           void print5 ();
+FMT (1, 2) void print5 (void);              // { dg-warning "\\\[-Wattributes" }
+
+FMT (1, 2) void print6 ();
+          void print6 (const char*, ...);   // { dg-error "conflicting types" }
+
+           void print7 (const char*, ...);
+FMT (1, 2) void print7 ();                  // { dg-error "conflicting types" }
+
+
+// Exercise function calls.
+void test_print (void)
+{
+  print1 ("%i %s", 123, "");
+  print1 ("%s %i", 123, 123);               // { dg-warning "\\\[-Wformat" }
+
+  print2 (0, "%s %i", "", 123);
+  print2 (1, "%i %s", "", 123);             // { dg-warning "\\\[-Wformat" }
+
+  print3 (0, 1, "%s %i", "", 123);
+  print3 (1, 2, "%i %s", "", 123);          // { dg-warning "\\\[-Wformat" }
+
+  // Just verify there's no ICE.
+  print4 ();
+  print5 ();
+  print6 ("%i %s", 123, "");
+}
+
+
+// Exercise declarations of pointers to functions.
+FMT (1, 2) void (*pfprint1)();
+
+FMT (2, 3) void (*pfprint2)();
+           void (*pfprint2)();
+
+FMT (3, 4) void (*pfprint3)();
+FMT (3, 4) void (*pfprint3)();
+
+FMT (1, 2) void (*pfprint4)();
+           void (*pfprint4)(void);              // { dg-warning "'format' attribute cannot be applied to a function that does not take variable arguments" }
+
+           void (*pfprint5)();
+FMT (1, 2) void (*pfprint5)(void);              // { dg-warning "\\\[-Wattributes" }
+
+FMT (1, 2) void (*pfprint6)();
+           void (*pfprint6)(const char*, ...);   // { dg-error "conflicting types" }
+
+           void (*pfprint7)(const char*, ...);
+FMT (1, 2) void (*pfprint7)();                  // { dg-error "conflicting types" }
+
+// Exercise calls via function pointers.
+void test_pfprint (void)
+{
+  pfprint1 ("%i %s", 123, "");
+  pfprint1 ("%s %i", 123, 123);             // { dg-warning "\\\[-Wformat" }
+
+  pfprint2 (0, "%s %i", "", 123);
+  pfprint2 (1, "%i %s", "", 123);           // { dg-warning "\\\[-Wformat" }
+
+  pfprint3 (0, 1, "%s %i", "", 123);
+  pfprint3 (1, 2, "%i %s", "", 123);        // { dg-warning "\\\[-Wformat" }
+
+  // Just verify there's no ICE.
+  pfprint4 ();
+  pfprint5 ();
+  pfprint6 ("%i %s", 123, "");
+}