Patchwork Fix Ada bootstrap failure

login
register
mail settings
Submitter Richard Guenther
Date Sept. 2, 2011, 11:50 a.m.
Message ID <alpine.LNX.2.00.1109021336341.2130@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/113130/
State New
Headers show

Comments

Richard Guenther - Sept. 2, 2011, 11:50 a.m.
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.

The following patch restores previous behavior by simply not
folding alloca (0).

Bootstrapped on x86_64-unknown-linux-gnu, applied to trunk.

Richard.

2011-09-02  Richard Guenther  <rguenther@suse.de>

	* tree-ssa-ccp.c (fold_builtin_alloca_for_var): Do not
	fold alloca (0).
	(ccp_fold_stmt): Continue replacing args when folding
	alloca fails.
Arnaud Charlet - Sept. 2, 2011, 12:52 p.m.
> 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
Richard Guenther - Sept. 2, 2011, 12:54 p.m.
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.
Arnaud Charlet - Sept. 2, 2011, 12:58 p.m.
> > 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
Robert Dewar - Sept. 2, 2011, 1:06 p.m.
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?
Robert Dewar - Sept. 2, 2011, 1:13 p.m.
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
Richard Guenther - Sept. 2, 2011, 1:16 p.m.
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.
Robert Dewar - Sept. 2, 2011, 1:55 p.m.
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.
Michael Matz - Sept. 2, 2011, 3:47 p.m.
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.
Arnaud Charlet - Sept. 2, 2011, 3:50 p.m.
> 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
Robert Dewar - Sept. 2, 2011, 3:56 p.m.
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.
Michael Matz - Sept. 2, 2011, 3:59 p.m.
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.
Eric Botcazou - Sept. 2, 2011, 4:01 p.m.
> 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.

Patch

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