Patchwork ObjC/ObjC++: Fix try-catch failures on a darwin, second take (this one fixes them all)

login
register
mail settings
Submitter Nicola Pero
Date Nov. 30, 2010, 6:25 p.m.
Message ID <E5284BD0-76AF-4A27-A027-B52BBAB19D31@meta-innovation.com>
Download mbox | patch
Permalink /patch/73638/
State New
Headers show

Comments

Nicola Pero - Nov. 30, 2010, 6:25 p.m.
This is a new patch to fix the sjlj exception handling machinery on  
Darwin, following
an idea I had during the recent discussions.  And this one fully  
works :-)

The way the new patch works is that when the  
objc_mark_locals_volatile() is called,
instead of immediately marking the relevant local variables as  
volatile, we record
them into a vector as needing to be marked as volatile.  At the very  
beginning of
finish_function(), then, we mark all the recorded local variables that  
need to be
volatilized as volatile (and empty the vector).  Doing it later means  
the parser warnings
about volatile qualifiers being lost are automatically avoided (since  
they have already been
produced by the time we volatilize the variables) without any needs  
for hacks. :-)

I was worried that marking them as volatile later may cause problems,  
but the existing
code is already marking them as volatile much later than they are  
defined (by the time
the setjmp is reached, the variables may already have been used a  
lot).  So the new code
is not any different in that respect, it still marks them as volatile  
after they have been defined
and maybe used (but, obviously, still before gimplify / code  
generation etc so the volatilization
still affects register allocation etc. and make them preserved across  
setjmp/longjmp).

With this new way of avoiding the warnings in place, I was able to  
remove all the complications
of creating objc_volatilized types etc.  The code ends up being quite  
clean as we can simply mark
the variables as volatile in the standard way and we get no warnings  
as the warnings have already
been generated.

It also simplifies the C/C++ frontend as I could remove a number of  
clumsy hooks from there
that were there only to silence type comparison warnings; and it feels  
that the new solution is solid
and won't be broken easily by changes in type management or such like  
in the rest of the compiler

This patch fixes all the testcase failures for exceptions on NeXT/ 
darwin, including the warning ones
that the other one didn't. :-)

It may still be a good future idea to extract this technique and make  
it available in C
when a -fsafe-setjmp flag is specified, but since this is stage 3, I'm  
content with this patch
that just fixes the ObjC/ObjC++ exceptions on NeXT/Darwin and all the  
remaining NeXT/Darwin
specific testcase failures. :-)

Ok to commit ?

Thanks

+                                      G_("assignment discards %qv  
qualifier "
+                                         "from pointer target type"),
+                                      G_("initialization discards %qv  
qualifier "
+                                         "from pointer target type"),
+                                      G_("return discards %qv  
qualifier from "
+                                         "pointer target type"),
+                                      TYPE_QUALS (ttr) & ~TYPE_QUALS  
(ttl));
                 }
               /* If this is not a case of ignoring a mismatch in  
signedness,
                  no warning.  */
Mike Stump - Nov. 30, 2010, 8:17 p.m.
On Nov 30, 2010, at 10:25 AM, Nicola Pero wrote:
> This is a new patch to fix the sjlj exception handling machinery on Darwin, following
> an idea I had during the recent discussions.  And this one fully works :-)

Sweet.  Aside from debug information, I don't know of any downside.  I think this should be strictly more reliable than the previous code.  Could you test debug information and ensure we don't see an extra voltile:

...
  int bar;
...


(gdb) ptype bar

We want to ensure we don't get volatile int.  If we do, we might need a line or two in the debugging backends to imagine a type that doesn't have volatile set.  Also, if volatile is on the decl, there should be an early bailout, so that:

  volatile int bar;

(gdb) ptype bar

does print volatile int.

> Ok to commit ?

Ok, and thanks for working on this.

Patch

Index: c-family/ChangeLog
===================================================================
--- c-family/ChangeLog  (revision 167292)
+++ c-family/ChangeLog  (working copy)
@@ -1,3 +1,12 @@ 
+2010-11-30  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * c-common.h (objc_finish_function): New.
+       (objc_non_volatilized_type): Removed.
+       (objc_type_quals_match): Removed.
+       * stub-objc.c (objc_finish_function): New.
+       (objc_non_volatilized_type): Removed.
+       (objc_type_quals_match): Removed.
+
  2010-11-29  Joseph Myers  <joseph@codesourcery.com>

         * c-opts.c (check_deps_environment_vars): Use getenv instead of
