[PING] [PATCH, PR 57748] Check for out of bounds access, Part 2

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

Hi,

ping...

this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html

Note: it does, as it is, _not_ depend on the keep_aligning patch.
And it would fix some really nasty wrong code generation issues.


Thanks
Bernd.

> Date: Tue, 19 Nov 2013 15:39:39 +0100
>
> Hello,
>
>
> this is a minor update to my previous version of this patch, (using a boolean expand_reference,
> instead of adding a new expand_modifier enum value):
>
> I forgot to pass down the expand_reference value at the second expand_expr call inside the
> case VIEW_CONVERT_EXPR. Sorry for the inconvenience.
>
>
>
> @@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
> }
>
> if (!op0)
> - op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
> + op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
> + NULL, expand_reference);
>
> /* If the input and output modes are both the same, we are done. */
> if (mode == GET_MODE (op0))
>
>
> Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
>
> Ok for trunk?
>
>
> Thanks
> Bernd.
>
>
>> Date: Thu, 7 Nov 2013 13:58:55 +0100
>>
>> oops - this time with attachments...
>>
>>
>>> Hi,
>>>
>>> On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
>>>>
>>>> On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
>>>> <bernd.edlinger@hotmail.de> wrote:
>>>>> Hi,
>>>>>
>>>>>> Eh ... even
>>>>>>
>>>>>> register struct { int i; int a[0]; } asm ("ebx");
>>>>>>
>>>>>> works. Also with int a[1] but not with a[2]. So just handling trailing
>>>>>> arrays makes this case regress as well.
>>>>>>
>>>>>> Now I'd call this use questionable ... but we've likely supported that
>>>>>> for decades and cannot change that now.
>>>>>>
>>>>>> Back to fixing everything in expand.
>>>>>>
>>>>>> Richard.
>>>>>>
>>>>>
>>>>> Ok, finally you asked for it.
>>>>>
>>>>> Here is my previous version of that patch again.
>>>>>
>>>>> I have now added a new value "EXPAND_REFERENCE" to the expand_modifier
>>>>> enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
>>>>> constant values.
>>>>>
>>>>> I have done the same modification to VIEW_CONVERT_EXPR too, because
>>>>> this is a possible inner reference, itself. It is however inherently hard to
>>>>> test around this code.
>>>>>
>>>>> To understand this patch it is good to know what type of object the
>>>>> return value "tem" of get_inner_reference can be.
>>>>>
>>>>> From the program logic at get_inner_reference it is clear that the
>>>>> return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
>>>>> ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
>>>>> be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
>>>>> further restricted because exp is gimplified.
>>>>>
>>>>> Usually the result will be a MEM_REF or a SSA_NAME of the memory where
>>>>> the structure is to be found.
>>>>>
>>>>> When you look at where EXPAND_MEMORY is handled you see it is special-cased
>>>>> in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
>>>>> ARRAY_RANGE_REF.
>>>>>
>>>>> At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
>>>>> same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
>>>>> If it is an unaligned memory, we just return the unaligned reference.
>>>>>
>>>>> This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case,
>>>>> because it is only a problem for STRICT_ALIGNMENT targets, and even there it will
>>>>> certainly be really hard to find test cases that exercise this code.
>>>>>
>>>>> In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
>>>>> we do not have to touch the handling of the outer modifier. However we pass
>>>>> EXPAND_REFERENCE to the inner object, which should not be a recursive
>>>>> use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
>>>>>
>>>>> TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
>>>>> EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
>>>>>
>>>>>
>>>>> Boot-strapped and regression-tested on x86_64-linux-gnu
>>>>> OK for trunk?
>>>>
>>>> You point to a weak spot in expansion - that it handles creating
>>>> the base MEM to offset with handled components by recursing
>>>> into the case that handles bare MEM_REFs. This makes the
>>>> bare MEM_REF handling code somewhat awkward (it's the
>>>> one to assign mem-attrs which are later adjusted for example).
>>>>
>>>> Maybe a better appropach than adding yet another expand
>>>> modifier would be to split out the "base MEM" expansion part
>>>> out of the bare MEM_REF handling code so we can call that
>>>> instead of recursing.
>>>>
>>>> In this light - instead of a new expand modifier don't you want
>>>> an actual flag that specifies we are coming from a call that
>>>> wants to expand a base? That is, allow EXPAND_SUM
>>>> but with the recursion flag set?
>>>>
>>>
>>> I think you are right. After some thought, I start to like that idea.
>>>
>>> This way we have at least much more flexibility, how to handle the inner
>>> references correctly, and if I change only the interfaces of expand_expr_real/real_1
>>> that will not be used at too many places, either.
>>>
>>>> Finally I think the recursion into the VIEW_CONVERT_EXPR case
>>>> is only there because of the keep_aligning flag of get_inner_reference
>>>> which should be obsolete now that we properly handle its effects
>>>> in get_object_alignment. So you wouldn't need to adjust this path
>>>> if we finally can get rid of that.
>>>>
>>>
>>> I proposed a patch for that, but did not hear from you:
>>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html
>>>
>>> nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner
>>> reference, the code there should be kept in sync with the normal_inner_ref,
>>> and MEM_REF, IMHO.
>>>
>>> Attached, my updated patch for PR57748, Part 2.
>>>
>>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
>>>
>>> Ok for trunk?
>>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>>> What do others think?
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Thanks
>>>>> Bernd.