[Bug tree-optimization/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[Bug tree-optimization/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |http://bugs.debian.org/6555
                   |                            |18
   Target Milestone|---                         |4.6.3

[Bug tree-optimization/51994] New: [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

             Bug #: 51994
           Summary: [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to
                    negative bitpos from get_inner_reference
    Classification: Unclassified
           Product: gcc
           Version: 4.6.3
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: tree-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: ubizjak@gmail.com
            Target: alpha-linux-gnu


Created attachment 26457
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26457
preprocessed source

git-1.7.8.3 is miscompiled [1] due to negative bit position returned from
get_inner_reference.

Start with following patch that ICEs for negative positions:

--cut here--
Index: expr.c
===================================================================
--- expr.c      (revision 183510)
+++ expr.c      (working copy)
@@ -6300,6 +6300,9 @@ get_inner_reference (tree exp, HOST_WIDE_INT *pbit
       *poffset = offset;
     }

+  /* Negative bit positions are not allowed.  */
+  gcc_assert (*pbitpos >= 0);
+
   /* We can use BLKmode for a byte-aligned BLKmode bitfield.  */
   if (mode == VOIDmode
       && blkmode_bitfield
--cut here--

Crosscompile attached config.i with -O2 for alpha-linux-gnu target:

Breakpoint 1, fancy_abort (file=0x9b5378
"../../gcc-svn/branches/gcc-4_6-branch/gcc/expr.c", line=6304, 
    function=0x9b6490 "get_inner_reference") at
../../gcc-svn/branches/gcc-4_6-branch/gcc/diagnostic.c:892
892     {
(gdb) up
#1  0x0000000000587234 in get_inner_reference (exp=0x2aaaaf2996e0,
pbitsize=0x7fffffffc1b8, pbitpos=0x7fffffffc1b0, 
    poffset=<value optimized out>, pmode=<value optimized out>,
punsignedp=<value optimized out>, 
    pvolatilep=0x7fffffffc1c4, keep_aligning=1 '\001') at
../../gcc-svn/branches/gcc-4_6-branch/gcc/expr.c:6304
6304      gcc_assert (*pbitpos >= 0);
(gdb) p *pbitpos
$2 = -8
(gdb) 

Negative bit positions should not be allowed. This is what happens with
negative positions:

#17 0x000000000057147b in adjust_address_1 (memref=0x2aaaaf8fd570, mode=QImode,
offset=2305843009213693951, validate=1, 
    adjust=<value optimized out>) at
../../gcc-svn/branches/gcc-4_6-branch/gcc/emit-rtl.c:2033
#18 0x000000000058156d in store_bit_field_1 (str_rtx=0x2aaaaf8fd570, bitsize=8,
bitnum=18446744073709551608, 
    fieldmode=<value optimized out>, value=0x2aaaae770500, fallback_p=<value
optimized out>)
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expmed.c:469
#19 0x00000000005817cf in store_bit_field (str_rtx=0x2aaaaf8f5fc0,
bitsize=46912578215904, bitnum=46912559843392, 
    fieldmode=370, value=0x7) at
../../gcc-svn/branches/gcc-4_6-branch/gcc/expmed.c:838
---Type <return> to continue, or q <return> to quit---
#20 0x000000000059483b in store_field (target=0x2aaaaf8fd570, bitsize=8,
bitpos=-8, mode=QImode, exp=0x2aaaaf29f3e8, 
    type=<value optimized out>, alias_set=0, nontemporal=0 '\000')
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expr.c:6056
#21 0x000000000058921a in expand_assignment (to=0x2aaaaf8ac6c0,
from=0x2aaaaf29f3e8, nontemporal=0 '\000')
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expr.c:4465

Please see frame #19. Kind of funny bitsizes and bitnums. To trigger this
problem on attached config.i, please put a breakpoint on gen_ashldi3 and skip a
couple of triggers, so operand2 is (const_int 61):

