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
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?
> 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.
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.
(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.
(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.
(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.
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--
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...
Created attachment 26462 [details] Patch to fix function prototypes, adds extract_bit_field
(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--
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?
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.
"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?
(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
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.
(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.
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.
Adding richi to CC.
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]
> 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.
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.
(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?
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.
Investigating.
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.
Created attachment 26545 [details] Tentative fix Uros, does it fix the original issue?
(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.
(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 *.
Created attachment 26547 [details] Correct fix This adds the missing division...
> 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.
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.
> 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.
(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!
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.
> 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).
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
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
Patch applied.