Bug 58970 - [4.7 Regression] internal compiler error: in get_bit_range, at expr.c:4562
Summary: [4.7 Regression] internal compiler error: in get_bit_range, at expr.c:4562
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.8.2
: P2 normal
Target Milestone: 4.7.4
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-02 10:16 UTC by Jan Smets
Modified: 2014-04-23 12:39 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.6.3, 4.8.3, 4.9.0
Known to fail: 4.8.2
Last reconfirmed: 2013-11-02 00:00:00


Attachments
For a possible patch. (469 bytes, patch)
2013-11-03 19:14 UTC, Bernd Edlinger
Details | Diff
gcc49-pr58970.patch (902 bytes, patch)
2013-11-04 09:30 UTC, Jakub Jelinek
Details | Diff
Another (better) attempt at fixing this ICE. (746 bytes, patch)
2013-11-05 19:35 UTC, Bernd Edlinger
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Smets 2013-11-02 10:16:08 UTC
/tmp/minimal9.i: In function 'function':
/tmp/minimal9.i:15:28: internal compiler error: in get_bit_range, at expr.c:4562
     pInfo->mode[x].member1 = 0;
                            ^
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.



struct info
{
    struct {
        unsigned char member1 : 1;
        unsigned char member2;
    } mode[1];
};

extern void call(void);

void function(int x, struct info *pInfo)
{
    if (x != -1)
        call();
    pInfo->mode[x].member1 = 0;
    if (pInfo->mode[x].member2 == (((x) == 1) ? 2 : 1))
    {
        call();
    }
}
Comment 1 Jan Smets 2013-11-02 10:17:49 UTC
And compile with -O1 or -O2. Reproducible on x86 and mips.
Comment 2 Marek Polacek 2013-11-02 12:44:59 UTC
Confirmed in all active branches.
Comment 3 Marek Polacek 2013-11-02 13:38:21 UTC
struct S
{
  struct
  {
    int b:1;
  } mode[1];
};

void
foo (int x, struct S *s)
{
  if (x == -1)
    s->mode[x].b = 0;
}
Comment 4 Bernd Edlinger 2013-11-03 19:14:49 UTC
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.
Comment 5 Jan Smets 2013-11-04 00:54:03 UTC
Seems to work. Thanks!
Comment 6 Jakub Jelinek 2013-11-04 08:39:44 UTC
(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.
Comment 7 Bernd Edlinger 2013-11-04 08:58:00 UTC
(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.
Comment 8 Jakub Jelinek 2013-11-04 09:29:43 UTC
(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.
Comment 9 Jakub Jelinek 2013-11-04 09:30:40 UTC
Created attachment 31147 [details]
gcc49-pr58970.patch

Untested fix.
Comment 10 Bernd Edlinger 2013-11-04 09:52:26 UTC
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;
     }
Comment 11 Jakub Jelinek 2013-11-04 09:59:49 UTC
(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.
Comment 12 Bernd Edlinger 2013-11-04 10:05:49 UTC
(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.
Comment 13 Jakub Jelinek 2013-11-04 10:10:13 UTC
(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...
Comment 14 Bernd Edlinger 2013-11-04 10:30:35 UTC
(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?
Comment 15 Bernd Edlinger 2013-11-04 11:05:50 UTC
(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?
Comment 16 Jakub Jelinek 2013-11-04 11:28:05 UTC
(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?
Comment 17 Bernd Edlinger 2013-11-04 11:44:42 UTC
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...
Comment 18 Bernd Edlinger 2013-11-04 12:08:03 UTC
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
Comment 19 Jakub Jelinek 2013-11-04 12:13:51 UTC
(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.
Comment 20 Richard Biener 2013-11-04 14:26:07 UTC
Eric declared negative bitpos to be a nono that the expander is not handling
well.  Thus, Eric, please chime in here.
Comment 21 Jakub Jelinek 2013-11-04 16:00:03 UTC
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?
Comment 22 Eric Botcazou 2013-11-04 17:53:10 UTC
> 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...
Comment 23 Bernd Edlinger 2013-11-05 08:26:44 UTC
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;
Comment 24 Bernd Edlinger 2013-11-05 19:35:51 UTC
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
Comment 25 Jakub Jelinek 2013-11-05 19:41:12 UTC
(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.
Comment 26 Bernd Edlinger 2013-11-06 07:28:22 UTC
(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.
Comment 27 Jakub Jelinek 2013-11-06 07:48:53 UTC
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
Comment 28 Jan Smets 2013-11-06 14:09:42 UTC
Can this be backported to 4.8 please.
Thanks
Comment 29 Jakub Jelinek 2013-11-11 07:57:13 UTC
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
Comment 30 Jan Smets 2013-11-11 09:41:02 UTC
Thanks!
Comment 31 Jan Smets 2014-04-23 12:39:44 UTC
Fixed in 4.8 and newer.