Bug 58970

And compile with -O1 or -O2. Reproducible on x86 and mips.

Confirmed in all active branches.

struct S
{
  struct
  {
    int b:1;
  } mode[1];
};

void
foo (int x, struct S *s)
{
  if (x == -1)
    s->mode[x].b = 0;
}

Created attachment 31145 [details]
For a possible patch.

This patch is assuming that a statement like "s->mode[-1].b = 0;"
is basically invoking undefined behaviour, so it should not matter
if we have correct bitregion_start/end values or just set them to zero
in this case.

Seems to work. Thanks!

(In reply to Bernd Edlinger from comment #4)
> Created attachment 31145 [details]
> For a possible patch.
> 
> This patch is assuming that a statement like "s->mode[-1].b = 0;"
> is basically invoking undefined behaviour, so it should not matter
> if we have correct bitregion_start/end values or just set them to zero
> in this case.

That doesn't look safe, negative rbitpos is not necessarily undefined behavior.
Can't you get the same with say
struct S { unsigned char s : 1; };

...
void function(struct S *p)
{
  p[-1].s = 0;
}

Apparently get_inner_reference only gives negative bitpos if offset is NULL_TREE, otherwise it adjusts offset such that the bitpos is positive.
I wonder if get_bit_range shouldn't do the same thing if it detects *bitpos
is negative and *offset is NULL_TREE before doing the bitoffset > *bitpos
comparison.  It might be as easy as replacing the
gcc_assert (*offset != NULL_TREE);
with if (*offset == NULL_TREE) *offset = size_int (0);
Of course, the comment would need to be adjusted.

(In reply to Jakub Jelinek from comment #6)
> That doesn't look safe, negative rbitpos is not necessarily undefined
> behavior.
> Can't you get the same with say
> struct S { unsigned char s : 1; };
> 
> ...
> void function(struct S *p)
> {
>   p[-1].s = 0;
> }

no. I checked that. This is folded quite differently:

  MEM[(struct S *)p_1(D) + -1B].s = 0;

and there is no ICE in this case.

> Apparently get_inner_reference only gives negative bitpos if offset is
> NULL_TREE, otherwise it adjusts offset such that the bitpos is positive.
> I wonder if get_bit_range shouldn't do the same thing if it detects *bitpos
> is negative and *offset is NULL_TREE before doing the bitoffset > *bitpos
> comparison.  It might be as easy as replacing the
> gcc_assert (*offset != NULL_TREE);
> with if (*offset == NULL_TREE) *offset = size_int (0);
> Of course, the comment would need to be adjusted.

That would work. But if all these negative bit-pos are due to
invalid code as I'd curently expect, we could also just stop generating
code for this type of access, to reduce code size.

What I'm kind of worried here, is that bitpos is signed int,
and bitregion_start/end is unsigned int, and later in the expmed.c
all that stuff is passed as unsigned int bitnum/bitsize.

Therefore, it would make sense to avoid all negative bitpos in
get_inner_reference, but probably only if there exist _valid_ code
with negative bitpos.

(In reply to Bernd Edlinger from comment #7)
> (In reply to Jakub Jelinek from comment #6)
> > That doesn't look safe, negative rbitpos is not necessarily undefined
> > behavior.
> > Can't you get the same with say
> > struct S { unsigned char s : 1; };
> > 
> > ...
> > void function(struct S *p)
> > {
> >   p[-1].s = 0;
> > }
> 
> no. I checked that. This is folded quite differently:
> 
>   MEM[(struct S *)p_1(D) + -1B].s = 0;
> 
> and there is no ICE in this case.

But this is really nothing the expansion code should rely on to detect if the code is undefined behavior or not.

Created attachment 31147 [details]
gcc49-pr58970.patch

Untested fix.

but this should'nt be neccessary then?

   if (bitoffset > *bitpos)
     {
       HOST_WIDE_INT adjust = bitoffset - *bitpos;
-
       gcc_assert ((adjust % BITS_PER_UNIT) == 0);
-      gcc_assert (*offset != NULL_TREE);
 
       *bitpos += adjust;
-      *offset
-	= size_binop (MINUS_EXPR, *offset, size_int (adjust / BITS_PER_UNIT));
+      if (*offset == NULL_TREE)
+	*offset = size_int (-adjust / BITS_PER_UNIT);
+      else
+	*offset
+	  = size_binop (MINUS_EXPR, *offset, size_int (adjust / BITS_PER_UNIT));
       *bitstart = 0;
     }

(In reply to Bernd Edlinger from comment #10)
> but this should'nt be neccessary then?
> 
>    if (bitoffset > *bitpos)
>      {
>        HOST_WIDE_INT adjust = bitoffset - *bitpos;
> -
>        gcc_assert ((adjust % BITS_PER_UNIT) == 0);
> -      gcc_assert (*offset != NULL_TREE);
>  
>        *bitpos += adjust;
> -      *offset
> -	= size_binop (MINUS_EXPR, *offset, size_int (adjust / BITS_PER_UNIT));
> +      if (*offset == NULL_TREE)
> +	*offset = size_int (-adjust / BITS_PER_UNIT);
> +      else
> +	*offset
> +	  = size_binop (MINUS_EXPR, *offset, size_int (adjust / BITS_PER_UNIT));
>        *bitstart = 0;
>      }

Can you prove it isn't necessary?  I mean, *bitpos could still be smaller than bitoffset, even when not negative, say with some packed+aligned structures containing bitfields or similar.  NULL_TREE *offset is the same thing as zero at that spot, just perhaps less efficient in that case, so I think if we can't prove it will not be hit, it is easy to handle it.

(In reply to Jakub Jelinek from comment #11)
> (In reply to Bernd Edlinger from comment #10)
> > but this should'nt be neccessary then?
> > 
> >    if (bitoffset > *bitpos)
> >      {
> >        HOST_WIDE_INT adjust = bitoffset - *bitpos;
> > -
> >        gcc_assert ((adjust % BITS_PER_UNIT) == 0);
> > -      gcc_assert (*offset != NULL_TREE);
> >  
> >        *bitpos += adjust;
> > -      *offset
> > -	= size_binop (MINUS_EXPR, *offset, size_int (adjust / BITS_PER_UNIT));
> > +      if (*offset == NULL_TREE)
> > +	*offset = size_int (-adjust / BITS_PER_UNIT);
> > +      else
> > +	*offset
> > +	  = size_binop (MINUS_EXPR, *offset, size_int (adjust / BITS_PER_UNIT));
> >        *bitstart = 0;
> >      }
> 
> Can you prove it isn't necessary?  I mean, *bitpos could still be smaller
> than bitoffset, even when not negative, say with some packed+aligned
> structures containing bitfields or similar.  NULL_TREE *offset is the same
> thing as zero at that spot, just perhaps less efficient in that case, so I
> think if we can't prove it will not be hit, it is easy to handle it.

hmm, my english...

I meant the change here is not necessary, because after the
if (*bitpos < 0) {...},
*offset can no longer be NULL, and I'd leave the assertion untouched.

(In reply to Bernd Edlinger from comment #12)
> I meant the change here is not necessary, because after the
> if (*bitpos < 0) {...},
> *offset can no longer be NULL, and I'd leave the assertion untouched.

Sure, if *bitpos was initially negative, then *offset won't be NULL there.
But what I mean, are you sure that non-negative *bitpos will never be smaller than bitoffset if *offset is NULL?  Of course not on this testcase...

(In reply to Jakub Jelinek from comment #13)
> (In reply to Bernd Edlinger from comment #12)
> > I meant the change here is not necessary, because after the
> > if (*bitpos < 0) {...},
> > *offset can no longer be NULL, and I'd leave the assertion untouched.
> 
> Sure, if *bitpos was initially negative, then *offset won't be NULL there.
> But what I mean, are you sure that non-negative *bitpos will never be
> smaller than bitoffset if *offset is NULL?  Of course not on this testcase...

If *bitpos is initially negative, I can proove that *offset is initially NULL.

The second statement cannot be prooved,
because after *bitpos &= ... *bitpos is between 0..7.

For instance 

struct S
{
  struct T
  {
    int a:8;
    int b:1;
  } mode[1];
};

Consider "p->mode[-1].b = 0", I'd expect bitoffset=8, less than *bitpos=0.

Right?

(In reply to Bernd Edlinger from comment #14)
> (In reply to Jakub Jelinek from comment #13)
> > (In reply to Bernd Edlinger from comment #12)
> > > I meant the change here is not necessary, because after the
> > > if (*bitpos < 0) {...},
> > > *offset can no longer be NULL, and I'd leave the assertion untouched.
> > 
> > Sure, if *bitpos was initially negative, then *offset won't be NULL there.
> > But what I mean, are you sure that non-negative *bitpos will never be
> > smaller than bitoffset if *offset is NULL?  Of course not on this testcase...
> 
> If *bitpos is initially negative, I can proove that *offset is initially
> NULL.

However we can be sure (to assert), that if offset is initially NULL,
and *bitpos is initially >= 0, then
the bit offset of the bitfield representative must be >= 0 too.
because otherwiese, the bitfield would start at offset < 0 and end
at an offset > 0. Which is not possible.

Therefore bitoffset <= *bitpos if *bitpos is initially >= 0 and *offset is
initially == NULL. QED?

(In reply to Bernd Edlinger from comment #15)
> (In reply to Bernd Edlinger from comment #14)
> > (In reply to Jakub Jelinek from comment #13)
> > > (In reply to Bernd Edlinger from comment #12)
> > > > I meant the change here is not necessary, because after the
> > > > if (*bitpos < 0) {...},
> > > > *offset can no longer be NULL, and I'd leave the assertion untouched.
> > > 
> > > Sure, if *bitpos was initially negative, then *offset won't be NULL there.
> > > But what I mean, are you sure that non-negative *bitpos will never be
> > > smaller than bitoffset if *offset is NULL?  Of course not on this testcase...
> > 
> > If *bitpos is initially negative, I can proove that *offset is initially
> > NULL.
> 
> However we can be sure (to assert), that if offset is initially NULL,
> and *bitpos is initially >= 0, then
> the bit offset of the bitfield representative must be >= 0 too.
> because otherwiese, the bitfield would start at offset < 0 and end
> at an offset > 0. Which is not possible.

Why is it not possible?
struct __attribute__((packed, aligned (2))) T { int a : 31; int b : 1; };
void
foo (char *p)
{
  ((struct T *)(p - 2))->b = 1;
}
Here, *bitpos could be (at least in theory) 15, while bitoffset 31 and *offset
could still be NULL_TREE, because the bitpos fits in signed HWI.
Again, perhaps it is right now optimized into a MEM_REF and get_bit_range won't see that, but can you rule that out just because of that?

struct T {
  unsigned char b : 8;
  unsigned char s : 1;
};

struct S {
  char x;
  struct T t[1];
};
 
void function(int x, struct S *p)
{
  if (x == -1)
    p->t[x].s = 0;
}

another test case. so this is possible...

Well, how about this version?
Does'nt it look like a much smaller change?

--- expr.c.jj	2013-10-31 14:57:05.000000000 +0100
+++ expr.c	2013-11-04 12:51:55.013931114 +0100
@@ -4582,7 +4582,8 @@ get_bit_range (unsigned HOST_WIDE_INT *b
       HOST_WIDE_INT adjust = bitoffset - *bitpos;
 
       gcc_assert ((adjust % BITS_PER_UNIT) == 0);
-      gcc_assert (*offset != NULL_TREE);
+      if (*offset == NULL_TREE)
+	*offset = size_zero_node;
 
       *bitpos += adjust;
       *offset

(In reply to Bernd Edlinger from comment #18)
> Well, how about this version?
> Does'nt it look like a much smaller change?
> 
> --- expr.c.jj	2013-10-31 14:57:05.000000000 +0100
> +++ expr.c	2013-11-04 12:51:55.013931114 +0100
> @@ -4582,7 +4582,8 @@ get_bit_range (unsigned HOST_WIDE_INT *b
>        HOST_WIDE_INT adjust = bitoffset - *bitpos;
>  
>        gcc_assert ((adjust % BITS_PER_UNIT) == 0);
> -      gcc_assert (*offset != NULL_TREE);
> +      if (*offset == NULL_TREE)
> +	*offset = size_zero_node;
>  
>        *bitpos += adjust;
>        *offset

But you then have undefined behavior if *bitpos is HOST_WIDE_INT_MIN.  Or could for very small other *bitpos and not really small bitoffset, etc.  And, while you save a few characters on the line, it means it is more expensive at runtime.

Eric declared negative bitpos to be a nono that the expander is not handling
well.  Thus, Eric, please chime in here.

If expansion has issues with that, then supposedly the
+  if (*bitpos < 0)
+    {
+      gcc_assert (*offset == NULL_TREE);
+      *offset = size_int (*bitpos >> (BITS_PER_UNIT == 8
+                                     ? 3 : exact_log2 (BITS_PER_UNIT)));
+      *bitpos &= BITS_PER_UNIT - 1;
+    }
hunk with s/\*//g could be moved to the get_bit_range caller, right above the call.  But, do we have testcases that fail now?

> Eric declared negative bitpos to be a nono that the expander is not handling
> well.  Thus, Eric, please chime in here.

Well, that's a basic sanity requirement, all the routines in expmed.c dealing with bitfields use unsigned HOST_WIDE_INT so, yes, better avoid negative values unless you really know what you're doing...

hmm...

all examples I can see, where bitpos is negative,
or less than the representative's bitoffset with offset=NULL,
are just blandtly invalid.

The only valid example would be

struct S { unsigned char s : 1; };
void function(struct S *p)
{
  p[-1].s = 0;
}

However this one is gimplified to

  D.1590 = p + -1;
  D.1590->s = 0;

no ARRAY_REFs at all, never.

Handling all that negative bitoffsets seems to be
a bit too much complication just to generate
valid code for invalid C.

At least returning negative bitoffset could be easily
avoided in get_inner_reference:

--- expr.c.jj	2013-10-31 14:57:05.000000000 +0100
+++ expr.c	2013-11-05 09:24:39.821624212 +0100
@@ -6708,7 +6708,7 @@
       tem = tem.sext (TYPE_PRECISION (sizetype));
       tem = tem.lshift (BITS_PER_UNIT == 8 ? 3 : exact_log2 (BITS_PER_UNIT));
       tem += bit_offset;
-      if (tem.fits_shwi ())
+      if (tem.fits_shwi () && !tem.is_negative ())
 	{
 	  *pbitpos = tem.to_shwi ();
 	  *poffset = offset = NULL_TREE;

Created attachment 31169 [details]
Another (better) attempt at fixing this ICE.

This avoids any negative bitpos from get_inner_reference.
These negative bitpos values are just _very_ dangerous all the
way down to expmed.c

(In reply to Bernd Edlinger from comment #24)
> Created attachment 31169 [details]
> Another (better) attempt at fixing this ICE.
> 
> This avoids any negative bitpos from get_inner_reference.
> These negative bitpos values are just _very_ dangerous all the
> way down to expmed.c

I disagree that it is better, you are forgetting get_inner_reference has other > 20 callers outside of expansion and there is no reason why negative bitpos would be a problem in those cases.

(In reply to Jakub Jelinek from comment #25)
> (In reply to Bernd Edlinger from comment #24)
> > Created attachment 31169 [details]
> > Another (better) attempt at fixing this ICE.
> > 
> > This avoids any negative bitpos from get_inner_reference.
> > These negative bitpos values are just _very_ dangerous all the
> > way down to expmed.c
> 
> I disagree that it is better, you are forgetting get_inner_reference has
> other > 20 callers outside of expansion and there is no reason why negative
> bitpos would be a problem in those cases.

Please, give me one example of _valid_ C where bitpos is negative.

Author: jakub
Date: Wed Nov  6 07:48:50 2013
New Revision: 204444

URL: http://gcc.gnu.org/viewcvs?rev=204444&root=gcc&view=rev
Log:
	PR middle-end/58970
	* expr.c (get_bit_range): Handle *offset == NULL_TREE.
	(expand_assignment): If *bitpos is negative, set *offset
	and adjust *bitpos, so that it is not negative.

	* gcc.c-torture/compile/pr58970.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/compile/pr58970-1.c
    trunk/gcc/testsuite/gcc.c-torture/compile/pr58970-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expr.c
    trunk/gcc/testsuite/ChangeLog

Can this be backported to 4.8 please.
Thanks

Author: jakub
Date: Mon Nov 11 07:57:11 2013
New Revision: 204663

URL: http://gcc.gnu.org/viewcvs?rev=204663&root=gcc&view=rev
Log:
	Backported from mainline
	2013-11-06  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/58970
	* expr.c (get_bit_range): Handle *offset == NULL_TREE.
	(expand_assignment): If *bitpos is negative, set *offset
	and adjust *bitpos, so that it is not negative.

	* gcc.c-torture/compile/pr58970-1.c: New test.
	* gcc.c-torture/compile/pr58970-2.c: New test.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/gcc.c-torture/compile/pr58970-1.c
    branches/gcc-4_8-branch/gcc/testsuite/gcc.c-torture/compile/pr58970-2.c
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/expr.c
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog

Thanks!

Fixed in 4.8 and newer.