Issue 4425041: [pph] Macro Validation Correction (Closed)
|
Created:
13 years ago by Lawrence Crowl Modified:
11 years, 11 months ago Reviewers:
tromey, Diego Novillo, hp CC:
gcc-patches_gcc.gnu.org Base URL:
svn+ssh://gcc.gnu.org/svn/gcc/branches/pph/ Visibility:
Public. |
Patch Set 1 #
MessagesTotal messages: 7
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
>>>>> "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
Sign in to reply to this message.
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
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
