This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Martin Sebor <msebor at gmail dot com>
- Cc: Jason Merrill <jason at redhat dot com>, Gcc Patch List <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 13 Apr 2016 19:35:27 -0700
- Subject: Re: [PATCH] 69517 - [5/6 regression] SEGV on a VLA with excess initializer elements
- Authentication-results: sourceware.org; auth=none
- References: <56DCDB80 dot 90208 at gmail dot com> <56F2DD43 dot 2000206 at redhat dot com> <56F2F2D8 dot 10708 at gmail dot com> <56F40CA1 dot 3060005 at redhat dot com> <56FEFE08 dot 8010207 at gmail dot com> <5702FA46 dot 9020807 at redhat dot com> <5706F5D5 dot 9030204 at gmail dot com> <570ADE43 dot 9080107 at gmail dot com> <570D3BC1 dot 1040608 at redhat dot com> <570E91B1 dot 90602 at gmail dot com>
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.