Add a STATIC_ASSERT on sizeof (struct cp_token)

On Fri, 2016-07-22 at 13:06 +0200, Jakub Jelinek wrote:
> On Fri, Jul 22, 2016 at 12:44:07PM +0200, Marek Polacek wrote:
> > --- gcc/gcc/cp/parser.h
> > +++ gcc/gcc/cp/parser.h
> > @@ -46,7 +46,7 @@ struct GTY (()) cp_token {
> >       Otherwise, this value is RID_MAX.  */
> >    ENUM_BITFIELD (rid) keyword : 8;
> >    /* Token flags.  */
> > -  unsigned char flags;
> > +  unsigned short flags;
> >    /* True if this token is from a context where it is implicitly
> > extern "C" */
> >    BOOL_BITFIELD implicit_extern_c : 1;
> >    /* True if an error has already been reported for this token,
> > such as a
> 
> I'm afraid this is really bad.
> Right now, there are 8 and 8 bit bitfields, then 8-bit char, 3
> individual
> bits, 5 unused bits and 32-bit int, nicely packed into 64-bit word
> before a
> union with pointer members, and the C++ FE lexes everything first, so
> there
> are possibly millions of tokens in memory.
> Can't you just make it unsigned int flags : 11; instead?  Or instead
> reshuffle the cpplib.h flags?  E.g. I don't see the C++ FE to use the
> NO_EXPAND flag, so moving it to the upper byte of the short and
> moving the
> new flag to its bit?  Perhaps that is even better for now

Would something like this be appropriate, if it bootstraps?

gcc/cp/ChangeLog:
	* parser.h (struct cp_token): Add a STATIC_ASSERT on the
	size of the struct.
---
 gcc/cp/parser.h | 9 +++++++++
 1 file changed, 9 insertions(+)

On Fri, Jul 22, 2016 at 10:33:50AM -0400, David Malcolm wrote:
> gcc/cp/ChangeLog:
> 	* parser.h (struct cp_token): Add a STATIC_ASSERT on the
> 	size of the struct.
> ---
>  gcc/cp/parser.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/gcc/cp/parser.h b/gcc/cp/parser.h
> index ccbace9..8c1de57 100644
> --- a/gcc/cp/parser.h
> +++ b/gcc/cp/parser.h
> @@ -71,6 +71,15 @@ struct GTY (()) cp_token {
>  	       "|| (%1.type == CPP_DECLTYPE)"))) u;
>  };
>  
> +/* The C++ frontend lexes everything first, and keeps the tokens
> +   in memory, so there are possibly millions of tokens in memory.
> +   Ensure that we don't accidentally grow the structure.  */
> +STATIC_ASSERT (sizeof (cp_token) ==
> +	       (2 // "type" and "keyword"
> +		+ 1 // "flags"
> +		+ 1 // bitfields
> +		+ 4 // location_t
> +		+ sizeof (void *))); // union

Doesn't that assume way too much on the host data layout?
This can be compiled with weirdo system compilers or on weirdo hosts,
I bet we don't really support non-8-bit char hosts, but still this might
break somewhere...

The formatting is wrong BTW, == shouldn't appear at the end of line.

	Jakub

On Fri, Jul 22, 2016 at 4:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jul 22, 2016 at 10:33:50AM -0400, David Malcolm wrote:
>> gcc/cp/ChangeLog:
>>       * parser.h (struct cp_token): Add a STATIC_ASSERT on the
>>       size of the struct.
>> ---
>>  gcc/cp/parser.h | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/gcc/cp/parser.h b/gcc/cp/parser.h
>> index ccbace9..8c1de57 100644
>> --- a/gcc/cp/parser.h
>> +++ b/gcc/cp/parser.h
>> @@ -71,6 +71,15 @@ struct GTY (()) cp_token {
>>              "|| (%1.type == CPP_DECLTYPE)"))) u;
>>  };
>>
>> +/* The C++ frontend lexes everything first, and keeps the tokens
>> +   in memory, so there are possibly millions of tokens in memory.
>> +   Ensure that we don't accidentally grow the structure.  */
>> +STATIC_ASSERT (sizeof (cp_token) ==
>> +            (2 // "type" and "keyword"
>> +             + 1 // "flags"
>> +             + 1 // bitfields
>> +             + 4 // location_t
>> +             + sizeof (void *))); // union
>
> Doesn't that assume way too much on the host data layout?
> This can be compiled with weirdo system compilers or on weirdo hosts,
> I bet we don't really support non-8-bit char hosts, but still this might
> break somewhere...
>
> The formatting is wrong BTW, == shouldn't appear at the end of line.

Maybe restrict it to __GNUC__ and __x86_64__ or so.

Richard.

>         Jakub