| Message ID | alpine.LNX.2.00.1109021336341.2130@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
> This fixes the Ada bootstrap failure introduced by alloca folding. > We now fold alloca (0) to &auto-with-size-zero which confuses us. > I didn't exactly investigate but what I think happens is that we > expand that &auto-with-size-zero to NULL instead of > virtual_stack_dynamic_rtx (see zero-size special-case in > allocate_dynamic_stack_space) and Ada ends up dereferencing the > pointer returned from alloca (0) (something to investigate for > the Ada folks I guess), something which "works" if we just > return a random stack address. Thanks! In Ada, it's quite natural to end up with a dynamically sized object of size 0. For instance, if you declare an array with a dynamic bound: Table : Unit_Table (1 .. Last_Unit); and Last_Unit happens to be 0 at run-time Arno
On Fri, 2 Sep 2011, Arnaud Charlet wrote: > > This fixes the Ada bootstrap failure introduced by alloca folding. > > We now fold alloca (0) to &auto-with-size-zero which confuses us. > > I didn't exactly investigate but what I think happens is that we > > expand that &auto-with-size-zero to NULL instead of > > virtual_stack_dynamic_rtx (see zero-size special-case in > > allocate_dynamic_stack_space) and Ada ends up dereferencing the > > pointer returned from alloca (0) (something to investigate for > > the Ada folks I guess), something which "works" if we just > > return a random stack address. > > Thanks! > > In Ada, it's quite natural to end up with a dynamically sized object of > size 0. For instance, if you declare an array with a dynamic bound: > > Table : Unit_Table (1 .. Last_Unit); > > and Last_Unit happens to be 0 at run-time But are we expected to read/store from the storage? I'd have expected that alloca (0) returning NULL shouldn't break anything at runtime ... Richard.
> > In Ada, it's quite natural to end up with a dynamically sized object of > > size 0. For instance, if you declare an array with a dynamic bound: > > > > Table : Unit_Table (1 .. Last_Unit); > > > > and Last_Unit happens to be 0 at run-time > > But are we expected to read/store from the storage? No, that shouldn't happen, although you can e.g. reference Table'Address and expect it to be non null. > I'd have > expected that alloca (0) returning NULL shouldn't break > anything at runtime ... Not sure exactly what failed here, probably something relatively subtle (perhaps related to passing this variable or a "slice" of this variable to another procedure). Arno
On 9/2/2011 8:52 AM, Arnaud Charlet wrote: > Thanks! > > In Ada, it's quite natural to end up with a dynamically sized object of > size 0. For instance, if you declare an array with a dynamic bound: > > Table : Unit_Table (1 .. Last_Unit); > > and Last_Unit happens to be 0 at run-time > > Arno But isn't it odd that we would dereference such an address?
On 9/2/2011 8:58 AM, Arnaud Charlet wrote: >>> In Ada, it's quite natural to end up with a dynamically sized object of >>> size 0. For instance, if you declare an array with a dynamic bound: >>> >>> Table : Unit_Table (1 .. Last_Unit); >>> >>> and Last_Unit happens to be 0 at run-time >> >> But are we expected to read/store from the storage? > > No, that shouldn't happen, although you can e.g. reference Table'Address > and expect it to be non null. Actually I am not sure of this, I discussed this with Bob, Address is defined as the pointing to the first storage unit allocated for an object. Not clear what this means when the object has no storage units. This is a gap in the RM. Bob's view is that it must return some random valid address (what exactly *is* a valid address?) > >> I'd have >> expected that alloca (0) returning NULL shouldn't break >> anything at runtime ... > > Not sure exactly what failed here, probably something relatively subtle > (perhaps related to passing this variable or a "slice" of this variable > to another procedure). But that wouldn't cause a dereference, however, it might cause an explicit test that the argument was not null, and perhaps that's what is causing the trouble. For example, if you have something like type S is aliased array (1 .. N); type P is access all S; B : S; procedure Q is (A : not null Astring) is begin null; end; Q (B'Access); Then there will be an explicit check that B is not null > > Arno
On Fri, 2 Sep 2011, Robert Dewar wrote: > On 9/2/2011 8:58 AM, Arnaud Charlet wrote: > > > > In Ada, it's quite natural to end up with a dynamically sized object of > > > > size 0. For instance, if you declare an array with a dynamic bound: > > > > > > > > Table : Unit_Table (1 .. Last_Unit); > > > > > > > > and Last_Unit happens to be 0 at run-time > > > > > > But are we expected to read/store from the storage? > > > > No, that shouldn't happen, although you can e.g. reference Table'Address > > and expect it to be non null. > > Actually I am not sure of this, I discussed this with Bob, Address > is defined as the pointing to the first storage unit allocated for > an object. Not clear what this means when the object has no storage > units. This is a gap in the RM. Bob's view is that it must return > some random valid address (what exactly *is* a valid address?) > > > > > I'd have > > > expected that alloca (0) returning NULL shouldn't break > > > anything at runtime ... > > > > Not sure exactly what failed here, probably something relatively subtle > > (perhaps related to passing this variable or a "slice" of this variable > > to another procedure). > > But that wouldn't cause a dereference, however, it might cause an > explicit test that the argument was not null, and perhaps that's > what is causing the trouble. > > For example, if you have something like > > type S is aliased array (1 .. N); > type P is access all S; > B : S; > > procedure Q is (A : not null Astring) is > begin > null; > end; > > Q (B'Access); > > Then there will be an explicit check that B is not null The bootstrap failure showed NULL pointer dereferences (which probably easily points to the affected part of the RTS). Richard.
On 9/2/2011 9:16 AM, Richard Guenther wrote: > The bootstrap failure showed NULL pointer dereferences (which > probably easily points to the affected part of the RTS). Might be interesting to pursue, but we don't know that the null pointers being dereferenced are in fact the ones returned by alloca. May not be worth the effort.
Hi, On Fri, 2 Sep 2011, Robert Dewar wrote: > On 9/2/2011 9:16 AM, Richard Guenther wrote: > > Might be interesting to pursue, but we don't know that the null pointers > being dereferenced are in fact the ones returned by alloca. May not be > worth the effort. Given the nature of the work-around which makes Ada work again it's fairly sure that the Ada frontend does emit accesses to an alloca'ed area of memory even if its size is zero. I.e. definitely a real bug. Ciao, Michael.
> Given the nature of the work-around which makes Ada work again it's fairly > sure that the Ada frontend does emit accesses to an alloca'ed area of > memory even if its size is zero. I.e. definitely a real bug. Well, it's not clear whether it's the Ada frontend or the middle which is emitting these, and until we have more info, it's hard to know whether it's a real bug. Although sounds like there might indeed potentially be an issue. I guess valgrind would detect and report this kind of issue? Arno
On 9/2/2011 11:47 AM, Michael Matz wrote: > Hi, > > On Fri, 2 Sep 2011, Robert Dewar wrote: > >> On 9/2/2011 9:16 AM, Richard Guenther wrote: >> >> Might be interesting to pursue, but we don't know that the null pointers >> being dereferenced are in fact the ones returned by alloca. May not be >> worth the effort. > > Given the nature of the work-around which makes Ada work again it's fairly > sure that the Ada frontend does emit accesses to an alloca'ed area of > memory even if its size is zero. I.e. definitely a real bug. maybe so, but I gave a scenario (there are others) in which exceptions are legitimately raised without deferencing the pointer. Once an exception is raised all sorts of funny things can happen (e.g. tasks silently terminating fi they have no top level exception handler), so you can't make that direct conclusion. I guess if you made alloca(0) return a junk non-derefencable address, *that* would be definitive. > > > Ciao, > Michael.
Hi, On Fri, 2 Sep 2011, Arnaud Charlet wrote: > > Given the nature of the work-around which makes Ada work again it's fairly > > sure that the Ada frontend does emit accesses to an alloca'ed area of > > memory even if its size is zero. I.e. definitely a real bug. > > Well, it's not clear whether it's the Ada frontend or the middle which is > emitting these, and until we have more info, it's hard to know whether it's > a real bug. Although sounds like there might indeed potentially be an issue. > I guess valgrind would detect and report this kind of issue? Only the null pointer access, but that already trivial segfaults, not need for valgrind. invalid stack accesses aren't reported by valgrind as such, because the stack is usually initialized and writable. Just apply the test patch from Richi (returning const0_rtx from allocate_dynamic_stack_space for the size=0 case) and investigate. Ciao, Michael.
> This fixes the Ada bootstrap failure introduced by alloca folding. > We now fold alloca (0) to &auto-with-size-zero which confuses us. > I didn't exactly investigate but what I think happens is that we > expand that &auto-with-size-zero to NULL instead of > virtual_stack_dynamic_rtx (see zero-size special-case in > allocate_dynamic_stack_space) and Ada ends up dereferencing the > pointer returned from alloca (0) (something to investigate for > the Ada folks I guess), something which "works" if we just > return a random stack address. This looks more convoluted than that though: AFAICS the miscompilation is introduced by reload which spills a pseudo to a stack slot that is already taken by something else: (insn 1523 1522 1524 184 (set (reg:SI 404 [ D.7515 ]) (mem/s/j/c:SI (plus:SI (reg/f:SI 20 frame) (const_int -32 [0xffffffffffffffe0])) [49 FRAME.261.last_unit+0 S4 A64])) /home/eric/gnat/gnat-head/src/gcc/ada/lib-writ.adb:545 50 {*movsi_internal} [...] (insn 1527 1526 1528 185 (set (reg/f:SI 581 [ pretmp.679 ]) (mem/s/f/j/c:SI (plus:SI (reg/f:SI 20 frame) (const_int -4 [0xfffffffffffffffc])) [49 FRAME.261.with_flags.141+0 S4 A32])) 50 {*movsi_internal} (nil)) and pseudo 404 is spilled to the location of FRAME.261.with_flags.141: (insn 1523 1522 3296 185 (set (reg:SI 1 dx) (mem/s/j/c:SI (plus:SI (reg/f:SI 6 bp) (const_int -56 [0xffffffffffffffc8])) [49 FRAME.261.last_unit+0 S4 A64])) /home/eric/gnat/gnat-head/src/gcc/ada/lib-writ.adb:545 50 {*movsi_internal} (nil)) (insn 3296 1523 1524 185 (set (mem/c:SI (plus:SI (reg/f:SI 6 bp) (const_int -28 [0xffffffffffffffe4])) [68 %sfp+-4 S4 A32]) (reg:SI 1 dx)) /home/eric/gnat/gnat-head/src/gcc/ada/lib-writ.adb:545 50 {*movsi_internal} (nil)) [...] (insn 1527 1526 1528 186 (set (reg/f:SI 4 si [orig:581 pretmp.679 ] [581]) (mem/s/f/j/c:SI (plus:SI (reg/f:SI 6 bp) (const_int -28 [0xffffffffffffffe4])) [49 FRAME.261.with_flags.141+0 S4 A32])) 50 {*movsi_internal} (nil)) so accessing the With_Flags array (which is not empty) yields a SEGV because the base pointer is equal to Last_Unit (i.e. 2). In other words, the GIMPLE code looks legitimate and the bug is very likely in the stack slot allocation code (maybe triggered by the newly created zero-sized arrays). In any case, thanks for fixing the bootstrap failure.
Index: gcc/tree-ssa-ccp.c =================================================================== --- gcc/tree-ssa-ccp.c (revision 178460) +++ gcc/tree-ssa-ccp.c (working copy) @@ -1702,10 +1687,14 @@ fold_builtin_alloca_for_var (gimple stmt /* Detect constant argument. */ arg = get_constant_value (gimple_call_arg (stmt, 0)); - if (arg == NULL_TREE || TREE_CODE (arg) != INTEGER_CST + if (arg == NULL_TREE + || TREE_CODE (arg) != INTEGER_CST || !host_integerp (arg, 1)) return NULL_TREE; + size = TREE_INT_CST_LOW (arg); + if (size == 0) + return NULL_TREE; /* Heuristic: don't fold large vlas. */ threshold = (unsigned HOST_WIDE_INT)PARAM_VALUE (PARAM_LARGE_STACK_FRAME); @@ -1804,12 +1793,12 @@ ccp_fold_stmt (gimple_stmt_iterator *gsi if (gimple_call_alloca_for_var_p (stmt)) { tree new_rhs = fold_builtin_alloca_for_var (stmt); - bool res; - if (new_rhs == NULL_TREE) - return false; - res = update_call_from_tree (gsi, new_rhs); - gcc_assert (res); - return true; + if (new_rhs) + { + bool res = update_call_from_tree (gsi, new_rhs); + gcc_assert (res); + return true; + } } /* Propagate into the call arguments. Compared to replace_uses_in