Re: [PATCH] split tree_type, a.k.a. "tuplifying types"

[Tom de Vries <vries@codesourcery.com>]

On 05/23/2011 05:04 PM, Nathan Froyd wrote:
> On 05/22/2011 02:24 PM, Tom de Vries wrote:
>> Now that struct tree_type does not exist anymore, 'sizeof (struct tree_type)'
>> generates an error in the following assert in fold_checksum_tree:
>> ...
>>   gcc_assert ((sizeof (struct tree_exp) + 5 * sizeof (tree)
>> 	       <= sizeof (struct tree_function_decl))
>> 	      && sizeof (struct tree_type) <= sizeof (struct tree_function_decl));
>> ...
>>
>> This error is triggered with -enable-checking=fold.
> 
> Doh.  Thanks for the report.
> 
> The easy fix is s/tree_type/tree_type_non_common/.  But I don't see why the
> assert has to even care about tree_type; doesn't:
> 
>   gcc_assert ((sizeof (struct tree_exp) + 5 * sizeof (tree)
>               <= sizeof (union tree_node));
> 
> accomplish the same thing?
> 
> -Nathan
> 
> 

I don't know for sure what the assert is trying to check, but I'm guessing it's
trying to check that the memcpys are save. A naive implementation would be:

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 173703)
+++ fold-const.c	(working copy)
@@ -13792,6 +13789,7 @@ recursive_label:
       && DECL_ASSEMBLER_NAME_SET_P (expr))
     {
       /* Allow DECL_ASSEMBLER_NAME to be modified.  */
+      gcc_assert (tree_size (expr) <= sizeof (buf));
       memcpy ((char *) &buf, expr, tree_size (expr));
       SET_DECL_ASSEMBLER_NAME ((tree)&buf, NULL);
       expr = (tree) &buf;
@@ -13805,6 +13803,7 @@ recursive_label:
     {
       /* Allow these fields to be modified.  */
       tree tmp;
+      gcc_assert (tree_size (expr) <= sizeof (buf));
       memcpy ((char *) &buf, expr, tree_size (expr));
       expr = tmp = (tree) &buf;
       TYPE_CONTAINS_PLACEHOLDER_INTERNAL (tmp) = 0;

But that turns it into a runtime check.

On the other hand, I'm not sure the original assert still makes sense. Neither
tcc_type nor tcc_declaration have variable size, so the '5 * sizeof (tree)' does
not seem applicable anymore.

If we want checks cheaper than the naive, but more maintainable than the
current, we would want something like:

+      gcc_assert (tree_class_max_size (tcc_declaration) <= sizeof (buf));
+      gcc_assert (tree_class_max_size (tcc_type) <= sizeof (buf));

We would want those checks moved out of the hot path, and we would need to
implement and maintain tree_class_max_size alongside tree_size and
tree_code_size.  But I'm not sure it's worth the effort.

Thanks,
- Tom