Patch for PR 37418
Richard Guenther
richard.guenther@gmail.com
Sun Sep 14 17:07:00 GMT 2008
On Sun, Sep 14, 2008 at 4:05 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> 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.
Thanks for the detailed analysis. Indeed while designing the type-system
(as of tree-ssa.c:useless_type_conversion_p) I didn't think of "abuses" of
qualifiers. As of the fix - would the following (IMHO cleaner - the validity
checks are used elsewhere as well) work as well?
Index: tree-ssa.c
===================================================================
--- tree-ssa.c (revision 140358)
+++ tree-ssa.c (working copy)
@@ -1126,8 +1126,12 @@ useless_type_conversion_p_1 (tree outer_
/* Don't lose casts between pointers to volatile and non-volatile
qualified types. Doing so would result in changing the semantics
of later accesses. */
- if ((TYPE_VOLATILE (TREE_TYPE (outer_type))
- != TYPE_VOLATILE (TREE_TYPE (inner_type)))
+ if (TREE_CODE (TREE_TYPE (outer_type)) != FUNCTION_TYPE
+ && TREE_CODE (TREE_TYPE (outer_type)) != METHOD_TYPE
+ && TREE_CODE (TREE_TYPE (inner_type)) != FUNCTION_TYPE
+ && TREE_CODE (TREE_TYPE (inner_type)) != METHOD_TYPE
+ && (TYPE_VOLATILE (TREE_TYPE (outer_type))
+ != TYPE_VOLATILE (TREE_TYPE (inner_type)))
&& TYPE_VOLATILE (TREE_TYPE (outer_type)))
return false;
it would ignore volatile qualifiers like const qualifiers. Which is
not 100% correct
wrt compatibility constraints, but as you note below we're in stage3 now.
Richard.
> (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
>
More information about the Gcc-patches
mailing list