An earlier change reduced the number of bits in cpp_hashnode.directive_index from 7 to 5, as ...
13 years, 11 months ago
(2011-04-15 02:01:39 UTC)
#1
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
On Thu, Apr 14, 2011 at 22:01, Lawrence Crowl <crowl@google.com> wrote: > Unfortunately, five bits ...
13 years, 11 months ago
(2011-04-16 16:09:04 UTC)
#2
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.
>>>>> "Diego" == Diego Novillo <dnovillo@google.com> writes: >> Unfortunately, five bits are not sufficient for ...
13 years, 11 months ago
(2011-04-18 13:18:56 UTC)
#3
>>>>> "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
On 4/16/11, Diego Novillo <dnovillo@google.com> wrote: > On Apr 14, 2011 Lawrence Crowl <crowl@google.com> wrote: ...
13 years, 11 months ago
(2011-04-18 18:58:57 UTC)
#4
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
On Mon, Apr 18, 2011 at 14:58, Lawrence Crowl <crowl@google.com> wrote: > On 4/16/11, Diego ...
13 years, 11 months ago
(2011-04-18 19:00:56 UTC)
#5
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.
On Sat, 16 Apr 2011, Diego Novillo wrote: > On Thu, Apr 14, 2011 at ...
13 years, 11 months ago
(2011-04-23 00:14:05 UTC)
#6
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
On 4/22/11, Hans-Peter Nilsson <hp@bitrange.com> wrote: > On Sat, 16 Apr 2011, Diego Novillo wrote: ...
13 years, 11 months ago
(2011-04-25 18:20:59 UTC)
#7
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
Issue 4425041: [pph] Macro Validation Correction
(Closed)
Created 13 years, 11 months ago by Lawrence Crowl
Modified 12 years, 10 months ago
Reviewers: Diego Novillo, tromey_redhat.com, hp_bitrange.com
Base URL: svn+ssh://gcc.gnu.org/svn/gcc/branches/pph/
Comments: 0