Index: c-family/c-common.h
===================================================================
--- c-family/c-common.h (revision 167292)
+++ c-family/c-common.h (working copy)
@@ -996,12 +996,10 @@  extern tree objc_is_object_ptr (tree);
  extern void objc_check_decl (tree);
  extern void objc_check_global_decl (tree);
  extern tree objc_common_type (tree, tree);
-extern tree objc_non_volatilized_type (tree);
  extern bool objc_compare_types (tree, tree, int, tree);
  extern bool objc_have_common_type (tree, tree, int, tree);
  extern bool objc_diagnose_private_ivar (tree);
  extern void objc_volatilize_decl (tree);
-extern bool objc_type_quals_match (tree, tree);
  extern tree objc_rewrite_function_call (tree, tree);
  extern tree objc_message_selector (void);
  extern tree objc_lookup_ivar (tree, tree);
@@ -1063,6 +1061,7 @@  extern const char * objc_maybe_printable
  extern bool objc_is_property_ref (tree);
  extern bool objc_string_ref_type_p (tree);
  extern void objc_check_format_arg (tree, tree);
+extern void objc_finish_function (void);

  /* The following are provided by the C and C++ front-ends, and  
called by
     ObjC/ObjC++.  */
Index: c-family/stub-objc.c
===================================================================
--- c-family/stub-objc.c        (revision 167292)
+++ c-family/stub-objc.c        (working copy)
@@ -67,12 +67,6 @@  objc_check_global_decl (tree ARG_UNUSED
  }

  tree
