[pph] Macro Validation Correction (issue4425041)

[pph] Macro Validation Correction (issue4425041)

[Lawrence Crowl]

An earlier change reduced the number of bits in cpp_hashnode.directive_index
from 7 to 5, as that was sufficient for indexing the directives.  Tom Tromey
asked for a static check on the size.  This patch adds that check.

Unfortunately, five bits are not sufficient for the alternate use of
cpp_hashnode.directive_index as a named operator index.  So, I have reverted
the number of bits from five back to seven.  As a result, we now have 34 bits
in small fields, and the size of cpp_hashnode will increase from two to three
words on 32-bit systems.  The size on 64-bit systems remains unchanged because
these bits go into an alignment gap.

Index: libcpp/ChangeLog.pph

2011-04-14  Lawrence Crowl  <crowl@google.com>

	* include/cpplib.h (cpp_hashnode):  Use a macro for the number of bits
	in the directive_index bitfield.  Change the number of bits back to 7.
	* directives.c (DIRECTIVE_TABLE):  Add a gcc-build-time check that the
	index of the directive table fits within the number of bits above.
	* init.c (operator_array):  Separate the named operator table into a
	macro.  Use it in operator_array.  Test that the values fit within the
	number of bits above.



Index: libcpp/directives.c
===================================================================
--- libcpp/directives.c	(revision 172457)
+++ libcpp/directives.c	(working copy)
@@ -137,10 +137,7 @@ static void cpp_pop_definition (cpp_read
    pcmcia-cs-3.0.9).  This is no longer important as directive lookup
    is now O(1).  All extensions other than #warning, #include_next,
    and #import are deprecated.  The name is where the extension
-   appears to have come from.
-
-   Make sure the bitfield directive_index in include/cpplib.h is large
-   enough to index the entire table.  */
+   appears to have come from.  */
 
 #define DIRECTIVE_TABLE							\
 D(define,	T_DEFINE = 0,	KANDR,     IN_I)	   /* 270554 */ \
@@ -181,6 +178,13 @@ enum
 };
 #undef D
 
+/* Make sure the bitfield directive_index in include/cpplib.h is large
+   enough to index the entire table.  */
+
+unsigned char too_many_directives_for_bitfield[
+        N_DIRECTIVES <= (1 << CPP_HASHNODE_INDEX_BITS)
+        ? 1 : -1];
+
 #define D(name, t, origin, flags) \
 { do_##name, (const uchar *) #name, \
   sizeof #name - 1, origin, flags },
Index: libcpp/include/cpplib.h
===================================================================
--- libcpp/include/cpplib.h	(revision 172457)
+++ libcpp/include/cpplib.h	(working copy)
@@ -650,12 +650,14 @@ union GTY(()) _cpp_hashnode_value {
   unsigned short GTY ((tag ("NTV_ARGUMENT"))) arg_index;
 };
 
+#define CPP_HASHNODE_INDEX_BITS 7
+
 struct GTY(()) cpp_hashnode {
   struct ht_identifier ident;
   unsigned int is_directive : 1;
-  unsigned int directive_index : 5;	/* If is_directive,
-					   then index into directive table.
-					   Otherwise, a NODE_OPERATOR.  */
+  unsigned int directive_index : CPP_HASHNODE_INDEX_BITS;
+	/* If is_directive, then index into directive table.
+	   Otherwise, a NODE_OPERATOR.  */
   unsigned int used_by_directive : 1;	/* In an active #if, #define etc.  */
   unsigned int expanded_to_text : 1;	/* Produces tokens for parser.  */
   unsigned char rid_code;		/* Rid code - for front ends.  */
Index: libcpp/init.c
===================================================================
--- libcpp/init.c	(revision 172457)
+++ libcpp/init.c	(working copy)
@@ -375,23 +375,34 @@ struct builtin_operator
   const unsigned short value;
 };
 
-#define B(n, t)    { DSC(n), t }
+#define NAMED_OPER_TABLE \
+  B("and",	CPP_AND_AND) \
+  B("and_eq",	CPP_AND_EQ) \
+  B("bitand",	CPP_AND) \
+  B("bitor",	CPP_OR) \
+  B("compl",	CPP_COMPL) \
+  B("not",	CPP_NOT) \
+  B("not_eq",	CPP_NOT_EQ) \
+  B("or",	CPP_OR_OR) \
+  B("or_eq",	CPP_OR_EQ) \
+  B("xor",	CPP_XOR) \
+  B("xor_eq",	CPP_XOR_EQ) \
+
+#define B(n, t)    { DSC(n), t },
 static const struct builtin_operator operator_array[] =
 {
-  B("and",	CPP_AND_AND),
-  B("and_eq",	CPP_AND_EQ),
-  B("bitand",	CPP_AND),
-  B("bitor",	CPP_OR),
-  B("compl",	CPP_COMPL),
-  B("not",	CPP_NOT),
-  B("not_eq",	CPP_NOT_EQ),
-  B("or",	CPP_OR_OR),
-  B("or_eq",	CPP_OR_EQ),
-  B("xor",	CPP_XOR),
-  B("xor_eq",	CPP_XOR_EQ)
+  NAMED_OPER_TABLE
 };
 #undef B
 
+/* Verify that the indicies of the named operators fit within the
+   number of bits available. */
+
+#define B(n, t) unsigned char t ## _too_large_for_bitfield[ \
+		t < (1 << CPP_HASHNODE_INDEX_BITS) ? 1 : -1];
+NAMED_OPER_TABLE
+#undef B
+
 /* Mark the C++ named operators in the hash table.  */
 static void
 mark_named_operators (cpp_reader *pfile, int flags)

