Re: Bug in Objective-C compiler found

["Zack Weinberg" <zackw at stanford dot edu>]

On Tue, Feb 20, 2001 at 02:14:32PM +0000, Nicola Pero wrote:
> Hi - 
> 
> the Objective-C compiler can't currently compile protocol stuff - see for
> example
> 
> testsuite/objc/bycopy-1
> testsuite/objc/bycopy-2
> testsuite/objc/np-1
> testsuite/objc/np-2
> 
> All these four tests are failing (they compile fine under gcc-2.95.2).

Also objc/protocol.m.

> The compiler seems to have been broken on 7 September 2000 by zack, with
> the change 1.152 to gcc/c-decl.c.

Hm, why haven't I been being chewed out by the regression tester for
the last six months?  ... because those tests were added later, duh.

> Reverting a tiny amount of his changes, as follows, seem to fix the
> problems:
> 
> Index: c-decl.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/c-decl.c,v
> retrieving revision 1.207.2.1
> diff -u -r1.207.2.1 c-decl.c
> --- c-decl.c	2001/02/16 13:02:14	1.207.2.1
> +++ c-decl.c	2001/02/20 13:59:45
> @@ -3912,7 +3912,7 @@
>        if (id == ridpointers[(int) RID_CHAR])
>  	explicit_char = 1;
>  
> -      if (TREE_CODE (id) == IDENTIFIER_NODE && C_IS_RESERVED_WORD (id))
> +      if (TREE_CODE (id) == IDENTIFIER_NODE)// && C_IS_RESERVED_WORD (id))
>  	{
>  	  enum rid i = C_RID_CODE (id);
>  	  if (i <= RID_LAST_MODIFIER)

[N.B. this is in grokdeclarator]

This is probably not safe.  IDENTIFIER_NODEs without
C_IS_RESERVED_WORD set cannot be trusted to have a meaningful
C_RID_CODE field.  If it can be proved that the declspecs argument to
grokdeclarator never contains an IDENTIFIER_NODE which is not a
reserved word, then it's safe; otherwise we need another solution.

Objective C has six keywords which are only reserved in a particular
context.  To express this, the objc front end toggles their
C_IS_RESERVED_WORD bit when it enters and leaves that context.  See
(forget,remember)_protocol_qualifiers in objc-act.c.  It seems that
the IDENTIFIER_NODEs for those keywords are winding up on the
declspecs list for processing by grokdeclarator, _outside_ the lexical
context where they are reserved - but they still need to be
interpreted as keywords.

I have an alternate fix in which the objc front end maintains separate
IDENTIFIER_NODEs for these keywords in their reserved capacity, and
swaps those in when it enters the appropriate context.  Then we don't
have to fiddle their bits, and when grokdeclarator sees them they will
be treated as keywords.  Patch is appended.

This does not fix protocol.m; that's a separate bug.  The mess with
build_tmp_decl/hack_method_prototype/encode_method_prototype inside
generate_protocols is leaving us with a bogus value for
current_function_decl.  That leads to a bogus DECL_CONTEXT for the
method descriptor table, and we crash in finish_decl trying to add to
the statement tree for a nonexistent function.  I don't believe I can
fix that one.

I would like:

- an opinion from C front end experts on whether true non-keywords
  ever appear in declspecs.
- a review of the appended patch, which I am bootstrapping now.

zw
	* stringpool.c (set_identifier): New function.
	* tree.h: Prototype it.
	
	* c-parse.in: Kill D_YES. If compiled for objc, call
	save_and_forget_protocol_qualifiers from init_reswords.
	* objc/objc-act.c (remember_protocol_qualifiers,
	forget_protocol_qualifiers): Don't diddle C_IS_RESERVED_WORD.
	Swap out the non-keyword IDENTIFIER_NODEs for keyword ones, or
	vice versa.
	(save_and_forget_protocol_qualifiers): New function.
	* c-lex.h: Prototype save_and_forget_protocol_qualifiers.

===================================================================
Index: stringpool.c
--- stringpool.c	2000/11/22 00:18:18	1.3
+++ stringpool.c	2001/02/20 18:54:01
@@ -340,6 +340,25 @@ maybe_get_identifier (text)
   return NULL_TREE;
 }
 
+/* Look up an identifier with the name TEXT, replace its identifier
+   node with NODE, and return the old identifier node.  This is used
+   by languages which need to enable and disable keywords based on
+   context; e.g. see remember_protocol_qualifiers in objc/objc-act.c.  */
+tree
+set_identifier (text, node)
+     const char *text;
+     tree node;
+{
+  struct str_header *str;
+  tree old;
+  size_t length = strlen (text);
+
+  str = alloc_string (text, length, INSERT);
+  old = str->data;	/* might be null */
+  str->data = node;
+  return old;
+}
+
 /* Report some basic statistics about the string pool.  */
 
 void
