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

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

Hi,

On Tue, 8 Oct 2013 10:01:37, Eric Botcazou wrote:
>
>> OK, what do you think of it now?
>
> My take on this is that the Proper Fix(tm) has been posted by Martin:
> http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00082.html
> IMO it's a no-brainer, modulo the ABI concern. Everything else is more or
> less clever stuff to paper over this original compute_record_mode bug.
>
> We are lying to the expander by pretending that the object has V2DImode since
> it's larger and thus cannot be manipulated in this mode; everything is clearly
> downhill from there. If we don't want to properly fix the bug then let's put
> a hack in the expander, possibly using EXPAND_MEMORY, but it should trigger
> _only_ for structures with zero-sized arrays and non-BLKmode and be preceded
> by a big ??? comment explaining why it is deemed necessary.
>
> --
> Eric Botcazou

I agree, that assigning a non-BLKmode to structures with zero-sized arrays
should be considered a bug.

And it should be checked that there is only ONE zero-sized array.
And it should be checked that it is only allowed at the end of the structure.

Otherwise we have something like a union instead of a structure,
which will break the code in tree-ssa-alias.c !

And again, this is not only a problem of structures with zero-sized
arrays at the end. Remember my previous example code:
On ARM (or anything with STRICT_ALIGNMENT) this union has the
same problems:
 
/* PR middle-end/57748 */
/* arm-eabi-gcc -mcpu=cortex-a9 -O3 */
#include <stdlib.h>
 
union  x
{
  short a[2];
  char x[4];
} __attribute__((packed, aligned(4))) ;
typedef volatile union  x *s;
 
void __attribute__((noinline, noclone))
check (void)
{
  s xx=(s)(0x80000002);
  /* although volatile xx->x[3] reads 4 bytes here */
  if (xx->x[3] != 3)
    abort ();
}
 
void __attribute__((noinline, noclone))
foo (void)
{
  s xx=(s)(0x80000002);
  xx->x[3] = 3;
}
 
int
main ()
{
  foo ();
  check ();
  return 0;
}


This union has a UINT32 mode, look at compute_record_mode!
Because of this example I still think that the expander should know what
we intend to do with the base object.


Regards
Bernd.