This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Tue, Oct 22, 2013 at 12:25 PM, Bernd Edlinger
<bernd.edlinger@hotmail.de> wrote:
> Hi,
>
>> On Tue, 8 Oct 2013 22:50:21, Eric Botcazou wrote:
>>>
>>>> I agree, that assigning a non-BLKmode to structures with zero-sized arrays
>>>> should be considered a bug.
>>>
>>> Fine, then let's apply Martin's patch, on mainline at least.
>>>
>>
>> That would definitely be a good move. Maybe someone should approve it?
>>
>>> But this testcase is invalid on STRICT_ALIGNMENT platforms: xx is pointer to a
>>> type with 4-byte alignment so its value must be a multiple of 4.
>>
>> Then you probably win. But I still have some doubts.
>>
>> I had to use this silly alignment/pack(4) to circumvent this statement
>> in compute_record_mode:
>>
>>   /* If structure's known alignment is less than what the scalar
>>      mode would need, and it matters, then stick with BLKmode.  */
>>   if (TYPE_MODE (type) != BLKmode
>>       && STRICT_ALIGNMENT
>>       && ! (TYPE_ALIGN (type)>= BIGGEST_ALIGNMENT
>>             || TYPE_ALIGN (type)>= GET_MODE_ALIGNMENT (TYPE_MODE (type))))
>>     {
>>       /* If this is the only reason this type is BLKmode, then
>>          don't force containing types to be BLKmode.  */
>>       TYPE_NO_FORCE_BLK (type) = 1;
>>       SET_TYPE_MODE (type, BLKmode);
>>     }
>>
>> But there are at least two targets where STRICT_ALIGNMENT = 0
>> and SLOW_UNALIGNED_ACCESS != 0: rs6000 and alpha.
>>
>> This example with a byte-aligned structure will on one of these targets
>> likely execute this code path in  expand_expr_real_1/case MEM_REF:
>>
>>             else if (SLOW_UNALIGNED_ACCESS (mode, align))
>>               temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
>>                                         0, TYPE_UNSIGNED (TREE_TYPE (exp)),
>>                                         (modifier == EXPAND_STACK_PARM
>>                                          ? NULL_RTX : target),
>>                                         mode, mode);
>>
>> This looks wrong, but unfortunately I cannot test on these targets...
>>
>
> Hmm, well,
>
> the condition that would be necessary to execute that code path
> would be
>
> STRICT_ALIGNMENT = 0
> and SLOW_UNALIGNED_ACCESS != 0 for any integer mode.
>
> The only target that is close to hit this "bug" is rs6000:
>
> #define STRICT_ALIGNMENT 0
>
> #define SLOW_UNALIGNED_ACCESS(MODE, ALIGN)                              \
>   (STRICT_ALIGNMENT                                                     \
>    || (((MODE) == SFmode || (MODE) == DFmode || (MODE) == TFmode        \
>         || (MODE) == SDmode || (MODE) == DDmode || (MODE) == TDmode)    \
>        && (ALIGN) < 32)                                                 \
>    || (VECTOR_MODE_P ((MODE)) && (((int)(ALIGN)) < VECTOR_ALIGN (MODE))))
>
> but, luckily this is 0 for all integer modes.
>
> So I am now convinced, there won't be any valid example with unions that
> executes this code path.
>
> Therefore I updated Martin's previous patch, to meet Eric's request:
> That is to only handle zero-sized arrays at the end of the structure.

That looks backwards.  Why should

struct { V i; V j[0]; }

have a different mode than

struct { V j[0]; V i; }

?  And why should the same issue not exist for

struct { V a[1]; }

which is also "flexible enough" that we support accesses beyond it
via a pointer?  That struct also has V2DImode.  And this is all
because

      /* If this field is the whole struct, remember its mode so
         that, say, we can put a double in a class into a DF
         register instead of forcing it to live in the stack.  */
      if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)))
        mode = DECL_MODE (field);

which is IMHO ok.

> Boot-strapped and regression-tested on x86_64-linux-gnu.
>
> Ok for trunk?

Well, I'm not so sure.  And if so then I'd prefer martins original
patch, rejecting all zero-sized fields.  But then only if you
consider it a "real" field.

Of course I blame those that added the zero-sized array extension
for all this mess ... maybe we can reduce it by rejecting
zero-sized arrays that are not at the end of a structure - which means
we can demote them to flexible arrays with a NULL TYPE_SIZE
which would completely side-step this issue in stor-layout.c.

Richard.

> Regards
> Bernd.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]