===================================================================
Index: tree.h
--- tree.h	2001/02/16 12:46:17	1.225
+++ tree.h	2001/02/20 18:54:06
@@ -1915,6 +1915,13 @@ extern tree get_identifier		PARAMS ((con
 
 extern tree maybe_get_identifier	PARAMS ((const char *));
 
+/* Look up an identifier with the name TEXT, replace its identifier
+   node with NODE, and return the old identifier node.  This is used
+   by languages which need to enable and disable keywords based on
+   context; e.g. see remember_protocol_qualifiers in objc/objc-act.c.  */
+
+extern tree set_identifier		PARAMS ((const char *, tree));
+
 /* Construct various types of nodes.  */
 
 #define build_int_2(LO,HI)  \
===================================================================
Index: c-lex.h
--- c-lex.h	2000/12/04 17:14:16	1.19
+++ c-lex.h	2001/02/20 18:54:00
@@ -27,6 +27,7 @@ extern void position_after_white_space P
 extern int c_lex PARAMS ((tree *));
 extern const char *init_c_lex PARAMS ((const char *));
 
+extern void save_and_forget_protocol_qualifiers PARAMS ((void));
 extern void forget_protocol_qualifiers PARAMS ((void));
 extern void remember_protocol_qualifiers PARAMS ((void));
 extern tree is_class_name PARAMS ((tree));
===================================================================
Index: c-parse.in
--- c-parse.in	2001/02/04 22:43:57	1.82
+++ c-parse.in	2001/02/20 18:54:00
@@ -2878,7 +2878,6 @@ struct resword
 #define D_EXT	0x04	/* GCC extension */
 #define D_EXT89	0x08	/* GCC extension incorporated in C99 */
 #define D_OBJC	0x10	/* Objective C only */
-#define D_YES	0x20	/* always starts disabled */
 
 static const struct resword reswords[] =
 {
@@ -2971,12 +2970,12 @@ ifobjc
   { "@public",		RID_AT_PUBLIC,		D_OBJC },
   { "@selector",	RID_AT_SELECTOR,	D_OBJC },
   { "id",		RID_ID,			D_OBJC },
-  { "bycopy",		RID_BYCOPY,		D_OBJC|D_YES },
-  { "byref",		RID_BYREF,		D_OBJC|D_YES },
-  { "in",		RID_IN,			D_OBJC|D_YES },
-  { "inout",		RID_INOUT,		D_OBJC|D_YES },
-  { "oneway",		RID_ONEWAY,		D_OBJC|D_YES },
-  { "out",		RID_OUT,		D_OBJC|D_YES },
+  { "bycopy",		RID_BYCOPY,		D_OBJC },
+  { "byref",		RID_BYREF,		D_OBJC },
+  { "in",		RID_IN,			D_OBJC },
+  { "inout",		RID_INOUT,		D_OBJC },
+  { "oneway",		RID_ONEWAY,		D_OBJC },
+  { "out",		RID_OUT,		D_OBJC },
 end ifobjc
 };
 #define N_reswords (sizeof reswords / sizeof (struct resword))
@@ -3138,14 +3137,12 @@ init_reswords ()
 
       id = get_identifier (reswords[i].word);
       C_RID_CODE (id) = reswords[i].rid;
+      C_IS_RESERVED_WORD (id) = 1;
       ridpointers [(int) reswords[i].rid] = id;
-
-      /* Objective C does tricky things with enabling and disabling 
-	 keywords.  So these we must not elide in the test above, but
-	 wait and not mark them reserved now.  */
-      if (! (reswords[i].disable & D_YES))
-	C_IS_RESERVED_WORD (id) = 1;
     }
+ifobjc
+  save_and_forget_protocol_qualifiers ();
+end ifobjc
 }
 
 const char *
===================================================================
Index: objc/objc-act.c
--- objc/objc-act.c	2001/02/16 09:15:19	1.67
+++ objc/objc-act.c	2001/02/20 18:54:08
@@ -8590,25 +8590,45 @@ lookup_objc_ivar (id)
     return 0;
 }
 
-/* Parser callbacks.  */
+/* Parser callbacks.
+   Some ObjC keywords are reserved only in a particular context:
+   in out inout bycopy byref oneway.
+   We have to save and restore the IDENTIFIER_NODEs that describe
+   them as keywords, when appropriate.  */
+
+#define N_PQ 6
+static tree saved_pq[N_PQ];
+static tree saved_not_pq[N_PQ];
+static const char *const pq_strings[N_PQ] = {
+  "bycopy", "byref", "in", "inout", "oneway", "out"
+};
+
+void
+save_and_forget_protocol_qualifiers ()
+{
+  int i;
+  for (i = 0; i < N_PQ; i++)
+    saved_pq[i] = set_identifier (pq_strings[i], NULL_TREE);
+
+  ggc_add_tree_root (saved_pq, N_PQ);
+  ggc_add_tree_root (saved_not_pq, N_PQ);
+}
+
 void
 forget_protocol_qualifiers ()
 {
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_IN]) = 0;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_OUT]) = 0;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_INOUT]) = 0;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_BYCOPY]) = 0;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_BYREF]) = 0;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_ONEWAY]) = 0;
+  int i;
+  for (i = 0; i < N_PQ; i++)
+    {
+      set_identifier (pq_strings[i], saved_not_pq[i]);
+      saved_not_pq[i] = NULL_TREE;
+    }
 }
 
 void
 remember_protocol_qualifiers ()
 {
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_IN]) = 1;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_OUT]) = 1;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_INOUT]) = 1;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_BYCOPY]) = 1;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_BYREF]) = 1;
-  C_IS_RESERVED_WORD (ridpointers[(int) RID_ONEWAY]) = 1;
+  int i;
+  for (i = 0; i < N_PQ; i++)
+    saved_not_pq[i] = set_identifier (pq_strings[i], saved_pq[i]);
 }