Breakpoint 1, gen_ashldi3 (operand0=0x2aaaaf8f5fc0, operand1=0x2aaaaf8f5fe0,
operand2=0x2aaaae770840) at insn-emit.c:429
429     {
(gdb) p debug_rtx (operand2)
(const_int 61 [0x3d])

The compiler falls apart at:

(gdb) up
#20 0x000000000059483b in store_field (target=0x2aaaaf8fd570, bitsize=8,
bitpos=-8, mode=QImode, exp=0x2aaaaf29f3e8, 
    type=<value optimized out>, alias_set=0, nontemporal=0 '\000')
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expr.c:6056
6056          store_bit_field (target, bitsize, bitpos, mode, temp);
(gdb) p bitpos
$14 = -8

However, store_bit_field is declared as:

void
store_bit_field (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
         unsigned HOST_WIDE_INT bitnum, enum machine_mode fieldmode,
         rtx value)

Compilation goes down the drain from here.

The problematic code is located in git_config_rename_section (see also [1]):

19765    output += offset + i;
19766    if (strlen(output) > 0) {
<blanks>
19773     output -= 1;
19774     output[0] = '\t';

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=655518

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-01-25
                 CC|                            |ebotcazou at gcc dot
                   |                            |gnu.org
          Component|tree-optimization           |middle-end
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-01-25 10:56:37 UTC ---
Negative bitpos is fine - Ada uses that quite extensively and with MEM_REFs
this just got more prominent.  get_inner_reference is declared to return
a _signed_ HOST_WIDE_INT bitpos for a reason.

What should happen instead is that store_field needs to adjust the address
to properly point before the bitfield for calling store_bit_field.  Or the
latter needs to take signed arguments for bitpos and do the adjustment
itself.

Does simply changing the store_bit_field[_1] prototype work for you?

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #2 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-01-25 11:15:09 UTC ---
> Negative bitpos is fine - Ada uses that quite extensively and with MEM_REFs
> this just got more prominent.  get_inner_reference is declared to return
> a _signed_ HOST_WIDE_INT bitpos for a reason.

Extensively is a bit of an overstatement, but Ada does use negative offsets.
The recent story about them in build_ref_for_model shows that they can be
problematic though.

> What should happen instead is that store_field needs to adjust the address
> to properly point before the bitfield for calling store_bit_field.  Or the
> latter needs to take signed arguments for bitpos and do the adjustment
> itself.
> 
> Does simply changing the store_bit_field[_1] prototype work for you?

It might also be interesting to find out why we have a negative bitpos here.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #3 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 11:21:34 UTC ---
Created attachment 26459
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26459
Patch to fix function prototypes

To my surprise, attached patch fixes all git failures. However, I have no idea
if changing the prototypes as suggested by Richi is the right way to fix this.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #4 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 11:39:11 UTC ---
(In reply to comment #2)

> > What should happen instead is that store_field needs to adjust the address
> > to properly point before the bitfield for calling store_bit_field.  Or the
> > latter needs to take signed arguments for bitpos and do the adjustment
> > itself.
> > 
> > Does simply changing the store_bit_field[_1] prototype work for you?
> 
> It might also be interesting to find out why we have a negative bitpos here.

gcc passes (&buf - 1) to get_innter_reference, so it returns *pbitpos = -8.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-01-25 12:41:13 UTC ---
(In reply to comment #3)
> Created attachment 26459 [details]
> Patch to fix function prototypes
> 
> To my surprise, attached patch fixes all git failures. However, I have no idea
> if changing the prototypes as suggested by Richi is the right way to fix this.

I think it is certainly obviously correct to do this.  OTOH it may uncover
latent issues (or not pass bootstrap due to signed/unsigned compares or so).
I'd say give it bootstrap & regtest on a fast machine and post this patch.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #6 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-01-25 12:42:14 UTC ---
(In reply to comment #2)
> > Negative bitpos is fine - Ada uses that quite extensively and with MEM_REFs
> > this just got more prominent.  get_inner_reference is declared to return
> > a _signed_ HOST_WIDE_INT bitpos for a reason.
> 
> Extensively is a bit of an overstatement, but Ada does use negative offsets.
> The recent story about them in build_ref_for_model shows that they can be
> problematic though.
> 
> > What should happen instead is that store_field needs to adjust the address
> > to properly point before the bitfield for calling store_bit_field.  Or the
> > latter needs to take signed arguments for bitpos and do the adjustment
> > itself.
> > 
> > Does simply changing the store_bit_field[_1] prototype work for you?
> 
> It might also be interesting to find out why we have a negative bitpos here.

It's as easy as doing

int foo(int *p)
{
  return p[-1];
}

these days.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #7 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 13:19:32 UTC ---
Testcase that crashes on alpha:

--cut here--
extern void abort (void);

char __attribute__((noinline))
test (int a)
{
  char buf[] = "0123456789";
  char *output = buf;

  output += a;
  output -= 1;

  return output[0];
}

int main ()
{
  if (test (2) != '1')
    abort ();

  return 0;
}
--cut here--

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #8 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 13:33:43 UTC ---
And the test in Comment #7 exposed the same problem in extract_bit_field & co.

#19 0x00000000005801f4 in extract_bit_field (str_rtx=0x2aaaae85b760,
bitsize=46912560805760, bitnum=46912559843392, 
    unsignedp=370, packedp=<value optimized out>, target=0x0, mode=QImode,
tmode=QImode)
---Type <return> to continue, or q <return> to quit---
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expmed.c:1697
#20 0x000000000058aa12 in expand_expr_real_1 (exp=0x2aaaae773870,
target=0x2aaaae85b700, tmode=<value optimized out>, 
    modifier=<value optimized out>, alt_rtl=<value optimized out>)
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expr.c:9380

What a can of worms...

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26459|0                           |1
        is obsolete|                            |

--- Comment #9 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 14:28:10 UTC ---
Created attachment 26462
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26462
Patch to fix function prototypes, adds extract_bit_field

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #10 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 14:41:39 UTC ---
(In reply to comment #7)
> Testcase that crashes on alpha:

Actually, the test in comment #7 exposed the problem, but was not 100% correct.

This one is:

--cut here--
#include <string.h>

extern void abort (void);

char __attribute__((noinline))
test (int a)
{
  char buf[16];
  char *output = buf;

  strcpy (&buf[0], "0123456789");

  output += a;
  output -= 1;

  return output[0];
}

int main ()
{
  if (test (2) != '1')
    abort ();

  return 0;
}
--cut here--

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-01-25 14:46:04 UTC ---
Please avoid string.h, then it depends on the C library headers.  Either
provide
your own strcpy prototype like you already do for abort, or use
__builtin_strcpy?

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26462|0                           |1
        is obsolete|                            |

--- Comment #12 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 18:17:25 UTC ---
Created attachment 26466
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26466
WIP gcc-46 patch, includes other expmed.c store/extract bit functions

This patch fixes regression at gcc.dg/pr48335-8.c on x86_64-linux from previous
patch. However, some kind of unterminated loop is now triggered for this test
on x86_64 target:

(gdb) up
#2  0x00000000005b1a3d in extract_split_bit_field (op0=0x7ffff1340a80,
bitsize=16, bitpos=-24, unsignedp=1)
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expmed.c:1996
1996              word = operand_subword_force (op0, offset, GET_MODE (op0));
(gdb) up
#3  0x00000000005b1962 in extract_split_bit_field (op0=0x7ffff1340a80,
bitsize=16, bitpos=-24, unsignedp=1)
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expmed.c:2006
2006          part = extract_fixed_bit_field (word_mode, word,
(gdb) up
#4  0x00000000005b1962 in extract_split_bit_field (op0=0x7ffff1340a80,
bitsize=16, bitpos=-24, unsignedp=1)
    at ../../gcc-svn/branches/gcc-4_6-branch/gcc/expmed.c:2006
2006          part = extract_fixed_bit_field (word_mode, word,

I was not able to fix it... definitelly for someone more experienced in this
area of the compiler.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #13 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 18:39:38 UTC ---
"Fixing" bit position to unsigned in headers is a No-Go. Too many parts of the
compiler depends on unsigned bit positions - we can end with negative subreg
indexes.

Perhpaps the return of get_inner_reference can be adjusted to return equivalent
negative offset expression instead of negative bit position?

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #14 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-25 19:44:03 UTC ---
(In reply to comment #13)

> Perhpaps the return of get_inner_reference can be adjusted to return equivalent
> negative offset expression instead of negative bit position?

Like this:

Index: expr.c
===================================================================
--- expr.c      (revision 183524)
+++ expr.c      (working copy)
@@ -6300,6 +6300,22 @@ get_inner_reference (tree exp, HOST_WIDE_INT *pbit
       *poffset = offset;
     }

+  /* Negative bit positions are not allowed.  */
+  if (*pbitpos < 0)
+    {
+      tree cst = build_int_cst (NULL_TREE, *pbitpos / BITS_PER_UNIT);
+
+      if (*poffset)
+       *poffset = fold_convert (sizetype,
+                                fold_build2 (PLUS_EXPR,
+                                             sizetype,
+                                             *poffset, cst));
+      else
+       *poffset = cst;
+
+      *pbitpos = 0;
+    }
+
   /* We can use BLKmode for a byte-aligned BLKmode bitfield.  */
   if (mode == VOIDmode
       && blkmode_bitfield

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #15 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-01-25 20:20:12 UTC ---
Can't most of the callers of get_inner_reference cope with negative bitpos
though?  If so, perhaps only the caller or two in the expansion which doesn't
should be adjusted.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #16 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-26 07:57:10 UTC ---
(In reply to comment #15)
> Can't most of the callers of get_inner_reference cope with negative bitpos
> though?  If so, perhaps only the caller or two in the expansion which doesn't
> should be adjusted.

I did look a bit for other uses, and negative offsets can result in subregs
with negative index and shifts with negative shift constant (see i.e.
fold-const.c, line 3454). So, I still think that infrastructure doesn't handle
negative bit positions well.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> 2012-01-26 08:02:50 UTC ---
Doing it in get_inner_reference sounds like a risky change though.
E.g. Richard's PR51750 change would need to be adjusted to match it.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #18 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-26 08:20:27 UTC ---
Adding richi to CC.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #19 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-01-26 10:01:23 UTC ---
I agree, all callers of get_inner_reference need to cope with a negative
bitpos.  Those that forward it unchecked to functions that expect an
unsigned bitpos are broken.

Thus I think fixing the prototypes is correct.  If that exposes other
issues we have to fix them.  The issue in extract_split_bit_field
is obviously the same - unsigned prototype and unsigned offset in

  while (bitsdone < bitsize)
    {
      unsigned HOST_WIDE_INT thissize;
      rtx part, word;
      unsigned HOST_WIDE_INT thispos;
      unsigned HOST_WIDE_INT offset;

      offset = (bitpos + bitsdone) / unit;

also

      thispos = (bitpos + bitsdone) % unit;

might not be correct with a negative (bitpos + bitsdone).

extract_fixed_bit_field has the same prototype issue, so eventually we
want to simply account for them in the callers (if there are less).
Only memory operands may have a negative bitpos and those we should be
able to adjust via adjust_address (but by what amount?) to make bitpos
positive.

So you could say already the routines called from the get_inner_reference
callers should do that.

Eric, you should know this area the best - what do you recommend here?
[we could assert in the unsigned bitpos taking functions that the MSB
is not set on bitpos]

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #20 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-01-26 11:00:33 UTC ---
> Eric, you should know this area the best - what do you recommend here?
> [we could assert in the unsigned bitpos taking functions that the MSB
> is not set on bitpos]

I agree that making get_inner_reference artificially return a non-zero poffset
would most certainly be problematic as this would change the semantics.  But
it's also clear that the lower level bit-field manipulation routines aren't
really prepared to deal with negative stuff.  So I think that we shouldn't
change the prototypes of these routines, but instead patch up callers that
forward the values returned by get_inner_reference to these routines.

Adding assertions in these routines could indeed help.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Uros Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26466|0                           |1
        is obsolete|                            |

--- Comment #21 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-26 18:40:24 UTC ---
Created attachment 26474
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26474
Patch that adds asserts to {extract,store}_bit_field

This patch regressed following tests on x86_64-linux (gcc-46):

FAIL: gcc.dg/pr48335-8.c (internal compiler error)
FAIL: gcc.dg/pr48335-8.c (test for excess errors)

This shows that x86_64 is also not immune to negative bitpos problem.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #22 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-26 18:41:57 UTC ---
(In reply to comment #20)

> I agree that making get_inner_reference artificially return a non-zero poffset
> would most certainly be problematic as this would change the semantics.  But
> it's also clear that the lower level bit-field manipulation routines aren't
> really prepared to deal with negative stuff.  So I think that we shouldn't
> change the prototypes of these routines, but instead patch up callers that
> forward the values returned by get_inner_reference to these routines.
> 
> Adding assertions in these routines could indeed help.

I have added these. But.. could you please take the fix to this problem
further?

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #23 from Uros Bizjak <ubizjak at gmail dot com> 2012-01-26 18:51:00 UTC ---
With a crosscompiler to alpha-linux-gnu, we can trigger both problems, one with
preprocessed source, another with the testcase in latest attached patch:

[uros@localhost testalpha]$ ~/gcc-build-alpha-46/gcc/cc1 -O2 -quiet pr51994.c
pr51994.c: In function ‘test’:
pr51994.c:17:3: internal compiler error: in extract_bit_field, at expmed.c:1701
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

[uros@localhost testalpha]$ ~/gcc-build-alpha-46/gcc/cc1 -O2 -quiet config.i
config.c: In function ‘git_config_rename_section’:
config.c:1533:16: internal compiler error: in store_bit_field, at expmed.c:839
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |ebotcazou at gcc dot
                   |gnu.org                     |gnu.org

--- Comment #24 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-01-26 21:01:12 UTC ---
Investigating.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #25 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-01 15:34:37 UTC ---
I've changed my mind: given that we shouldn't have references outside the base
object in valid programs, there must be an offset if the bitpos is negative, so
we can simply add the (rounded) latter to the former.  That's what the callers
would have to do anyway, so we'd better factor it in get_inner_reference.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #26 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-01 15:36:06 UTC ---
Created attachment 26545
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26545
Tentative fix

Uros, does it fix the original issue?

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #28 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-01 15:41:52 UTC ---
(In reply to comment #27)
> (In reply to comment #25)
> > I've changed my mind: given that we shouldn't have references outside the base
> > object in valid programs, there must be an offset if the bitpos is negative, so
> > we can simply add the (rounded) latter to the former.  That's what the callers
> > would have to do anyway, so we'd better factor it in get_inner_reference.
> 
> I'm not sure it's the best thing to do (adjusting 'offset' by
> a BITS_PER_UNIT multiple instead of whatever the caller would like the most).
> Only the callers would know how they want to adjust for negative bitpos.

Oh, and if you do that, please change bitpos to unsigned HOST_WIDE_INT *.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #27 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-01 15:41:15 UTC ---
(In reply to comment #25)
> I've changed my mind: given that we shouldn't have references outside the base
> object in valid programs, there must be an offset if the bitpos is negative, so
> we can simply add the (rounded) latter to the former.  That's what the callers
> would have to do anyway, so we'd better factor it in get_inner_reference.

I'm not sure it's the best thing to do (adjusting 'offset' by
a BITS_PER_UNIT multiple instead of whatever the caller would like the most).
Only the callers would know how they want to adjust for negative bitpos.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26545|0                           |1
        is obsolete|                            |

--- Comment #29 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-01 15:49:14 UTC ---
Created attachment 26547
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=26547
Correct fix

This adds the missing division...

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #30 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-01 15:53:25 UTC ---
> I'm not sure it's the best thing to do (adjusting 'offset' by
> a BITS_PER_UNIT multiple instead of whatever the caller would like the most).
> Only the callers would know how they want to adjust for negative bitpos.

I don't think the callers are prepared for a reference outside the base object,
so there must be a offset.  And I don't see what they would want to do with a
negative bitpos, apart from translating it somehow or other.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #31 from rguenther at suse dot de <rguenther at suse dot de> 2012-02-01 16:00:41 UTC ---
On Wed, 1 Feb 2012, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994
> 
> --- Comment #30 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-01 15:53:25 UTC ---
> > I'm not sure it's the best thing to do (adjusting 'offset' by
> > a BITS_PER_UNIT multiple instead of whatever the caller would like the most).
> > Only the callers would know how they want to adjust for negative bitpos.
> 
> I don't think the callers are prepared for a reference outside the base object,
> so there must be a offset.  And I don't see what they would want to do with a
> negative bitpos, apart from translating it somehow or other.

The base object can be an indirect reference, so yes, there doesn't have
to be an overall positive offset (well, yes, to the _real_ object,
but we don't see that).

Yes, you have to adjust for a negative _bit_ position.  But fact is
the whole constant offset (even if negative) is usually returned via
bitpos.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #32 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-01 16:34:30 UTC ---
> The base object can be an indirect reference, so yes, there doesn't have
> to be an overall positive offset (well, yes, to the _real_ object,
> but we don't see that).

If this is an indirect reference, there is no base object by definition.  So
I'm not sure we should care in get_inner_reference and, in any case, I'm not
sure what to do.  Probably avoid sending MEM_REF to get_inner_reference in this
case,
after all it's clearly not a handled_component_p-like thing.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #33 from Uros Bizjak <ubizjak at gmail dot com> 2012-02-01 18:41:59 UTC ---
(In reply to comment #29)
> Created attachment 26547 [details]
> Correct fix
> 
> This adds the missing division...

This patch fixes both the testcase and original issue. Thanks!

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenther at suse dot de]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #34 from rguenther at suse dot de <rguenther at suse dot de> 2012-02-02 08:56:04 UTC ---
On Wed, 1 Feb 2012, ebotcazou at gcc dot gnu.org wrote:

> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994
> 
> --- Comment #32 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-01 16:34:30 UTC ---
> > The base object can be an indirect reference, so yes, there doesn't have
> > to be an overall positive offset (well, yes, to the _real_ object,
> > but we don't see that).
> 
> If this is an indirect reference, there is no base object by definition.  So
> I'm not sure we should care in get_inner_reference and, in any case, I'm not
> sure what to do.  Probably avoid sending MEM_REF to get_inner_reference in this
> case,
> after all it's clearly not a handled_component_p-like thing.

Well, you can have component refs wrapped around a MEM_REF (or formerly
an INDIRECT_REF).  The only difference now is that the MEM_REF may
have a (negative) constant offset embedded.  Now, only if the MEM_REF
is based on an ADDR_EXPR (and thus a real object) we factor in its
(possibly negative) offset to bitpos.  So, hum - now I don't see
as easily that we can get a negative bitpos from a not
undefined input ... (maybe except for the Ada fat pointer case).

So your patch is probably ok (can you try verifying we don't get
(too much) codegen differences on a bootstrap?)

Richard.

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #35 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-06 12:29:06 UTC ---
> So your patch is probably ok (can you try verifying we don't get
> (too much) codegen differences on a bootstrap?)

There are no differences for a compiler build on Alpha and i586 (4.6 branch).

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #36 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-07 17:21:44 UTC ---
Author: ebotcazou
Date: Tue Feb  7 17:21:36 2012
New Revision: 183974

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183974
Log:
    PR middle-end/51994
    * expr.c (get_inner_reference): If there is an offset, add a negative
    bit position to it (if any).

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/20120207-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/expr.c
    trunk/gcc/testsuite/ChangeLog

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

--- Comment #37 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-07 17:24:32 UTC ---
Author: ebotcazou
Date: Tue Feb  7 17:24:27 2012
New Revision: 183975

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=183975
Log:
    PR middle-end/51994
    * expr.c (get_inner_reference): If there is an offset, add a negative
    bit position to it (if any).

Added:
    branches/gcc-4_6-branch/gcc/testsuite/gcc.c-torture/execute/20120207-1.c
      - copied unchanged from r183974,
trunk/gcc/testsuite/gcc.c-torture/execute/20120207-1.c
Modified:
    branches/gcc-4_6-branch/gcc/ChangeLog
    branches/gcc-4_6-branch/gcc/expr.c
    branches/gcc-4_6-branch/gcc/testsuite/ChangeLog

[Bug middle-end/51994] [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference

[ebotcazou at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51994

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #38 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2012-02-07 17:28:03 UTC ---
Patch applied.