[PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements

[H.J. Lu]

On Wed, Apr 13, 2016 at 11:36 AM, Martin Sebor <msebor@gmail.com> wrote:
> On 04/12/2016 12:17 PM, Jason Merrill wrote:
>>
>> On 04/10/2016 07:14 PM, Martin Sebor wrote:
>>>
>>> The call to build_vla_check() in check_initializer() is to check
>>> only explicitly initialized VLAs.  The latter function may need
>>> to complete the VLA element type and reshape the initializer so
>>> the call cannot be made earlier.  The function returns the runtime
>>> initializer code so the call cannot be made after it returns.
>>
>>
>> But this call isn't checking the initializer; we're looking at the
>> initializer in build_vec_init now.
>
>
> Let me try to explain.  Strings aside, there are three cases each
> of which the patch handles in a different function:
>
> 1) The No Initializer case is handled by calling build_vla_check
>     directly from cp_finish_decl.
>
> 2) The Empty initializer case is handled from check_initializer
>     in the hunk you had pasted upthread.  (It isn't handled in
>     build_vec_init because length_check is false for empty
>     initializers, and because it's too late to check VLA bounds
>     there anyway -- see below.)
>
> 3) In the Non-empty Initializer case, excess initializer elements
>     are checked for in build_vec_init.  VLA bounds are checked from
>     check_initializer same as in (2).
>
> As I mentioned last Friday, emitting the check for the VLA bounds
> in build_vec_init appears to be too late because by then the code
> to allocate the stack has already been emitted.  Maybe there's
> a way to do it, I don't know.  Controlling what piece of code is
> emitted when is something I don't know much about yet.
>
>>> The call to build_vla_check() in cp_finish_decl() is guarded with
>>> !init and made to check VLAs that are not explicitly initialized.
>>> This call could perhaps be moved into check_initializer() though
>>> it doesn't seem like it logically belongs there (since there is
>>> no initializer).
>>
>>
>> check_initializer handles implicit (default-) as well as explicit
>> initialization.
>>
>>> If it were moved there, it seems to me that it
>>> would require more extensive changes to the function than making
>>> it in cp_finish_decl().
>>
>>
>> I don't see that; you ought to be able to move the check_initializer
>> copy down out of the if/else structure and use that for both cases.
>
>
> You're right, it was simpler than I thought.  I was being overly
> conservative in an effort to avoid changing more code than is
> absolutely necessary.
>
> Attached is an updated patch with this simplification. It avoids
> case (1) above.  It also adds an argument to build_vla_check to
> avoid building the size check when it's called from build_vec_init.
>
> I also modified the alternate patch accordingly.  It's attached
> for comparison. I still find it preferable to the first patch.
> It's simpler because it doesn't require the special handling for
> strings and avoids parameterizing build_vla_check so as not to
> build the duplicate check in build_vec_init.
>
> I don't see the benefit in doing the checking in build_vec_init
> and split_constant_init_1 when it can all be done just in
> check_initializer.  I'm sure you have your reasons for going that
> route so I'd appreciate if you could let me know why you think
> that's better.

It caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70652


-- 
H.J.