[patch] Code hoisting bug on arm-elf at -Os

[Diego Novillo <dnovillo at redhat dot com>]

I ran into this bug with a rather bulky C++ program. I have not
been able to reduce the program into a small testcase that
reproduces the problem yet.

The bug occurs in this piece of code:


-------------------------------------------------------------------------------
SDhWord lockPronWord( const TCHAR *pszPron, SDhVoc hVoc, IClsWordNew* pWordNew )
{
  [ ... ]
  csWordName += pszPron;
  SDhWord hWord = SDWord_GetHandle( hVoc, csWordName.getCStr() );
  [ ... ]
-------------------------------------------------------------------------------

ClsString is a wrapper class for _STL::string. The code that gets
generated at -Os for these two statements looks something like
this:

---------------------------------------------------------------------------
	ldr	r4, [fp, #-56]
	sub	r5, fp, #84
	bl	append__[ ... ]
	ldr	r0, [fp, #-108]
	mov	r1, r4
	bl	SDWord_GetHandle(PLT)
---------------------------------------------------------------------------

The intent of the code is to load r0 with hVoc and r1 with
csWordName.getCStr() when calling SDWord_GetHandle(). In this
case, hVoc lives at [fp, #-108] so it is loaded correctly.

Now, csWordName.getCStr() is actually the first field in
csWordName. The assembly fragment above, loads r4 with [fp, #-56]
which is the starting address of csWordName. So, in principle,
loading r1 with r4 before calling SDWord_GetHandle is correct.

The problem is that the call to append__ may have changed the
value at [fp, #-56]. Therefore, when we move r4 into r1, we are
loading r1 with the cached value from [fp, #-56]. If that memory
location changed, we will get it wrong. This happens if append__
needs to reallocate the char array.

We get this one right at -O2 but not at -Os. This is the same
code fragment at both optimization levels (I removed all the
unintersting parts).

The variables live at
	pszPron		-> [fp, #-104]
	hVoc		-> [fp, #-108]
	csWordName	-> [fp, #-56]  (At -Os this value is
					stored in [fp, #-116])


	csWordName += pszPron;
	SDhWord hWord = SDWord_GetHandle( hVoc, csWordName.getCStr() );


-O2				|	-Os
---------------------------------------------------------------------------
sub	r9, fp, #56		|
...                             |	...
ldr	r1, [fp, #-104]         |	ldr	r1, [fp, #-104]
...				|	ldr	r4, [fp, #-56]         [1]
...                             |	...
mov	r0, r9                  |	ldr	r0, [fp, #-116]
bl	append__...          	|	bl	append__...
ldr	r1, [fp, #-56]          |	ldr	r0, [fp, #-108]
ldr	r0, [fp, #-108]         |	mov	r1, r4                 [2]
bl	SDWord_GetHandle(PLT)   |	bl	SDWord_GetHandle(PLT)



We mess up at points [1] and [2]. Loading [fp, #-56] into r4 is
fine, but the value held at [fp, #-56] may change as a result of
the call to append__. To do this right, we should change [2] to
'ldr r1, [fp, #-56]'.

The problem is in hoist_expr_reaches_here_p(). When visiting the
predecessor basic blocks, we mark the immediately preceding bb as
visited before we even visit it.

In this case, the calls to append__... and SDWord_GetHandle are
in separate basic blocks:

[bb 14]
  ...
  ldr r0, [fp, #-56]
  bl append__...

[bb 15]
  ldr r1, [fp, #-56]
  ...
  bl SDWord_GetHandle


When examining expression [fp, #-56] for hoisting, we visit all
the predecessors for bb 15. But in this case, we only have one
predecessor (14) and the very first thing that
`hoist_expr_reaches_here_p' does is mark it visited. But in this
case, the expression is not transparent (it's killed by bb 14).
By not visiting bb 14 we are missing this fact and doing the
wrong thing.

The following patch fixes this problem.


2000-11-08  Diego Novillo  <dnovillo@redhat.com>

	* gcse.c (hoist_expr_reaches_here_p): Do not mark expr_bb as
	visited before visiting it.

Index: gcse.c
===================================================================
RCS file: /cvs/cvsfiles/devo/gcc/gcse.c,v
retrieving revision 1.87
diff -d -c -p -r1.87 gcse.c
*** gcse.c	2000/01/11 14:59:28	1.87
--- gcse.c	2000/11/08 22:23:26
*************** hoist_expr_reaches_here_p (expr_bb, expr
*** 5925,5931 ****
         visited = xcalloc (n_basic_blocks, 1);
      }
  
-   visited[expr_bb] = 1;
    for (pred = BASIC_BLOCK (bb)->pred; pred != NULL; pred = pred->pred_next)
      {
        int pred_bb = pred->src->index;
--- 5925,5930 ----