Re: prevent creation of invalid static constructors from gimplifier

[Olivier Hainque <hainque at adacore dot com>]

Hello Ian,

Thanks for your detailed and constructive review.

Ian Lance Taylor wrote:
> >    prevent creation of improper static vars in gimplify_init_constructor
> >    http://gcc.gnu.org/ml/gcc-patches/2006-05/msg00450.html
> 
> The issue I see with this patch is that we are going to wind up
> walking the entire constructor twice: once in categorize_ctor_elements
> and once in initializer_constant_valid_p.  This could be a bit painful
> for very large initializers, which is a case which we already handle
> poorly.

 Agreed. I felt concerned about this point as well. More details below.

> As far as I can see, the reason that your patch works, the reason that
> initializer_constant_valid_p returns false, is that TREE_CONSTANT is
> not set for the constructor, even though it is a constant.  If
> TREE_CONSTANT were set, initializer_constant_valid_p would walk the
> elements and report that they were all constant.

 Right.
 
> I haven't looked into why TREE_CONSTANT is not set, but I assume it
> has something to do with the Ada frontend.

 Right. utils2.c:gnat_build_constructor is where this happens.

> In any case it suggests a simple fix: change
> categorize_ctor_elements_1 so that when it sees a constructor for
> which TREE_CONSTANT is not set, it increments nc_elts.

 I had thought of something along these lines but this unfortunately
 doesn't catch all the cases. The basic point is that, as the name
 conveys, categorize_ctor_elements_1 states facts about the constructor
 _elements_ and not about the outer constructor itself.

 The testcase in the submission message features a !TREE_CONSTANT outer
 constructor which happens to have only TREE_CONSTANT elements, including
 a nested constructor.

 Whether this should be handled by categorize_ctor_elements_1 was not clear
 to me, since the implementation matches what the name conveys and we seemed
 to be looking for a more general property. This is what prompted my question
 at http://gcc.gnu.org/ml/gcc/2006-03/msg00729.html.

 This is also what eventually prompted an attempt at the
 gimplify_init_constructor level. I first thought of just testing for
 TREE_CONSTANT (ctor) at the points where the patch checks for
 initializer_constant_valid_p, then thought that it might possibly
 cause other troubles and that initializer_constant_valid_p had to
 be honored when deciding to go static in any case.

 I'll be happy to follow any direction that seems most suitable :)

> Please make sure that there is a test case in the Ada testsuite.

 Sure.

 Thanks for your feeback,

 Olivier