Patch for PR 37418

["Joseph S. Myers" <joseph at codesourcery dot com>]

This patch fixes (or at least works around) PR 37418, an ICE involving
taking addresses of noreturn functions failing subsequent GIMPLE
validity checks.

The underlying problem is that the qualifiers GCC uses to represent
noreturn attributes are not placed on the function type in all cases
(see <http://gcc.gnu.org/ml/gcc/2005-05/msg00974.html> for further
discussion of this), only on the DECL, so some code to add them to the
pointer type when taking the address triggered, and that code
generated trees now considered invalid.  (For variables, this code
should have been dead since Mark's patch
<http://gcc.gnu.org/ml/gcc-patches/2001-06/msg00498.html>.)  The
GIMPLE verifier allows changes of const qualifiers in ADDR_EXPR -
although given that for functions they have semantic significance,
arguably it shouldn't do so for functions - but doesn't allow such
changes of volatile qualifiers; so only two of the four testcases
added actually failed before the patch.

So I tried placing those qualifiers directly on the type and putting
an assertion in place of adding the qualifier when forming an
ADDR_EXPR.  But this runs into other problems.

First, various back ends add TREE_READONLY directly to built-in
functions without adding the const qualifier to their types.  So those
back ends would all need updating to ensure the qualifiers are set
correctly in all cases.

Second, placing the qualifiers on the type leads to further issues
about how these attributes are meant to interact with the type system.
It is the case, and has been since at least 2.7, that in the code

extern void f(void) __attribute__((noreturn));
extern void f(void);
extern void (*fp)(void) __attribute__((noreturn));
extern void (*fp)(void);

the duplicate declarations of the attributed function are accepted,
and the duplicate declarations of the function pointer are rejected
(this is the case for C++ as well).  This patch preserves this - you
don't get past building libgcc without allowing some declarations to
have the noreturn attribute and others not.  But it doesn't really fit
with either obvious model of how these attributes fit in the type
system.  One model is that variants differing only in these attributes
are generally compatible, and attributes nested anywhere are merged as
needed - like complete and incomplete array types, or prototyped and
unprototyped function types.  In that model, both duplicates should be
accepted.  But there's various code in the compiler checking
assignments etc. for presence of these qualifiers on function types.
In this model, these attributes act like normal qualifiers but
reversed in that you can quietly convert from a qualified function to
a general one but not the other way round - but then you'd expect such
duplicate declarations to be rejected as well.

Making the types compatible runs into problems with losing the
existing qualifier-like checks on uses of these attributes.  If they
are incompatible, you need special cases to allow the duplicates in
the first case, and new code to handle merging the qualifiers, and in
turn you find various checks with comptypes that would only expect a
failure for an erroneous program (before merging types of multiple
declarations in one or another case) also need to handle the different
qualifiers.  This requires working out what every call to comptypes
wants (probably including C++ front end changes as well - I didn't get
that far), and seems likely to end up introducing as many problems as
one is trying to fix.

So if you don't change the representation of const and noreturn
functions in the present Stage 3, then this issue needs solving some
other way.  The next thing I tried was inserting conversions in
build_unary_op.  But if you insert conversions without having
compatibility then this runs into the checks for casting functions
pointers to incompatible types and inserting traps.  __builtin_trap is
a noreturn function and you get infinite recursion as the code decides
__builtin_trap is being cast to an incompatible type and tries to
insert another trap.  So that code would need to handle such
conversions specially.  Either it could treat them as not
incompatible, or it could remove those conversions.  In the first case
the function calls are now calls to an expression involving a
conversion instead of directly to a known function, and in the second
case the calls are now through a pointer to a non-qualified type
instead of through a pointer to a qualified type.  Both seem like they
could potentially affect optimization adversely.

This leads to the patch I have: declaring that this particular case of
ADDR_EXPR is OK and allowing it in GIMPLE verification.  Bootstrapped
with no regressions on i686-pc-linux-gnu.  OK to commit?

I still think setting the qualifiers on the types of const and
noreturn functions is the right thing to do, and is needed to fix the
const and noreturn parts of PR 3481.  I now have two incomplete and
mostly nonoverlapping patches attempting different parts of the
problem (from my attempts on this bug, and a previous partial patch
from 2001).  But it's also clear from this attempt that adding the
qualifiers has consequences for a lot of different code using
qualifiers on types that may or may not be function types - with
language design consequences for the type system that need to be
carefully thought out - and from the 2001 attempt that a lot of code
needs checking to find everything checking or setting the const or
noreturn properties on functions and make it use the qualifiers on the
type instead.  And as such, despite PR 3481 being a clear bug, such
fixes are not Stage 3 material.

(Actually I think that even if qualifiers are used as the
representation, there should be distinct accessors for object type
qualifiers and const/noreturn properties, and when tree-checking is
enabled those accessors should make sure the type is an
object/function type as applicable.  Front end code would then
generally not use the low-level TYPE_QUALS that access shared bits
directly.)

Attribute proposals for C1x are at an early stage, though const and
noreturn attributes may well end up being standardized.  An important
issue for such proposals will be how they interact with the type
system.  Such questions for some other attributes were discussed in
<http://gcc.gnu.org/ml/gcc/2006-10/msg00318.html>.  Having thought a
lot about the issues (including detailed semantics of individual
attributes), having attempted fixes such as this one, having rewritten
attributes support once in the past and reverse-engineered fine
details of attributes semantics on other occasions, I cannot say I
have been impressed by C1x proposals when they have gone as far as
proposing specific semantics or syntax rather than just broad surveys
of what attributes presently exist; proposals don't really seem to
have tackled the hard issues of defining the precise, general
semantics while remaining compatible with existing practice in cases
that presently work and are used.

In the C++0x context, N2751 also leaves me distinctly unimpressed that
it refers to GCC 3.0.1 as "current" and is still talking about some
speculative text I removed from the manual a while back
<http://gcc.gnu.org/ml/gcc-patches/2007-09/msg01613.html> as if it
were a good idea (I specifically pointed out the nature of those
speculations in WG14 N1259).  Not taking into account post-2001
experience of the issues with attributes or implementation experience
with what the problems with attributes have been in practice does not
make for a good specification.  (N2751 gives a list of people the
proposals were discussed with, none of them GCC developers, and while
no doubt many have extensive C++ experience in their own ways,
ignoring all those people with experience of working on the GCC
implementation and knowledge from experience of the pitfalls with it,
when trying to standardize something derived from it, seems a very
strange decision to me.  C++ is more open to invention incompatible
with existing practice than C, but that should be taken as an
opportunity to address issues found in implementation practice rather
than to ignore them.)

2008-09-14  Joseph Myers  <joseph@codesourcery.com>

	PR middle-end/37418
	* tree-cfg.c (verify_types_in_gimple_assign): Allow ADDR_EXPR to
	add volatile qualification when taking the address of a noreturn
	function.

testsuite:
2008-09-14  Joseph Myers  <joseph@codesourcery.com>

	PR middle-end/37418
	* gcc.c-torture/compile/pr37418-1.c,
	gcc.c-torture/compile/pr37418-2.c,
	gcc.c-torture/compile/pr37418-3.c,
	gcc.c-torture/compile/pr37418-4.c: New tests.

Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 140346)
+++ tree-cfg.c	(working copy)
@@ -3478,6 +3478,17 @@
 	  }
 
 	if (!one_pointer_to_useless_type_conversion_p (lhs_type, TREE_TYPE (op))
+	    /* FIXME: noreturn functions do not get volatile-qualified
+	       type.  */
+	    && !(TREE_CODE (op) == FUNCTION_DECL
+		 && TREE_THIS_VOLATILE (op)
+		 && !TYPE_VOLATILE (TREE_TYPE (op))
+		 && TYPE_VOLATILE (TREE_TYPE (lhs_type))
+		 && one_pointer_to_useless_type_conversion_p
+		      (lhs_type,
+		       build_qualified_type (TREE_TYPE (op),
+					     (TYPE_QUALS (TREE_TYPE (op))
+					      | TYPE_QUAL_VOLATILE))))
 	    /* FIXME: a longstanding wart, &a == &a[0].  */
 	    && (TREE_CODE (TREE_TYPE (op)) != ARRAY_TYPE
 		|| !one_pointer_to_useless_type_conversion_p (lhs_type,
Index: testsuite/gcc.c-torture/compile/pr37418-1.c
===================================================================
--- testsuite/gcc.c-torture/compile/pr37418-1.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/pr37418-1.c	(revision 0)
@@ -0,0 +1,6 @@
+typedef void ft(int);
+void f(int args)__attribute__((noreturn));
+void f2(ft *p __attribute__((noreturn)))
+{
+  p = f;
+}
Index: testsuite/gcc.c-torture/compile/pr37418-2.c
===================================================================
--- testsuite/gcc.c-torture/compile/pr37418-2.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/pr37418-2.c	(revision 0)
@@ -0,0 +1,6 @@
+typedef void ft(int);
+volatile ft f;
+void f2(ft *p __attribute__((noreturn)))
+{
+  p = f;
+}
Index: testsuite/gcc.c-torture/compile/pr37418-3.c
===================================================================
--- testsuite/gcc.c-torture/compile/pr37418-3.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/pr37418-3.c	(revision 0)
@@ -0,0 +1,6 @@
+typedef void ft(int);
+void f(int args)__attribute__((const));
+void f2(ft *p __attribute__((const)))
+{
+  p = f;
+}
Index: testsuite/gcc.c-torture/compile/pr37418-4.c
===================================================================
--- testsuite/gcc.c-torture/compile/pr37418-4.c	(revision 0)
+++ testsuite/gcc.c-torture/compile/pr37418-4.c	(revision 0)
@@ -0,0 +1,6 @@
+typedef void ft(int);
+const ft f;
+void f2(ft *p __attribute__((const)))
+{
+  p = f;
+}

-- 
Joseph S. Myers
joseph@codesourcery.com