[PATCH] Fix PR middle-end/17813

[PATCH] Fix PR middle-end/17813

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

Hi,

This is the Ada compiler bootstrap failure on i586 and below, which again 
pertains to stack adjustments.  cstand.adb:Create_Standard is miscompiled 
because a stack adjustment is emitted right after a __builtin_stack_restore.

The proposed fix is to emit pending stack adjustments right before 
__builtin_stack_save and __builtin_stack_restore.  Bootstrapped on 
i586-mandrake-linux-gnu (with the workaround for PR tree-opt/17986).
OK for mainline?


2004-10-16  Eric Botcazou  <ebotcazou@libertysurf.fr>

	PR middle-end/17813
	* explow.c (emit_stack_save): Emit pending stack adjustments.
	(emit_stack_restore): Likewise.


-- 
Eric Botcazou


[-- Attachment #2: pr17813.diff --]
[-- Type: text/x-diff, Size: 1301 bytes --]

Index: explow.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/explow.c,v
retrieving revision 1.139
diff -u -p -r1.139 explow.c
--- explow.c	8 Sep 2004 08:05:13 -0000	1.139
+++ explow.c	16 Oct 2004 16:41:55 -0000
@@ -965,6 +965,7 @@ emit_stack_save (enum save_level save_le
       rtx seq;
 
       start_sequence ();
+      do_pending_stack_adjust ();
       /* We must validize inside the sequence, to ensure that any instructions
 	 created by the validize call also get moved to the right place.  */
       if (sa != 0)
@@ -976,6 +977,7 @@ emit_stack_save (enum save_level save_le
     }
   else
     {
+      do_pending_stack_adjust ();
       if (sa != 0)
 	sa = validize_mem (sa);
       emit_insn (fcn (sa, stack_pointer_rtx));
@@ -1037,13 +1039,17 @@ emit_stack_restore (enum save_level save
       rtx seq;
 
       start_sequence ();
+      do_pending_stack_adjust ();
       emit_insn (fcn (stack_pointer_rtx, sa));
       seq = get_insns ();
       end_sequence ();
       emit_insn_after (seq, after);
     }
   else
-    emit_insn (fcn (stack_pointer_rtx, sa));
+    {
+      do_pending_stack_adjust ();
+      emit_insn (fcn (stack_pointer_rtx, sa));
+    }
 }
 
 /* Invoke emit_stack_save on the nonlocal_goto_save_area for the current

Re: [PATCH] Fix PR middle-end/17813

[Roger Sayle]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 929 bytes --]


On Sat, 16 Oct 2004, Eric Botcazou wrote:
> 2004-10-16  Eric Botcazou  <ebotcazou@libertysurf.fr>
>
> 	PR middle-end/17813
> 	* explow.c (emit_stack_save): Emit pending stack adjustments.
> 	(emit_stack_restore): Likewise.


This is OK for mainline.  A slight improvement might be to add a new
ignore_pending_stack_adjust function (like clear_pending_stack_adjust)
that resets stack_pointer_delta and pending_stack_adjust to zero, and
call that in emit_stack_restore.  Currently, we'll emit a "dead" stack
adjustment immediately prior to restoring the stack pointer, and then
leave it to the RTL optimizers to clean up.  All we really need to do
is flush the "memory" of the pending adjustment.


It's also not clear whether the two builtins __builtin_stack_save and
__builtin_stack_restore need to be documented in extend.texi, or if
they're intended purely as internal placeholders.  Zdenek?

Roger
--

Re: [PATCH] Fix PR middle-end/17813

[Jan Hubicka]

> 
> On Sat, 16 Oct 2004, Eric Botcazou wrote:
> > 2004-10-16  Eric Botcazou  <ebotcazou@libertysurf.fr>
> >
> > 	PR middle-end/17813
> > 	* explow.c (emit_stack_save): Emit pending stack adjustments.
> > 	(emit_stack_restore): Likewise.
> 
> 
> This is OK for mainline.  A slight improvement might be to add a new
> ignore_pending_stack_adjust function (like clear_pending_stack_adjust)
> that resets stack_pointer_delta and pending_stack_adjust to zero, and
> call that in emit_stack_restore.  Currently, we'll emit a "dead" stack
> adjustment immediately prior to restoring the stack pointer, and then
> leave it to the RTL optimizers to clean up.  All we really need to do
> is flush the "memory" of the pending adjustment.

RTL optimizers are not terribly smart about optimizing stack
adjustments.  WOuld be possible to double check if they are really
elliminated?
> 
> 
> It's also not clear whether the two builtins __builtin_stack_save and
> __builtin_stack_restore need to be documented in extend.texi, or if
> they're intended purely as internal placeholders.  Zdenek?

For purely internal builtins we probably should have something
preventing frontend from recognizing them in the input source so user
can't confuse us.

Honza
> 
> Roger
> --

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> This is OK for mainline.

Thanks for the review.

> A slight improvement might be to add a new ignore_pending_stack_adjust
> function (like clear_pending_stack_adjust) that resets stack_pointer_delta
> and pending_stack_adjust to zero, and  call that in emit_stack_restore.  

I have 3 remarks/questions:
- what about discard_pending_stack_adjust instead?  I think that "ignore" 
could fool people into thinking that the stack adjustment is only temporarily 
suspended.
- what about changing clear_pending_stack_adjust into 
maybe_discard_pending_stack_adjust?  The rationale is that the typical idiom 
is currently:

  clear_pending_stack_adjust ();
  do_pending_stack_adjust ();

which seems odd at best.

- I think that stack_pointer_delta should not be reset to zero for strict 
correctness, and instead the same adjustment should be made in the new 
function as in clear_pending_stack_adjust.

> Currently, we'll emit a "dead" stack  adjustment immediately prior to
> restoring the stack pointer, and then  leave it to the RTL optimizers to
> clean up.  All we really need to do is flush the "memory" of the pending 
> adjustment. 

This was the acknowledged strategy used in the patch.  Now, after Honza's 
remark, I think yours is better.

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Roger Sayle]

On Sun, 17 Oct 2004, Eric Botcazou wrote:
> > A slight improvement might be to add a new ignore_pending_stack_adjust
> > function (like clear_pending_stack_adjust) that resets stack_pointer_delta
> > and pending_stack_adjust to zero, and  call that in emit_stack_restore.
>
> I have 3 remarks/questions:
> - what about discard_pending_stack_adjust instead?  I think that "ignore"
> could fool people into thinking that the stack adjustment is only temporarily
> suspended.

Sure.


> - what about changing clear_pending_stack_adjust into
> maybe_discard_pending_stack_adjust?  The rationale is that the typical
> idiom is currently:
>
>   clear_pending_stack_adjust ();
>   do_pending_stack_adjust ();
>
> which seems odd at best.

I'll agree that the semantics of clear_pending_stack_adjust are a bit
strange, i.e. that this function is occassionally a no-op even if there's
a pending stack adjustment.  And it does indeed look like, with the
single exception of expand_call in calls.c, all uses of
clear_pending_stack_adjust are followed by do_pending_stack_adjust, so an
interface/API clean-up (simplification) might be a good thing.  Especially
as expand_call's exceptional use looks like a good candidate for the
proposed discard_pending_stack_adjust.


> - I think that stack_pointer_delta should not be reset to zero for strict
> correctness, and instead the same adjustment should be made in the new
> function as in clear_pending_stack_adjust.

Probably.  I'll admit that I didn't fully investigate exactly how
stack_pointer_delta needs to be updated in discard_pending_stack_adjust,
just that it needs to be updated (which is why init_pending_stack_adjust
isn't suitable).  I'd need to read through more code to convince myself
that a non-zero stack pointer delta is correct/safe for these builtins.


Roger
--

Re: [PATCH] Fix PR middle-end/17813

[Matthias Klose]

Eric Botcazou writes:
> Hi,
> 
> This is the Ada compiler bootstrap failure on i586 and below, which again 
> pertains to stack adjustments.  cstand.adb:Create_Standard is miscompiled 
> because a stack adjustment is emitted right after a __builtin_stack_restore.

with this patch (configured for i486-linux) I get until:

Bootstrap comparison failure!
ada/a-except.o differs
ada/ali-util.o differs
ada/s-crc32.o differs
make[3]: *** [gnucompare] Error 1

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> with this patch (configured for i486-linux) I get until:
>
> Bootstrap comparison failure!
> ada/a-except.o differs
> ada/ali-util.o differs
> ada/s-crc32.o differs
> make[3]: *** [gnucompare] Error 1

Great ;-)  Could you post the exact configure line?

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Matthias Klose]

Eric Botcazou writes:
> > with this patch (configured for i486-linux) I get until:
> >
> > Bootstrap comparison failure!
> > ada/a-except.o differs
> > ada/ali-util.o differs
> > ada/s-crc32.o differs
> > make[3]: *** [gnucompare] Error 1
> 
> Great ;-)  Could you post the exact configure line?

bootstrap compiler is gcc-3.3.5.

../src/configure -v --enable-languages=c,c++,java,f95,objc,ada --prefix=/usr --libexecdir=/usr/lib --with-gxx-include-dir=/usr/include/c++/4.0 --enable-shared --with-system-zlib --enable-nls --enable-threads=posix --without-included-gettext --program-suffix=-4.0 --enable-__cxa_atexit --enable-libstdcxx-allocator=mt  --enable-clocale=gnu --enable-libstdcxx-debug --enable-java-gc=boehm --enable-java-awt=gtk --enable-mpfr i486-linux

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> ../src/configure -v --enable-languages=c,c++,java,f95,objc,ada
> --prefix=/usr --libexecdir=/usr/lib
> --with-gxx-include-dir=/usr/include/c++/4.0 --enable-shared
> --with-system-zlib --enable-nls --enable-threads=posix
> --without-included-gettext --program-suffix=-4.0 --enable-__cxa_atexit
> --enable-libstdcxx-allocator=mt  --enable-clocale=gnu
> --enable-libstdcxx-debug --enable-java-gc=boehm --enable-java-awt=gtk
> --enable-mpfr i486-linux

OK, I could reproduce with i486-linux.  The bad news is that the stage2 Ada 
front-end appears to be miscompiled by the stage1 compiler and that there is 
currently no Ada-aware debugger that understands location lists...

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Daniel Jacobowitz]

On Sun, Oct 17, 2004 at 07:55:33PM +0200, Eric Botcazou wrote:
> > ../src/configure -v --enable-languages=c,c++,java,f95,objc,ada
> > --prefix=/usr --libexecdir=/usr/lib
> > --with-gxx-include-dir=/usr/include/c++/4.0 --enable-shared
> > --with-system-zlib --enable-nls --enable-threads=posix
> > --without-included-gettext --program-suffix=-4.0 --enable-__cxa_atexit
> > --enable-libstdcxx-allocator=mt  --enable-clocale=gnu
> > --enable-libstdcxx-debug --enable-java-gc=boehm --enable-java-awt=gtk
> > --enable-mpfr i486-linux
> 
> OK, I could reproduce with i486-linux.  The bad news is that the stage2 Ada 
> front-end appears to be miscompiled by the stage1 compiler and that there is 
> currently no Ada-aware debugger that understands location lists...

Have you tried GDB HEAD lately?  I was under the impression that Ada
support had been turned on (not long ago).

-- 
Daniel Jacobowitz

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> Have you tried GDB HEAD lately?  I was under the impression that Ada
> support had been turned on (not long ago).

Oops!  I really should follow what the GDB folks at ACT are doing...  Right, 
it was turned on recently and this is great news.  Thanks for the heads up.

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Arnaud Charlet]

> Oops!  I really should follow what the GDB folks at ACT are doing...  Right, 
> it was turned on recently and this is great news.  Thanks for the heads up.

Note that for doing low-level debugging of Ada code, you can use
GDB in C mode without too much trouble, so for working on GCC/GNAT itself,
the fancy Ada mode features are not strictly necessary.

That's how we worked on GNAT for years before there we wrote Ada support
in gdb.

Arno

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> Note that for doing low-level debugging of Ada code, you can use
> GDB in C mode without too much trouble, so for working on GCC/GNAT itself,
> the fancy Ada mode features are not strictly necessary.

Of course, but for debugging a miscompilation of the Ada scanner (e.g.
PR middle-end/18045), having "pn" at hand is almost required if you are not 
familiar with the internal structure of the front-end (e.g. how to walk the 
node hierarchy) like me.

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> with this patch (configured for i486-linux) I get until:
>
> Bootstrap comparison failure!
> ada/a-except.o differs
> ada/ali-util.o differs
> ada/s-crc32.o differs
> make[3]: *** [gnucompare] Error 1

The stage3 s-crc32.o is miscompiled by the stage2 compiler because the stage2 
uintp.o is miscompiled by the stage1 compiler because of PR middle-end/18045.

I'm testing a fix for the latter on a full bootstrap (a C+Ada bootstrap 
already completed, even without stumbling upon PR tree-opt/17986).

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

> This is the Ada compiler bootstrap failure on i586 and below, which again
> pertains to stack adjustments.  cstand.adb:Create_Standard is miscompiled
> because a stack adjustment is emitted right after a
> __builtin_stack_restore.

Here's the final patch I commited to mainline.  Bootstrapped/regtested (all 
languages except treelang with upcoming patch for PR middle-end/18045) on 
i486-mandrake-linux-gnu, approved offline by Roger.


2004-10-18  Eric Botcazou  <ebotcazou@libertysurf.fr>
              Roger Sayle  <roger@eyesopen.com>

	PR middle-end/17813
	* dojump.c (discard_pending_stack_adjust): New function.
	(clear_pending_stack_adjust): Call it.
	* expr.h (discard_pending_stack_adjust): Declare it.
	* explow.c (emit_stack_save): Emit pending stack adjustments
	before saving the stack pointer.
	(emit_stack_restore): Discard pending stack adjustments before
	restoring the stack pointer.


-- 
Eric Botcazou

[-- Attachment #2: pr17813-2.diff --]
[-- Type: text/x-diff, Size: 2799 bytes --]

Index: dojump.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/dojump.c,v
retrieving revision 1.29
diff -u -p -r1.29 dojump.c
--- dojump.c	8 Sep 2004 07:47:45 -0000	1.29
+++ dojump.c	18 Oct 2004 18:48:23 -0000
@@ -50,6 +50,15 @@ init_pending_stack_adjust (void)
   pending_stack_adjust = 0;
 }
 
+/* Discard any pending stack adjustment.  This avoid relying on the
+   RTL optimizers to remove useless adjustments when we know the
+   stack pointer value is dead.  */
+void discard_pending_stack_adjust (void)
+{
+  stack_pointer_delta -= pending_stack_adjust;
+  pending_stack_adjust = 0;
+}
+
 /* When exiting from function, if safe, clear out any pending stack adjust
    so the adjustment won't get done.
 
@@ -64,10 +73,7 @@ clear_pending_stack_adjust (void)
       && EXIT_IGNORE_STACK
       && ! (DECL_INLINE (current_function_decl) && ! flag_no_inline)
       && ! flag_inline_functions)
-    {
-      stack_pointer_delta -= pending_stack_adjust,
-      pending_stack_adjust = 0;
-    }
+    discard_pending_stack_adjust ();
 }
 
 /* Pop any previously-pushed arguments that have not been popped yet.  */
Index: explow.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/explow.c,v
retrieving revision 1.139
diff -u -p -r1.139 explow.c
--- explow.c	8 Sep 2004 08:05:13 -0000	1.139
+++ explow.c	18 Oct 2004 18:48:33 -0000
@@ -965,6 +965,7 @@ emit_stack_save (enum save_level save_le
       rtx seq;
 
       start_sequence ();
+      do_pending_stack_adjust ();
       /* We must validize inside the sequence, to ensure that any instructions
 	 created by the validize call also get moved to the right place.  */
       if (sa != 0)
@@ -976,6 +977,7 @@ emit_stack_save (enum save_level save_le
     }
   else
     {
+      do_pending_stack_adjust ();
       if (sa != 0)
 	sa = validize_mem (sa);
       emit_insn (fcn (sa, stack_pointer_rtx));
@@ -1032,6 +1034,8 @@ emit_stack_restore (enum save_level save
 		    gen_rtx_MEM (BLKmode, stack_pointer_rtx)));
     }
 
+  discard_pending_stack_adjust ();
+
   if (after)
     {
       rtx seq;
Index: expr.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.h,v
retrieving revision 1.171
diff -u -p -r1.171 expr.h
--- expr.h	8 Sep 2004 18:44:56 -0000	1.171
+++ expr.h	18 Oct 2004 18:48:36 -0000
@@ -499,6 +499,9 @@ extern void expand_var (tree);
    arguments waiting to be popped.  */
 extern void init_pending_stack_adjust (void);
 
+/* Discard any pending stack adjustment.  */
+extern void discard_pending_stack_adjust (void);
+
 /* When exiting from function, if safe, clear out any pending stack adjust
    so the adjustment won't get done.  */
 extern void clear_pending_stack_adjust (void);

Re: [PATCH] Fix PR middle-end/17813

[Richard Kenner]

    OK, I could reproduce with i486-linux.  The bad news is that the
    stage2 Ada front-end appears to be miscompiled by the stage1 compiler
    and that there is currently no Ada-aware debugger that understands
    location lists...

I'm also seeing a miscompilation of gnatmake.

But there are still ACATS execution failures, so I'm hoping that fixing
those will fix these issues.  I hope to get back to that task shortly.

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> I'm also seeing a miscompilation of gnatmake.

Do you mean an infinite loop compiling make.adb?  If so, it's PR 
tree-opt/17986.  Please assign it to you if you're working on it, this will 
avoid parallel work.

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Richard Kenner]

    Do you mean an infinite loop compiling make.adb?  

Nope.  I mean gnatmake blowing up building gnatmem and also in a few of the C2
ACATS tests.  This is on x86-64.

    Please assign it to you if you're working on it, this will avoid
    parallel work.

And, as I said, I'm not working on it yet: I want to fix the recent ACATS
regressions first and hope that gets fixed in the process.

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> Nope.  I mean gnatmake blowing up building gnatmem and also in a few of the
> C2 ACATS tests.  This is on x86-64.

OK, thanks for the clarification.  For some reason, PR tree-opt/17986 shows up 
on i686-mingw32, i586-linux, sparc-solaris but apparently not on i686-linux 
and x86_64-linux.

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Richard Kenner]

    Of course, but for debugging a miscompilation of the Ada scanner (e.g.
    PR middle-end/18045), having "pn" at hand is almost required if you
    are not familiar with the internal structure of the front-end
    (e.g. how to walk the node hierarchy) like me.

But that doesn't need an Ada-aware debugger!  I use that all the time
with non-Ada-aware debuggers.  Just do

define pn
set pn($)
end

Re: [PATCH] Fix PR middle-end/17813

[Eric Botcazou]

> But that doesn't need an Ada-aware debugger!  I use that all the time
> with non-Ada-aware debuggers.  Just do
>
> define pn
> set pn($)
> end

I obviously missed something.  So the debug functions are exposed by the 
front-end through a C interface?  More generally, is a non-Ada-aware debugger 
sufficient to debug the Ada part of the front-end?

-- 
Eric Botcazou

Re: [PATCH] Fix PR middle-end/17813

[Laurent GUERBY]

On Mon, 2004-10-18 at 22:39, Eric Botcazou wrote:
> > But that doesn't need an Ada-aware debugger!  I use that all the time
> > with non-Ada-aware debuggers.  Just do
> >
> > define pn
> > set pn($)
> > end
> 
> I obviously missed something.  So the debug functions are exposed by the 
> front-end through a C interface?  More generally, is a non-Ada-aware debugger 
> sufficient to debug the Ada part of the front-end?

See ada/treepr.ads and ada/sprint.ads for more documentation, the "pX"
function are pragma Exported so they're callable from gdb even if not
Ada aware.

Laurent

Re: [PATCH] Fix PR middle-end/17813

[Richard Kenner]

    I obviously missed something.  So the debug functions are exposed by
    the front-end through a C interface?  

No.  There's a pragma Export giving that name.

    More generally, is a non-Ada-aware debugger sufficient to debug the Ada
    part of the front-end?

Pretty much. I don't do work in that code, but the few times I've had to look
at it, I've had no problems.  I only very recently started using an Ada-aware
debugger.  You just have to know a lot about how things are encoded, but
I'm sure you do!