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

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Sun, Sep 15, 2013 at 06:55:17PM +0200, Bernd Edlinger wrote:
> Hello Richard,
> 
> attached is my second attempt at fixing PR 57748. This time the
> movmisalign path is completely removed and a similar bug in the read
> handling of misaligned structures with a non-BLKmode is fixed
> too. There are several new test cases for the different possible
> failure modes.

I think the third and fourth testcases are undefined as the
description of zero-length arrays extension clearly says the whole
thing only makes sense when used as the last field of the
outermost-aggregate type.  I have not really understood what the third
testcase is supposed to test but I did not try too much.  Instead of
the fourth testcase, you can demonstrate the need for your change in
expand_expr_real_1 by augmenting the original testcase a little like
in attached pr57748-m1.c.

The hunk in expand_expr_real_1 can prove problematic if at any point
we need to pass some other modifier to the expansion of tem.  I'll try
to see if I can come up with a testcase tomorrow.  But perhaps we
never do (and can hope we never will) and then it would be sort of
OKish (note that I cannot approve anything) even though it can
pessimize unaligned access paths (by not using movmisalign_optab even
when perfectly possible - which is always when there is no zero sized
array).  It really just shows how evil non-BLKmode structures with
zero-sized arrays are and how they complicate things.  The expansion
of component_refs is reasonably built around the assumption that we'd
expand the structure in its mode in the most efficient manner and then
chuck the correct part out of it, but here we need to tell the
expansion of the structure to hold itself back because we'll be
looking outside of the structure (as specified by mode).

I'm not sure to what extent the hunk adding tests for bitregion_start
and bitregion_end being zero is connected to this issue.  I do not see
any of the testcases exercising that path.  If it is indeed another
problem, I think it should be submitted (and potentially committed) as
a separate patch, preferably with a testcase.

Having said all that, I think that removing the misalignp path from
expand_assignment altogether is a good idea.  I have verified that
when the expander is now presented with basically the same thing that
4.7 choked on, expand_expr (..., EXPAND_WRITE) can cope with it (see
attached file c.c) and doing that simplifies this complex code path.

Thanks,

Martin

> 
> This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu
> and i686-pc-linux-gnu.
> 
> Additionally I generated eCos and an eCos-application (on ARMv5 using packed
> structures) with an arm-eabi cross compiler, and looked for differences in the
> disassembled code with and without this patch, but there were none.
> 
> OK for trunk?
> 
> Regards
> Bernd. 		 	   		  

> 2013-09-15  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	PR middle-end/57748
> 	* expr.c (expand_assignment): Remove misalignp code path.
> 	Check for bitregion in offset arithmetic.
> 	(expand_expr_real_1): Use EXAND_MEMORY on base object.
> 
> testsuite:
> 
> 	PR middle-end/57748
> 	* gcc.dg/torture/pr57748-1.c: New test.
> 	* gcc.dg/torture/pr57748-2.c: New test.
> 	* gcc.dg/torture/pr57748-3.c: New test.
> 	* gcc.dg/torture/pr57748-3a.c: New test.
> 	* gcc.dg/torture/pr57748-4.c: New test.
> 	* gcc.dg/torture/pr57748-4a.c: New test.
> 


/* PR middle-end/57748 */
/* { dg-do run } */

#include <stdlib.h>

extern void abort (void);

typedef long long V
  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));

typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));

struct __attribute__((packed)) T { char c; P s; };

void __attribute__((noinline, noclone))
check (struct T *t)
{
  if (t->s.b[0][0] != 3 || t->s.b[0][1] != 4)
    abort ();
}

void __attribute__((noinline, noclone))
check_1 (P *p)
{
  if (p->b[0][0] != 3 || p->b[0][1] != 4)
    abort ();
}

int __attribute__((noinline, noclone))
get_i (void)
{
  return 0;
}

void __attribute__((noinline, noclone))
foo (P *p)
{
  V a = { 3, 4 };
  int i = get_i();
  p->b[i] = a;
}

int
main ()
{
  struct T *t = (struct T *) malloc (128);

  foo (&t->s);
  check (t);
  check_1 (&t->s);

  return 0;
}

#include <stdlib.h>

extern void abort (void);

typedef long long V
  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));

typedef struct S { V a; } P __attribute__((aligned (1)));

struct __attribute__((packed)) T { char c; P s; };

void __attribute__((noinline, noclone))
foo (P *p)
{
  V a = { 3, 4 };
  p->a = a;
}

int
main ()
{
  struct T *t = (struct T *) malloc (128);
  foo (&t->s);
  return 0;
}