--
This patch is available for review at http://codereview.appspot.com/4425041

Re: [pph] Macro Validation Correction (issue4425041)

[Diego Novillo]

On Thu, Apr 14, 2011 at 22:01, Lawrence Crowl <crowl@google.com> wrote:

> Unfortunately, five bits are not sufficient for the alternate use of
> cpp_hashnode.directive_index as a named operator index.  So, I have reverted
> the number of bits from five back to seven.  As a result, we now have 34 bits
> in small fields, and the size of cpp_hashnode will increase from two to three
> words on 32-bit systems.  The size on 64-bit systems remains unchanged because
> these bits go into an alignment gap.

I don't think this is a big issue.  Tom?

> +/* Make sure the bitfield directive_index in include/cpplib.h is large
> +   enough to index the entire table.  */
> +
> +unsigned char too_many_directives_for_bitfield[
> +        N_DIRECTIVES <= (1 << CPP_HASHNODE_INDEX_BITS)
> +        ? 1 : -1];

Heh, I'm not sure what to think of this trick. I think I like it, though.

> +/* Verify that the indicies of the named operators fit within the
> +   number of bits available. */

s/indicies/indices/


OK otherwise.


Diego.

Re: [pph] Macro Validation Correction (issue4425041)

[Tom Tromey]

>>>>> "Diego" == Diego Novillo <dnovillo@google.com> writes:

>> Unfortunately, five bits are not sufficient for the alternate use of
>> cpp_hashnode.directive_index as a named operator index.  So, I have reverted
>> the number of bits from five back to seven.  As a result, we now have 34 bits
>> in small fields, and the size of cpp_hashnode will increase from two to three
>> words on 32-bit systems.  The size on 64-bit systems remains unchanged because
>> these bits go into an alignment gap.

Diego> I don't think this is a big issue.  Tom?

In the past I have pushed back on growing this structure, but that was
because I thought there was a different way to achieve the same result.
In this case I am not so sure.

Tom

Re: [pph] Macro Validation Correction (issue4425041)

[Lawrence Crowl]

On 4/16/11, Diego Novillo <dnovillo@google.com> wrote:
> On Apr 14, 2011 Lawrence Crowl <crowl@google.com> wrote:
> > Unfortunately, five bits are not sufficient for the alternate
> > use of cpp_hashnode.directive_index as a named operator index.
> > So, I have reverted the number of bits from five back to seven.
> > As a result, we now have 34 bits in small fields, and the
> > size of cpp_hashnode will increase from two to three words on
> > 32-bit systems.  The size on 64-bit systems remains unchanged
> > because these bits go into an alignment gap.
>
> I don't think this is a big issue.  Tom?
>
> > +/* Make sure the bitfield directive_index in include/cpplib.h is large
> > +   enough to index the entire table.  */
> > +
> > +unsigned char too_many_directives_for_bitfield[
> > +        N_DIRECTIVES <= (1 << CPP_HASHNODE_INDEX_BITS)
> > +        ? 1 : -1];
>
> Heh, I'm not sure what to think of this trick. I think I like
> it, though.

It is used elsewhere in gcc.  I took that use as permission to
reuse the technique.

> > +/* Verify that the indicies of the named operators fit within the
> > +   number of bits available. */
>
> s/indicies/indices/

In the queue.

-- 
Lawrence Crowl

Re: [pph] Macro Validation Correction (issue4425041)

[Diego Novillo]

On Mon, Apr 18, 2011 at 14:58, Lawrence Crowl <crowl@google.com> wrote:
> On 4/16/11, Diego Novillo <dnovillo@google.com> wrote:
>> On Apr 14, 2011 Lawrence Crowl <crowl@google.com> wrote:
>> > +/* Make sure the bitfield directive_index in include/cpplib.h is large
>> > +   enough to index the entire table.  */
>> > +
>> > +unsigned char too_many_directives_for_bitfield[
>> > +        N_DIRECTIVES <= (1 << CPP_HASHNODE_INDEX_BITS)
>> > +        ? 1 : -1];
>>
>> Heh, I'm not sure what to think of this trick. I think I like
>> it, though.
>
> It is used elsewhere in gcc.  I took that use as permission to
> reuse the technique.

Yeah, it wasn't an objection.  I just find it interesting.  I'm fine
with it, of course.


Diego.

Re: [pph] Macro Validation Correction (issue4425041)

[Hans-Peter Nilsson]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 412 bytes --]

On Sat, 16 Apr 2011, Diego Novillo wrote:
> On Thu, Apr 14, 2011 at 22:01, Lawrence Crowl <crowl@google.com> wrote:
> > +unsigned char too_many_directives_for_bitfield[
> > +        N_DIRECTIVES <= (1 << CPP_HASHNODE_INDEX_BITS)
> > +        ? 1 : -1];
>
> Heh, I'm not sure what to think of this trick. I think I like it, though.

One up: better make it a declaration: prepend with "extern".

brgds, H-P

Re: [pph] Macro Validation Correction (issue4425041)

[Lawrence Crowl]

On 4/22/11, Hans-Peter Nilsson <hp@bitrange.com> wrote:
> On Sat, 16 Apr 2011, Diego Novillo wrote:
>> On Thu, Apr 14, 2011 at 22:01, Lawrence Crowl <crowl@google.com> wrote:
>> > +unsigned char too_many_directives_for_bitfield[
>> > +        N_DIRECTIVES <= (1 << CPP_HASHNODE_INDEX_BITS)
>> > +        ? 1 : -1];
>>
>> Heh, I'm not sure what to think of this trick. I think I like it, though.
>
> One up: better make it a declaration: prepend with "extern".

I will fold that into my next patch.

-- 
Lawrence Crowl