Re: x86 sibling call stuff

[Mark Mitchell <mark at codesourcery dot com>]

>>>>> "Richard" == Richard Henderson <rth@twiddle.net> writes:

    Richard> On Sun, Mar 19, 2000 at 12:18:07PM -0800, Mark Mitchell
    Richard> wrote:
    >> Right now, this test won't compile.  The reason is that a
    >> NOTE_INSN_EH_REGION_BEG note that gets created during an
    >> expand_call never makes it into the final instruction stream,
    >> even thow the END note does.  A sanity check in flow causes an
    >> abort.

    Richard> The argument to g(f()) is expanded once while trying for
    Richard> a sibcall.  We find out after the fact that there are
    Richard> cleanups pending and dump the sequence.  All well and
    Richard> good.

    Richard> The problem is that when we go to re-evaluate the
    Richard> argument for the normal call sequence, we don't get the
    Richard> same sequence.  We get the call to `f', but we don't get
    Richard> the same cleanups that we got the first time.

I think there's a more fundamental problem.  I've attached a reduced
test-case, together with a minor modification to the C++ front-end
that makes for more consistent handling of TARGET_EXPRs through
UNSAVEs -- but I'm not sure whether that change is necessary or
correct, yet.  Still, it helps to illustrate the problem.  Here's the
code:

  struct A {
    ~A();
  };

  A f();
  void g( A const& );

  int
  main()
  {
    g( f() );
  }

As before, compile with -O2 on i686-pc-linux-gnu to see the bug.

Without intending insult, I'll review the C++ semantics of this
program.  The call to `f' creates a new temporary object of type `A'
in the stack frame of `main'.  The destructor for that object
(`A::~A') is run "at the end of the enclosing full-expression", i.e.,
after the call to `g'.

I think the problem is this:

  o Both attempts to call `f' (the sibling call version, and
    non-sibling call version) register a cleanup (namely the
    destruction of `A'.  (This cleanup is to be run either when
    an exception occurs, or when the call completes normally.)

  o We only expand the cleanups after the call to `g', i.e., well
    out of the range of the sibling call machinery.

Confirming this hypothesis is the result of compiling this code
without exceptions (i.e., with -O2 -fno-exceptions).  I get:

  main:
	pushl	%ebp
	movl	%esp, %ebp
	pushl	%ebx
	subl	$16, %esp
	leal	-5(%ebp), %eax
	pushl	%eax
	leal	-6(%ebp), %ebx
	call	f__Fv
	movzbl	-5(%ebp), %eax
	pushl	%ebx
	movb	%al, -6(%ebp)
	call	g__FRC1A
	addl	$8, %esp
	pushl	$2
	pushl	%ebx
	call	_._1A
	addl	$8, %esp
	leal	-7(%ebp), %eax
	pushl	$2
	pushl	%eax
	call	_._1A
	movl	-4(%ebp), %ebx
	addl	$16, %esp
	xorl	%eax, %eax
	movl	%ebp, %esp
	popl	%ebp
	ret

Note that there are two calls to the destructor there (_._1A).  That's
all straight-line code, so it's going to be running the destructor
twice.

You'll only see this if you apply the attached patch; otherwise, due
to the probably bug you noted with TARGET_EXPRs we don't expand the
cleanups twice -- just because we don't realize we're supposed to
reexpand the TARGET_EXPR.

If I'm right, the sibling call stuff is going to take some extra work
in order to work right with exceptions: you're going to have to
unregister the cleanups registered during the proto-call, or
something.  For now, they should probably just be turned off for C++,
and perhaps Java, if the semantics there are similar.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Index: tree.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/cp/tree.c,v
retrieving revision 1.185
diff -c -p -r1.185 tree.c
*** tree.c	2000/03/03 02:27:15	1.185
--- tree.c	2000/03/20 00:54:40
*************** mark_local_for_remap_r (tp, walk_subtree
*** 2317,2334 ****
  {
    tree t = *tp;
    splay_tree st = (splay_tree) data;
  
!   if ((TREE_CODE (t) == DECL_STMT
!        && nonstatic_local_decl_p (DECL_STMT_DECL (t)))
!       || TREE_CODE (t) == LABEL_STMT)
      {
-       tree decl;
        tree copy;
  
-       /* Figure out what's being declared.  */
-       decl = (TREE_CODE (t) == DECL_STMT
- 	      ? DECL_STMT_DECL (t) : LABEL_STMT_LABEL (t));
-       
        /* Make a copy.  */
        copy = copy_decl_for_inlining (decl, 
  				     DECL_CONTEXT (decl), 
--- 2317,2340 ----
  {
    tree t = *tp;
    splay_tree st = (splay_tree) data;
+   tree decl;
  
!   
!   if (TREE_CODE (t) == DECL_STMT
!       && nonstatic_local_decl_p (DECL_STMT_DECL (t)))
!     decl = DECL_STMT_DECL (t);
!   else if (TREE_CODE (t) == LABEL_STMT)
!     decl = LABEL_STMT_LABEL (t);
!   else if (TREE_CODE (t) == TARGET_EXPR
! 	   && nonstatic_local_decl_p (TREE_OPERAND (t, 0)))
!     decl = TREE_OPERAND (t, 0);
!   else
!     decl = NULL_TREE;
! 
!   if (decl)
      {
        tree copy;
  
        /* Make a copy.  */
        copy = copy_decl_for_inlining (decl, 
  				     DECL_CONTEXT (decl), 
*************** mark_local_for_remap_r (tp, walk_subtree
*** 2344,2350 ****
  }
  
  /* Called via walk_tree when an expression is unsaved.  Using the
!    splay_tree pointed to by ST (which is really a `splay_tree *'),
     remaps all local declarations to appropriate replacements.  */
  
  static tree
--- 2350,2356 ----
  }
  
  /* Called via walk_tree when an expression is unsaved.  Using the
!    splay_tree pointed to by ST (which is really a `splay_tree'),
     remaps all local declarations to appropriate replacements.  */
  
  static tree