-objc_non_volatilized_type (tree type)
-{
-  return type;
-}
-
-tree
  objc_common_type (tree ARG_UNUSED (type1), tree ARG_UNUSED (type2))
  {
    return 0;
@@ -97,12 +91,6 @@  objc_volatilize_decl (tree ARG_UNUSED (d
  {
  }

-bool
-objc_type_quals_match (tree ARG_UNUSED (ltyp), tree ARG_UNUSED (rtyp))
-{
-  return false;
-}
-
  tree
  objc_rewrite_function_call (tree function, tree ARG_UNUSED  
(first_param))
  {
@@ -461,3 +449,8 @@  objc_check_format_arg (tree ARG_UNUSED (
                        tree ARG_UNUSED (args_list))
  {
  }
+
+void
+objc_finish_function (void)
+{
+}
Index: objc/objc-act.c
===================================================================
--- objc/objc-act.c     (revision 167292)
+++ objc/objc-act.c     (working copy)
@@ -405,6 +405,10 @@  static int objc_collecting_ivars = 0;

  static char *errbuf;   /* Buffer for error diagnostics */

+/* An array of all the local variables in the current function that
+   need to be marked as volatile.  */
+VEC(tree,gc) *local_variables_to_volatilize = NULL;
+
  ^L
  static int flag_typed_selectors;

@@ -2257,61 +2261,6 @@  objc_build_struct (tree klass, tree fiel
    return s;
  }

-/* Build a type differing from TYPE only in that TYPE_VOLATILE is set.
-   Unlike tree.c:build_qualified_type(), preserve TYPE_LANG_SPECIFIC  
in the
-   process.  */
-static tree
-objc_build_volatilized_type (tree type)
-{
-  tree t;
-
-  /* Check if we have not constructed the desired variant already.  */
-  for (t = TYPE_MAIN_VARIANT (type); t; t = TYPE_NEXT_VARIANT (t))
-    {
-      /* The type qualifiers must (obviously) match up.  */
-      if (!TYPE_VOLATILE (t)
-         || (TYPE_READONLY (t) != TYPE_READONLY (type))
-         || (TYPE_RESTRICT (t) != TYPE_RESTRICT (type)))
-       continue;
-
-      /* For pointer types, the pointees (and hence their  
TYPE_LANG_SPECIFIC
-        info, if any) must match up.  */
-      if (POINTER_TYPE_P (t)
-         && (TREE_TYPE (t) != TREE_TYPE (type)))
-       continue;
-
-      /* Only match up the types which were previously volatilized in  
similar fashion and not
-        because they were declared as such. */
-      if (!lookup_attribute ("objc_volatilized", TYPE_ATTRIBUTES (t)))
-       continue;
-
-      /* Everything matches up!  */
-      return t;
-    }
-
-  /* Ok, we could not re-use any of the pre-existing variants.  Create
-     a new one.  */
-  t = build_variant_type_copy (type);
-  TYPE_VOLATILE (t) = 1;
-
-  TYPE_ATTRIBUTES (t) = merge_attributes (TYPE_ATTRIBUTES (type),
-                                         tree_cons (get_identifier  
("objc_volatilized"),
-                                         NULL_TREE,
-                                         NULL_TREE));
-  if (TREE_CODE (t) == ARRAY_TYPE)
-    TREE_TYPE (t) = objc_build_volatilized_type (TREE_TYPE (t));
-
-  /* Set up the canonical type information. */
-  if (TYPE_STRUCTURAL_EQUALITY_P (type))
-    SET_TYPE_STRUCTURAL_EQUALITY (t);
-  else if (TYPE_CANONICAL (type) != type)
-    TYPE_CANONICAL (t) = objc_build_volatilized_type (TYPE_CANONICAL  
(type));
-  else
-    TYPE_CANONICAL (t) = t;
-
-  return t;
-}
-
  /* Mark DECL as being 'volatile' for purposes of Darwin
     _setjmp()/_longjmp() exception handling.  Called from
     objc_mark_locals_volatile().  */
@@ -2324,17 +2273,44 @@  objc_volatilize_decl (tree decl)
        && (TREE_CODE (decl) == VAR_DECL
           || TREE_CODE (decl) == PARM_DECL))
      {
-      tree t = TREE_TYPE (decl);
+      if (local_variables_to_volatilize == NULL)
+       local_variables_to_volatilize = VEC_alloc (tree, gc, 8);

-      t = objc_build_volatilized_type (t);
+      VEC_safe_push (tree, gc, local_variables_to_volatilize, decl);
+    }
+}

-      TREE_TYPE (decl) = t;
-      TREE_THIS_VOLATILE (decl) = 1;
-      TREE_SIDE_EFFECTS (decl) = 1;
-      DECL_REGISTER (decl) = 0;
+/* Called when parsing of a function completes; if any local variables
+   in the function were marked as variables to volatilize, change them
+   to volatile.  We do this at the end of the function when the
+   warnings about discarding 'volatile' have already been produced.
+   We are making the variables as volatile just to force the compiler
+   to preserve them between setjmp/longjmp, but we don't want warnings
+   for them as they aren't really volatile.  */
+void
+objc_finish_function (void)
+{
+  /* If there are any local variables to volatilize, volatilize  
them.  */
+  if (local_variables_to_volatilize)
+    {
+      int i;
+      tree decl;
+      FOR_EACH_VEC_ELT (tree, local_variables_to_volatilize, i, decl)
+       {
+         tree t = TREE_TYPE (decl);
+
+         t = build_qualified_type (t, TYPE_QUALS (t) |  
TYPE_QUAL_VOLATILE);
+         TREE_TYPE (decl) = t;
+         TREE_THIS_VOLATILE (decl) = 1;
+         TREE_SIDE_EFFECTS (decl) = 1;
+         DECL_REGISTER (decl) = 0;
  #ifndef OBJCPLUS
-      C_DECL_REGISTER (decl) = 0;
+         C_DECL_REGISTER (decl) = 0;
  #endif
+       }
+
+      /* Now we delete the vector.  This sets it to NULL as well.  */
+      VEC_free (tree, gc, local_variables_to_volatilize);
      }
  }

@@ -2691,24 +2667,6 @@  objc_have_common_type (tree ltyp, tree r
    return false;
  }

-/* Check if LTYP and RTYP have the same type qualifiers.  If either  
type
-   lives in the volatilized hash table, ignore the 'volatile' bit when
-   making the comparison.  */
-
-bool
-objc_type_quals_match (tree ltyp, tree rtyp)
-{
-  int lquals = TYPE_QUALS (ltyp), rquals = TYPE_QUALS (rtyp);
-
-  if (lookup_attribute ("objc_volatilized", TYPE_ATTRIBUTES (ltyp)))
-    lquals &= ~TYPE_QUAL_VOLATILE;
-
-  if (lookup_attribute ("objc_volatilized", TYPE_ATTRIBUTES (rtyp)))
-    rquals &= ~TYPE_QUAL_VOLATILE;
-
-  return (lquals == rquals);
-}
-
  #ifndef OBJCPLUS
  /* Determine if CHILD is derived from PARENT.  The routine assumes  
that
     both parameters are RECORD_TYPEs, and is non-reflexive.  */
@@ -2828,16 +2786,6 @@  objc_check_global_decl (tree decl)
      error ("redeclaration of Objective-C class %qs",  
IDENTIFIER_POINTER (id));
  }

-/* Return a non-volatalized version of TYPE. */
-
-tree
-objc_non_volatilized_type (tree type)
-{
-  if (lookup_attribute ("objc_volatilized", TYPE_ATTRIBUTES (type)))
-    type = build_qualified_type (type, (TYPE_QUALS (type) &  
~TYPE_QUAL_VOLATILE));
-  return type;
-}
-
  /* Construct a PROTOCOLS-qualified variant of INTERFACE, where
     INTERFACE may either name an Objective-C class, or refer to the
     special 'id' or 'Class' types.  If INTERFACE is not a valid ObjC
@@ -5353,6 +5301,9 @@  objc_begin_try_stmt (location_t try_locu
        error_at (try_locus, "%<-fobjc-exceptions%> is required to  
enable Objective-C exception syntax");
      }

+  /* Collect the list of local variables.  We'll mark them as volatile
+     at the end of compilation of this function to prevent them being
+     clobbered by setjmp/longjmp.  */
    if (flag_objc_sjlj_exceptions)
      objc_mark_locals_volatile (NULL);
  }
Index: objc/ChangeLog
===================================================================
--- objc/ChangeLog      (revision 167292)
+++ objc/ChangeLog      (working copy)
@@ -1,3 +1,14 @@ 
+2010-11-30  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * objc-act.c (objc_build_volatilized_type): Removed.
+       (objc_non_volatilized_type): Removed.
+       (objc_type_quals_match): Removed.
+       (local_variables_to_volatilize): New.
+       (objc_volatilize_decl): Add the decl to volatilize to
+       local_variables_to_volatilize, but don't volatilize it yet.
+       (objc_finish_function): New.
+       * objc-act.h (local_variables_to_volatilize): New.
+
  2010-11-29  Nicola Pero  <nicola.pero@meta-innovation.com>
             Mike Stump  <mikestump@comcast.net>

Index: objc/objc-act.h
===================================================================
--- objc/objc-act.h     (revision 167292)
+++ objc/objc-act.h     (working copy)
@@ -253,6 +253,10 @@  extern GTY ((length ("SIZEHASHTABLE")))

  #define SIZEHASHTABLE          257

+/* An array of all the local variables in the current function that
+   need to be marked as volatile.  */
+extern GTY(()) VEC(tree,gc) *local_variables_to_volatilize;
+
  /* Objective-C/Objective-C++ @implementation list.  */

  struct GTY(()) imp_entry {
Index: ChangeLog
===================================================================
--- ChangeLog   (revision 167292)
+++ ChangeLog   (working copy)
@@ -1,3 +1,10 @@ 
+2010-11-30  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * c-decl.c (finish_function): Call objc_finish_function in
+       Objective-C.
+       * c-typeck.c (convert_for_assignment): Do not call
+       objc_type_quals_match().
+
  2010-11-30  Richard Guenther  <rguenther@suse.de>

         PR lto/44986
Index: cp/typeck.c
===================================================================
--- cp/typeck.c (revision 167292)
+++ cp/typeck.c (working copy)
@@ -7869,16 +7869,10 @@  comp_ptr_ttypes_real (tree to, tree from
          so the usual checks are not appropriate.  */
        if (TREE_CODE (to) != FUNCTION_TYPE && TREE_CODE (to) !=  
METHOD_TYPE)
         {
-         /* In Objective-C++, some types may have been 'volatilized' by
-            the compiler for EH; when comparing them here, the volatile
-            qualification must be ignored.  */
-         tree nv_to = objc_non_volatilized_type (to);
-         tree nv_from = objc_non_volatilized_type (from);
-
-         if (!at_least_as_qualified_p (nv_to, nv_from))
+         if (!at_least_as_qualified_p (to, from))
             return 0;

-         if (!at_least_as_qualified_p (nv_from, nv_to))
+         if (!at_least_as_qualified_p (from, to))
             {
               if (constp == 0)
                 return 0;
@@ -7886,7 +7880,7 @@  comp_ptr_ttypes_real (tree to, tree from
             }

           if (constp > 0)
-           constp &= TYPE_READONLY (nv_to);
+           constp &= TYPE_READONLY (to);
         }

        if (TREE_CODE (to) == VECTOR_TYPE)
Index: cp/decl.c
===================================================================
--- cp/decl.c   (revision 167292)
+++ cp/decl.c   (working copy)
@@ -12811,6 +12811,9 @@  finish_function (int flags)
    if (fndecl == NULL_TREE)
      return error_mark_node;

+  if (c_dialect_objc ())
+    objc_finish_function ();
+
    gcc_assert (!defer_mark_used_calls);
    defer_mark_used_calls = true;

Index: cp/ChangeLog
===================================================================
--- cp/ChangeLog        (revision 167292)
+++ cp/ChangeLog        (working copy)
@@ -1,3 +1,12 @@ 
+2010-11-30  Nicola Pero  <nicola.pero@meta-innovation.com>
+
+       * decl.c (finish_function): Call objc_finish_function when
+       compiling Objective-C++.
+       * call.c (standard_conversion): Do not call
+       objc_non_volatilized_type().
+       (implicit_conversion): Same change.
+       * typeck.c (comp_ptr_ttypes_real): Same change.
+
  2010-11-29  Dodji Seketeli  <dodji@redhat.com>

         PR c++/42260
Index: cp/call.c
===================================================================
--- cp/call.c   (revision 167292)
+++ cp/call.c   (working copy)
@@ -872,8 +872,6 @@  standard_conversion (tree to, tree from,
                && TREE_CODE (TREE_TYPE (from)) != FUNCTION_TYPE)
         {
           tree nfrom = TREE_TYPE (from);
-         if (c_dialect_objc ())
-           nfrom = objc_non_volatilized_type (nfrom);
           from = build_pointer_type
             (cp_build_qualified_type (void_type_node,
                                       cp_type_quals (nfrom)));
@@ -1483,9 +1481,6 @@  implicit_conversion (tree to, tree from,
        || expr == error_mark_node)
      return NULL;

-  if (c_dialect_objc ())
-    from = objc_non_volatilized_type (from);
-
    if (TREE_CODE (to) == REFERENCE_TYPE)
      conv = reference_binding (to, from, expr, c_cast_p, flags);
    else
Index: c-decl.c
===================================================================
--- c-decl.c    (revision 167292)
+++ c-decl.c    (working copy)
@@ -8184,6 +8184,9 @@  void
  finish_function (void)
  {
    tree fndecl = current_function_decl;
+
+  if (c_dialect_objc ())
+    objc_finish_function ();

    if (TREE_CODE (fndecl) == FUNCTION_DECL
        && targetm.calls.promote_prototypes (TREE_TYPE (fndecl)))
Index: c-typeck.c
===================================================================
--- c-typeck.c  (revision 167292)
+++ c-typeck.c  (working copy)
@@ -5605,20 +5605,16 @@  convert_for_assignment (location_t locat
               if (TYPE_QUALS_NO_ADDR_SPACE (ttr)
                   & ~TYPE_QUALS_NO_ADDR_SPACE (ttl))
                 {
-                 /* Types differing only by the presence of the  
'volatile'
-                    qualifier are acceptable if the 'volatile' has  
been added
-                    in by the Objective-C EH machinery.  */
-                 if (!objc_type_quals_match (ttl, ttr))
-                   WARN_FOR_QUALIFIERS (location, 0,
-                                        G_("passing argument %d of  
%qE discards "
-                                           "%qv qualifier from  
pointer target type"),
-                                        G_("assignment discards %qv  
qualifier "
-                                           "from pointer target type"),
-                                        G_("initialization discards  
%qv qualifier "
-                                           "from pointer target type"),
-                                        G_("return discards %qv  
qualifier from "
-                                           "pointer target type"),
-                                        TYPE_QUALS (ttr) &  
~TYPE_QUALS (ttl));
+                 WARN_FOR_QUALIFIERS (location, 0,
+                                      G_("passing argument %d of %qE  
discards "
+                                         "%qv qualifier from pointer  
target type"),