Bug 51994 - [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_inner_reference
Summary: [4.6/4.7 Regression] git-1.7.8.3 miscompiled due to negative bitpos from get_...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.6.3
: P2 normal
Target Milestone: 4.6.3
Assignee: Eric Botcazou
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2012-01-25 10:26 UTC by Uroš Bizjak
Modified: 2012-02-07 17:28 UTC (History)
3 users (show)

See Also:
Host:
Target: alpha-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-01-25 00:00:00


Attachments
preprocessed source (94.44 KB, text/plain)
2012-01-25 10:26 UTC, Uroš Bizjak
Details
Patch to fix function prototypes (426 bytes, patch)
2012-01-25 11:21 UTC, Uroš Bizjak
Details | Diff
Patch to fix function prototypes, adds extract_bit_field (528 bytes, patch)
2012-01-25 14:28 UTC, Uroš Bizjak
Details | Diff
WIP gcc-46 patch, includes other expmed.c store/extract bit functions (1.34 KB, patch)
2012-01-25 18:17 UTC, Uroš Bizjak
Details | Diff
Patch that adds asserts to {extract,store}_bit_field (603 bytes, text/plain)
2012-01-26 18:40 UTC, Uroš Bizjak
Details
Tentative fix (395 bytes, patch)
2012-02-01 15:36 UTC, Eric Botcazou
Details | Diff
Correct fix (443 bytes, patch)
2012-02-01 15:49 UTC, Eric Botcazou
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Uroš Bizjak 2012-01-25 10:26:32 UTC
Created attachment 26457 [details]
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
Comment 1 Richard Biener 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?
Comment 2 Eric Botcazou 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.
Comment 3 Uroš Bizjak 2012-01-25 11:21:34 UTC
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.
Comment 4 Uroš Bizjak 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.
Comment 5 Richard Biener 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.
Comment 6 Richard Biener 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.
Comment 7 Uroš Bizjak 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--
Comment 8 Uroš Bizjak 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...
Comment 9 Uroš Bizjak 2012-01-25 14:28:10 UTC
Created attachment 26462 [details]
Patch to fix function prototypes, adds extract_bit_field
Comment 10 Uroš Bizjak 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--
Comment 11 Jakub Jelinek 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?
Comment 12 Uroš Bizjak 2012-01-25 18:17:25 UTC
Created attachment 26466 [details]
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.
Comment 13 Uroš Bizjak 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?
Comment 14 Uroš Bizjak 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
Comment 15 Jakub Jelinek 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.
Comment 16 Uroš Bizjak 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.
Comment 17 Jakub Jelinek 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.
Comment 18 Uroš Bizjak 2012-01-26 08:20:27 UTC
Adding richi to CC.
Comment 19 Richard Biener 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]
Comment 20 Eric Botcazou 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.
Comment 21 Uroš Bizjak 2012-01-26 18:40:24 UTC
Created attachment 26474 [details]
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.
Comment 22 Uroš Bizjak 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?
Comment 23 Uroš Bizjak 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.
Comment 24 Eric Botcazou 2012-01-26 21:01:12 UTC
Investigating.
Comment 25 Eric Botcazou 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.
Comment 26 Eric Botcazou 2012-02-01 15:36:06 UTC
Created attachment 26545 [details]
Tentative fix

Uros, does it fix the original issue?
Comment 27 Richard Biener 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.
Comment 28 Richard Biener 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 *.
Comment 29 Eric Botcazou 2012-02-01 15:49:14 UTC
Created attachment 26547 [details]
Correct fix

This adds the missing division...
Comment 30 Eric Botcazou 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.
Comment 31 rguenther@suse.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.
Comment 32 Eric Botcazou 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.
Comment 33 Uroš Bizjak 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!
Comment 34 rguenther@suse.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.
Comment 35 Eric Botcazou 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).
Comment 36 Eric Botcazou 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
Comment 37 Eric Botcazou 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
Comment 38 Eric Botcazou 2012-02-07 17:28:03 UTC
Patch applied.