| Message ID | 1290997695.991412057@192.168.2.229 |
|---|---|
| State | New |
| Headers | show |
On Nov 28, 2010, at 6:28 PM, Nicola Pero wrote: > This patch fixes the 5 @try/@catch ObjC++ testsuite failures on Darwin (Apple) with the NeXT runtime. Hum... I don't think this is going to work. > The code we "inherited" from Apple flagged these artificially volatilized types > using an attribute ("objc_volatilized") but the problem is that newer GCCs are > smarted and consider attributes when building variants of types. So the > additional attribute actually confuses the type system (for example, if you start > with a non-volatilized 'int' type, and build the artificially volatilized variant, > and then you try to get a non-volatile type variant using build_qualified_type(), > you don't get the original type but a new 'int' type which preserves your new > objc_volatilized attribute. Ah, but this sounds like a one line fix to remove the attribute in that one case. Would this work? Downsides? > This new 'int' is identical to the normal 'int' > though, except for the 'objc_volatilized' attribute, but has a different > canonical type. The C++ frontend detects this inconsistency and aborts). > So, in the end I created a new TYPE_ARTIFICIALLY_VOLATILIZED() flag that can > be used for types (I used the base.side_effects flag which is unused for types > in C/ObjC/C++/ObjC++), and used it to mark our artificially volatilized types. The problem is, that bit isn't ours to play with, and, no existing code plays with that bit or understands it and we don't really want to propagate such system specific details into such an important bit. Whereas, playing with attributes isn't unreasonable and it just for such system specific playing. > Then the type system works again and everything seems to work with the next > runtime, with the exception of two warnings in try-catch-12.m. These occur when > a real 'volatile' variable is encountered in the same context as artificially > volatilized ones, and if certain conditions are met the two types can get confused > and some warnings for the real volatile variable may not be produced. Yes, these are the problems that we wanted to avoid by using the attribute. > To fix this, either we think of a completely different way of doing everything, or we > may need to push some (or a lot of) awareness of TYPE_ARTIFICIALLY_VOLATILIZED() > into tree.c. Well, we want to avoid pushing just as much as we possibly can into tree.c. Really, the volatile is about codegen, and conceptually, is only for codegen. So, let's talk about this some more... I'm hoping that there is a more tactical solution. I'm expecting that we could put in a call to objc_non_volatilized_type in a spot or two and be done with it.
On 29 Nov 2010, at 21:00, Mike Stump wrote: > On Nov 28, 2010, at 6:28 PM, Nicola Pero wrote: >> This patch fixes the 5 @try/@catch ObjC++ testsuite failures on >> Darwin (Apple) with the NeXT runtime. > > Hum... I don't think this is going to work. > >> The code we "inherited" from Apple flagged these artificially >> volatilized types >> using an attribute ("objc_volatilized") but the problem is that >> newer GCCs are >> smarted and consider attributes when building variants of types. >> So the >> additional attribute actually confuses the type system (for >> example, if you start >> with a non-volatilized 'int' type, and build the artificially >> volatilized variant, >> and then you try to get a non-volatile type variant using >> build_qualified_type(), >> you don't get the original type but a new 'int' type which >> preserves your new >> objc_volatilized attribute. > > Ah, but this sounds like a one line fix to remove the attribute in > that one case. Would this work? Downsides? > >> This new 'int' is identical to the normal 'int' >> though, except for the 'objc_volatilized' attribute, but has a >> different >> canonical type. The C++ frontend detects this inconsistency and >> aborts). > >> So, in the end I created a new TYPE_ARTIFICIALLY_VOLATILIZED() flag >> that can >> be used for types (I used the base.side_effects flag which is >> unused for types >> in C/ObjC/C++/ObjC++), and used it to mark our artificially >> volatilized types. > > The problem is, that bit isn't ours to play with, and, no existing > code plays with that bit or understands it and we don't really want > to propagate such system specific details into such an important > bit. Whereas, playing with attributes isn't unreasonable and it > just for such system specific playing. > >> Then the type system works again and everything seems to work with >> the next >> runtime, with the exception of two warnings in try-catch-12.m. >> These occur when >> a real 'volatile' variable is encountered in the same context as >> artificially >> volatilized ones, and if certain conditions are met the two types >> can get confused >> and some warnings for the real volatile variable may not be produced. > > Yes, these are the problems that we wanted to avoid by using the > attribute. > >> To fix this, either we think of a completely different way of >> doing everything, or we >> may need to push some (or a lot of) awareness of >> TYPE_ARTIFICIALLY_VOLATILIZED() >> into tree.c. > > Well, we want to avoid pushing just as much as we possibly can into > tree.c. Really, the volatile is about codegen, and conceptually, is > only for codegen. > > So, let's talk about this some more... I'm hoping that there is a > more tactical solution. I'm expecting that we could put in a call > to objc_non_volatilized_type in a spot or two and be done with it. Is there any way we can shift the code-gen to the lang-specific- gimpifier? (I fear we will need yet another code-gen for m64 NeXT exceptions). Ideally, we'd have only one parse pathway [on the basis that the language is common and we're only changing runtime] and switch the code-gen on runtime... too big a change I guess? Iain
On Nov 29, 2010, at 1:19 PM, IainS wrote: >> So, let's talk about this some more... I'm hoping that there is a more tactical solution. I'm expecting that we could put in a call to objc_non_volatilized_type in a spot or two and be done with it. > > Is there any way we can shift the code-gen to the lang-specific-gimpifier? Unknown to me. With a clean separation between semantic analysis and codegen, the change needs to happen just before code-gen. I'd need middle/gimple type of people to help design a nice solution. The best approach would not to constrain the solution space, but rather say, imagine if C where defined so that int i; in the presence of setjmp/longjmp were required to work. How would you design it? They could then come up with a nice design, and then we'd just implement it. > (I fear we will need yet another code-gen for m64 NeXT exceptions). Nope, that already works. They use eh bits, and under eh, int i; just works. It is only because setjmp doesn't require int i work, that problems arise. > Ideally, we'd have only one parse pathway [on the basis that the language is common and we're only changing runtime] > and switch the code-gen on runtime... Yes. > too big a change I guess? Can't render that judgment without code.
> imagine if C where defined so that int i; in the presence of setjmp/ > longjmp were required to work. I was thinking about the same thing :-) Having a flag for the C compiler that enables that behaviour would be great. There's plenty of C users who use setjmp/longjmp and would like a flag that makes them safe (at the expense of some speed obviously). By the way, as far as I can see, older GCCs (up to 2004) had setjmp_protect() and setjmp_protect_args() that you could use to protect a function against setjmp clobbering. In fact, until 2002, if -traditional was passed, GCC would automatically call setjmp_protect() on all functions that used setjmp(), which is exactly what the flag above would need to do. ;-) Maybe we could reimplement setjmp_protect() ... a vanilla implementation that just marks all local variables and arguments in the function as volatile ? Then we can call it in finish_function() if a -fsafe-setjmp flag is passed and the function contains a call to sejmp ... assuming it works to mark the variables as volatile after the fact, you'd get no spurious warnings since the volatile warnings are produced earlier. Anyway, just brainstorming. Maybe someone who knows more about the internals can chip in and suggest if there is a good way of doing this. ;-) Thanks
On Nov 29, 2010, at 4:32 PM, Nicola Pero wrote: > Maybe we could reimplement setjmp_protect() ... a vanilla implementation that just marks all local variables > and arguments in the function as volatile ? Then we can call it in finish_function() if a -fsafe-setjmp flag is > passed and the function contains a call to sejmp ... assuming it works to mark the variables as volatile after > the fact, you'd get no spurious warnings since the volatile warnings are produced earlier. Debug information is usually written late, and we don't want volatile in there... Also, for performance, we don't really want to mark them volatile, only that at calls (that can throw, I mean, that can longjmp), the values are stable in the variables home, if they are live on the other side of the setjmp.
Index: gcc/tree.h =================================================================== --- gcc/tree.h (revision 167220) +++ gcc/tree.h (working copy) @@ -535,6 +535,9 @@ struct GTY(()) tree_common { FORCED_LABEL in LABEL_DECL + ARTIFICIALLY_VOLATILIZED in + all types + volatile_flag: TREE_THIS_VOLATILE in @@ -1234,6 +1237,13 @@ extern void omp_clause_range_check_faile computed gotos. */ #define FORCED_LABEL(NODE) (LABEL_DECL_CHECK (NODE)->base.side_effects_flag) +/* In a _TYPE, nonzero means that the type was artificially made + volatile for the purpose of code generation when using ObjC/ObjC++ + setjmp/longjmp exceptions (currently only on Darwin). So the _TYPE + is volatile, but we want to disable all warnings about assigning to + it to/from non-volatile variables. */ +#define TYPE_ARTIFICIALLY_VOLATILIZED(NODE) (TYPE_CHECK (NODE)->base.side_effects_flag) + /* Nonzero means this expression is volatile in the C sense: its address should be of type `volatile WHATEVER *'. In other words, the declared item is volatile qualified. Index: gcc/objc/objc-act.c =================================================================== --- gcc/objc/objc-act.c (revision 167231) +++ gcc/objc/objc-act.c (working copy) @@ -2260,49 +2260,135 @@ 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. */ +/* Search for an existing type which is identical to TYPE but has + TYPE_VOLATILE and TYPE_ARTIFICIALLY_VOLATILIZED set. Return + NULL_TREE if no such type was found. */ static tree -objc_build_volatilized_type (tree type) +objc_get_volatilized_type (tree type) { - tree t; + tree t, volatilized_pointee = NULL_TREE; + + /* See if we already have the appropriate variant. */ + if (TYPE_ARTIFICIALLY_VOLATILIZED (type)) + return type; + + /* If we have a pointer, find the volatilized type for the pointee + first. When we build a volatilized type, we volatilize the + pointee as well. */ + if (POINTER_TYPE_P (type)) + { + volatilized_pointee = objc_get_volatilized_type (TREE_TYPE (type)); + + /* If there is none, we never built a volatilized type for the + pointer either (otherwise, there would be one). */ + if (volatilized_pointee == NULL_TREE) + return NULL_TREE; + } /* 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))) + if (!TYPE_ARTIFICIALLY_VOLATILIZED (t)) 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))) + if (!check_qualified_type + (t, type, (TYPE_QUALS (type) | TYPE_QUAL_VOLATILE))) continue; + + /* For pointer types, the pointees (and hence their + TYPE_LANG_SPECIFIC info, if any) must match up. */ + if (POINTER_TYPE_P (t)) + { + if (TREE_TYPE (t) != volatilized_pointee) + continue; + } + + /* Everything matches up! */ + return t; + } + + return NULL_TREE; +} + +/* Search for an existing type which is identical to TYPE but has + TYPE_VOLATILE not set, and TYPE_ARTIFICIALLY_VOLATILIZED not set. */ +static tree +objc_get_non_volatilized_type (tree type) +{ + tree t, non_volatilized_pointee = NULL_TREE; + + /* See if we already have the appropriate variant. */ + if (!TYPE_ARTIFICIALLY_VOLATILIZED (type)) + return type; + + /* At this point, we know that type is artificially volatilized, + hence it was created from an existing, + non-artificially-volatilized type. So we know that the search + will complete. */ + + /* If we have a pointer, find the non-volatilized type for the + pointee first. When we build a volatilized type, we volatilize + the pointee as well. */ + if (POINTER_TYPE_P (type)) + { + non_volatilized_pointee = objc_get_non_volatilized_type (TREE_TYPE (type)); - /* 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))) + /* If there is none, something is wrong. */ + gcc_assert (non_volatilized_pointee); + } + + for (t = TYPE_MAIN_VARIANT (type); t; t = TYPE_NEXT_VARIANT (t)) + { + /* We are looking for a non-volatile type. */ + if (TYPE_ARTIFICIALLY_VOLATILIZED (t)) continue; + if (!check_qualified_type + (t, type, (TYPE_QUALS (type) & ~TYPE_QUAL_VOLATILE))) + continue; + + /* For pointer types, the pointees (and hence their + TYPE_LANG_SPECIFIC info, if any) must match up. */ + if (POINTER_TYPE_P (t)) + { + if (TREE_TYPE (t) != non_volatilized_pointee) + continue; + } + /* Everything matches up! */ return t; } + /* This should be impossible. */ + gcc_unreachable (); +} + +/* 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 and mark the new type with TYPE_ARTIFICIALLY_VOLATILIZED. */ +static tree +objc_build_volatilized_type (tree type) +{ + tree t = objc_get_volatilized_type (type); + + if (t) + 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)); + TYPE_ARTIFICIALLY_VOLATILIZED (t) = 1; + if (TREE_CODE (t) == ARRAY_TYPE) - TREE_TYPE (t) = objc_build_volatilized_type (TREE_TYPE (t)); + TREE_TYPE (t) = objc_build_volatilized_type (TREE_TYPE (type)); + + if (TYPE_HAS_OBJC_INFO (type)) + { + DUP_TYPE_OBJC_INFO (t, type); + TYPE_OBJC_INTERFACE (t) = TYPE_OBJC_INTERFACE (type); + TYPE_OBJC_PROTOCOL_LIST (t) = TYPE_OBJC_PROTOCOL_LIST (type); + } /* Set up the canonical type information. */ if (TYPE_STRUCTURAL_EQUALITY_P (type)) @@ -2703,10 +2789,10 @@ objc_type_quals_match (tree ltyp, tree r { int lquals = TYPE_QUALS (ltyp), rquals = TYPE_QUALS (rtyp); - if (lookup_attribute ("objc_volatilized", TYPE_ATTRIBUTES (ltyp))) + if (TYPE_ARTIFICIALLY_VOLATILIZED (ltyp)) lquals &= ~TYPE_QUAL_VOLATILE; - if (lookup_attribute ("objc_volatilized", TYPE_ATTRIBUTES (rtyp))) + if (TYPE_ARTIFICIALLY_VOLATILIZED (rtyp)) rquals &= ~TYPE_QUAL_VOLATILE; return (lquals == rquals); @@ -2831,14 +2917,14 @@ objc_check_global_decl (tree decl) error ("redeclaration of Objective-C class %qs", IDENTIFIER_POINTER (id)); } -/* Return a non-volatalized version of TYPE. */ - +/* Return a non-artificially-volatilized version of TYPE. This is the + reverse of objc_build_volatilized_type(); if the type is marked as + TYPE_ARTIFICIALLY_VOLATILIZED, it returns a variant without the + flag and which is not volatile. */ 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; + return objc_get_non_volatilized_type (type); } /* Construct a PROTOCOLS-qualified variant of INTERFACE, where Index: gcc/objc/ChangeLog =================================================================== --- gcc/objc/ChangeLog (revision 167231) +++ gcc/objc/ChangeLog (working copy) @@ -1,5 +1,14 @@ 2010-11-29 Nicola Pero <nicola.pero@meta-innovation.com> + * objc-act.c (objc_get_volatilized_type): New. + (objc_get_non_volatilized_type): New. + (objc_build_volatilized_type): Use objc_get_volatilized_type. + Duplicate ObjC type information when building the volatilized + type. Set TYPE_ARTIFICIALLY_VOLATILIZED for the new type. + (objc_non_volatilized_type): Use objc_get_non_volatilized_type. + +2010-11-29 Nicola Pero <nicola.pero@meta-innovation.com> + * objc-act.c (objc_demangle): Return immediately if the string is too short. Detect names that do not need demangling, and return them unchanged. Index: gcc/ChangeLog =================================================================== --- gcc/ChangeLog (revision 167220) +++ gcc/ChangeLog (working copy) @@ -1,3 +1,7 @@ +2010-11-28 Nicola Pero <nicola.pero@meta-innovation.com> + + * tree.h (TYPE_ARTIFICIALLY_VOLATILIZED): New. + 2010-11-27 Jan Hubicka <jh@suse.cz> * dwarf2out.c (dwarf2out_begin_function): Set cold_text_section
This patch fixes the 5 @try/@catch ObjC++ testsuite failures on Darwin (Apple) with the NeXT runtime. The problem is with the artificially volatilized types that are used when setjmp/longjmp ObjC exception handling is being used (this currently happens only on Darwin with the NeXT runtime). The code we "inherited" from Apple flagged these artificially volatilized types using an attribute ("objc_volatilized") but the problem is that newer GCCs are smarted and consider attributes when building variants of types. So the additional attribute actually confuses the type system (for example, if you start with a non-volatilized 'int' type, and build the artificially volatilized variant, and then you try to get a non-volatile type variant using build_qualified_type(), you don't get the original type but a new 'int' type which preserves your new objc_volatilized attribute. This new 'int' is identical to the normal 'int' though, except for the 'objc_volatilized' attribute, but has a different canonical type. The C++ frontend detects this inconsistency and aborts). So, in the end I created a new TYPE_ARTIFICIALLY_VOLATILIZED() flag that can be used for types (I used the base.side_effects flag which is unused for types in C/ObjC/C++/ObjC++), and used it to mark our artificially volatilized types. Then the type system works again and everything seems to work with the next runtime, with the exception of two warnings in try-catch-12.m. These occur when a real 'volatile' variable is encountered in the same context as artificially volatilized ones, and if certain conditions are met the two types can get confused and some warnings for the real volatile variable may not be produced. To fix this, either we think of a completely different way of doing everything, or we may need to push some (or a lot of) awareness of TYPE_ARTIFICIALLY_VOLATILIZED() into tree.c. This should be a separate patch; in the meanwhile I'd suggest we xfail the test and open a PR so we don't forget about the problem. Anyway, all in all we're down from 5 hard compilation failures to 2 missed minor warnings (for unusual cases), which seems a good improvement. Ok to commit ? Thanks In gcc/: 2010-11-29 Nicola Pero <nicola.pero@meta-innovation.com> * tree.h (TYPE_ARTIFICIALLY_VOLATILIZED): New. In gcc/objc/: 2010-11-29 Nicola Pero <nicola.pero@meta-innovation.com> * objc-act.c (objc_get_volatilized_type): New. (objc_get_non_volatilized_type): New. (objc_build_volatilized_type): Use objc_get_volatilized_type. Duplicate ObjC type information when building the volatilized type. Set TYPE_ARTIFICIALLY_VOLATILIZED for the new type. (objc_non_volatilized_type): Use objc_get_non_volatilized_type.