Bug 24996 - [4.0 Regression] ICE on throw code
Summary: [4.0 Regression] ICE on throw code
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 4.2.0
: P1 critical
Target Milestone: 4.0.3
Assignee: Jason Merrill
URL:
Keywords: ice-on-valid-code, monitored
Depends on:
Blocks:
 
Reported: 2005-11-23 04:12 UTC by Jan Dvorak
Modified: 2006-02-12 23:35 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work: 3.4.1 4.1.0 4.2.0
Known to fail: 4.0.0 3.4.0
Last reconfirmed: 2005-11-23 05:09:51


Attachments
Possible patch (4.75 KB, patch)
2006-01-26 23:51 UTC, Zdenek Dvorak
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Dvorak 2005-11-23 04:12:30 UTC
Got ICE with latest cvs (but also with 4.0.1 release) on this code:
-------------------------------
#include <stdexcept> 
void foo(bool b)
{
    throw b ? std::logic_error("") : std::logic_error("");
}
-------------------------------

Preprocessed file: http://napalm.sf.cz/throw.ii.gz

ICE:
# g++ ./throw.cpp 
 <with_cleanup_expr 0xb7367de0>./throw.cpp: In function 'void foo(bool)':
./throw.cpp:5: internal compiler error: unexpected node
Please submit a full bug report,
with preprocessed source if appropriate.
See <URL:http://gcc.gnu.org/bugs.html> for instructions.

Version:
# g++ -v
Using built-in specs.
Target: i686-pc-linux-gnu
Configured with: ../gcc/configure --enable-shared --enable-threads=posix --enable-languages=c,c++
Thread model: posix
gcc version 4.2.0 20051123 (experimental)
Comment 1 Andrew Pinski 2005-11-23 04:18:22 UTC
Reducing.
Comment 2 Andrew Pinski 2005-11-23 05:09:51 UTC
Reduced testcase:
struct allocator {
  ~allocator();
};
struct string {
  string(int, const allocator& __a = allocator());
};
struct logic_error {
  logic_error(const string& __arg);
};
void foo(bool b) {
  throw b ? logic_error(0) : logic_error(0);
}
Comment 3 Volker Reichelt 2005-11-23 17:37:30 UTC
Testcase with simpler class hierarchy:

======================================
struct A
{
    ~A();
};

struct B
{
    B(A);
};

void foo(bool b)
{
    throw b ? B(A()) : B(A());
}
======================================

This testaces also crashes GCC 3.4.0, but not 3.4.1 - 3.4.4.
Comment 4 Janis Johnson 2005-11-29 18:36:56 UTC
A regression hunt identified the following patch (not terribly useful):

  r81764 | dnovillo | 2004-05-13 06:41:07 +0000 (Thu, 13 May 2004) | 3 lines

  Merge tree-ssa-20020619-branch into mainline.

Andrew, are you sure about 3.4.0 crashing for this testcase?  I tried mainline
as far back as 2003-03-08 at regular intervals and saw no failures before the
tree-ssa merge.
Comment 5 Janis Johnson 2005-11-29 18:37:30 UTC
The question in my previous comment should have been to Volker, not Andrew.
Comment 6 Volker Reichelt 2005-11-29 18:44:31 UTC
Subject: Re:  [4.0/4.1/4.2 Regression] ICE on throw code

On 29 Nov, janis at gcc dot gnu dot org wrote:
> Andrew, are you sure about 3.4.0 crashing for this testcase?  I tried mainline
> as far back as 2003-03-08 at regular intervals and saw no failures before the
> tree-ssa merge.

I can see the failure in 3.4.0 on i686-pc-linux-gnu and
x86_64-unknown-linux-gnu. This is a segfault, however, and
therefore probably caused a different problem.

Regards,
Volker


Comment 7 Mark Mitchell 2005-12-19 18:08:20 UTC
We need to at least understand this problem before release: P1.
Comment 8 Zdenek Dvorak 2005-12-21 14:34:35 UTC
Gimplification lets with_cleanup_exprs escape to gimple.  I am investigating why that happens.
Comment 9 Zdenek Dvorak 2006-01-02 18:34:45 UTC
As the comments in gimplify_cleanup_point_expr indicate,
it only handles cases when cleanup_exprs are not within nested
constructs (with the exception of conditions).

In this PR, the code basically looks like

cleanup_point_expr
  {
    ...
    try
      {
        tmp = b ? target_expr (bla1, something, cleanup1) : target_expr (bla2, something, cleanup2)
      }
    catch {...}

    throw (tmp); 
  }

The target_exprs expand cleanups to cleanup_exprs, that however are not spotted
by gimplify_cleanup_point_expr, since they are within try_catch_expr.

The reason why this does not reproduce without the test for b is that then the code
looks like

cleanup_point_expr
  {
    ...
    try
      {
        tmp = target_expr (bla1, something, cleanup1);
      }
    catch {...}

    throw (tmp); 
  }

and there is another bug in gimplify_modify_expr_rhs that causes cleanup1
to be ignored.

Fixing this seems to need a complete rewrite of gimplify_cleanup_point_expr.
Comment 10 Steven Bosscher 2006-01-21 22:46:50 UTC
Zdenek, are you working on a patch for this?
Comment 11 Zdenek Dvorak 2006-01-22 21:49:48 UTC
This ICE works this way: to build_throw, we get expression exp that looks like

TARGET_EXPR <D.2066, b ? aggr_init_expr (... t1 ...) : aggr_init_expr (... t2 ...)>

Where t1 and t2 are TARGET_EXPRs with cleanups (*).  build_throw generates code

TARGET_EXPR <D.2069, __cxa_allocate_exception (1)>;
try
  {
    *(struct logic_error *) D.2069 = exp;
  }
catch
  {
    __cxa_free_exception (D.2069);
  };
__cxa_throw (D.2069, (void *) &_ZTI11logic_error, 0B);

This is a problem, since exp (that contains cleanups) is withing the try statement,
and gimplify_cleanup_expr expects that there never is a compound statement between
cleanup and cleanup_point_expr (except for COND_EXPRs).

For comparison, with

void foo(bool b) {
      throw logic_error(1);
      }

exp is

TARGET_EXPR <D.2066, aggr_init_expr (... t1 ...)>

In this case, however, stabilize_init will allow to pre-evaluate t1, and the produced
code is (where exp' is exp with t1 replaced by tmp):

TARGET_EXPR <tmp, t1>
TARGET_EXPR <D.2069, __cxa_allocate_exception (1)>;
try
  {
    *(struct logic_error *) D.2069 = exp';
  }
catch
  {
    __cxa_free_exception (D.2069);
  };
__cxa_throw (D.2069, (void *) &_ZTI11logic_error, 0B);

Here gimplify_cleanup_expr has no problem, since t1 is outside of the try statement.

Now the question is whether we can persuade throw_expr to also evaluate
(the relevant parts of) exp outside of the try statement.  Answering this question
is far beyond my understanding of c++ frontend; stabilize_init claims that

      /* If the initializer is a COND_EXPR, we can't preevaluate
         anything.  */
      if (TREE_CODE (t) == COND_EXPR)
        return false;

which might suggest that it is not possible, but the statement is too vague for me to
be sure.

If it is somehow possible, it would be much preferable.  The other possibility is to teach
gimplify_cleanup_expr to deal with try statements, which basically means to rewrite it
completely from scratch.

--------------

(*) The exact expression is

TARGET_EXPR <D.2066
  init: bD.2024
           ? TARGET_EXPR <D.2062
               init: aggr_init_expr (__comp_ctor ;
                       0B, (struct string &) &TARGET_EXPR <D.2061
                                                init: aggr_init_expr (__comp_ctor ;
                                                        0B, 0, (struct allocator &) (struct allocator *) &TARGET_EXPR <D.2045
                                                                                                            init: {}
                                                                                                            clean: __comp_dtor  (&D.2045);
                                                                                                            >
                                                        D.2061)
                                                >
                       D.2062)
               >
           : TARGET_EXPR <D.2065
               init: aggr_init_expr (__comp_ctor ;
                       0B, (struct string &) &TARGET_EXPR <D.2064
                                                init: aggr_init_expr (__comp_ctor ;
                                                        0B, 1, (struct allocator &) (struct allocator *) &TARGET_EXPR <D.2063
                                                                                                            init: {}
                                                                                                            clean: __comp_dtor  (&D.2063);
                                                                                                            >
                                                        D.2064)
                                                >
                       D.2065)
               >;
  >;
Comment 12 Mark Mitchell 2006-01-23 05:10:57 UTC
I don't fully understand stabilize_expr; that's Jason's invention, IIRC.  However, I think that the problem is that for a COND_EXPR we can't pre-evaluate both arms of the conditional, because only one of them is supposed to be executed at runtime.

I think this is a bug in the gimplifier.  I certainly don't see anything in the documentation for CLEANUP_EXPR that suggests the restriction you've described.

I suppose that we could try to turn:

  b ? T(x) : T(y)

into:

  SAVE_EXPR b,
  TARGET_EXPR (tmp, b ? T(&tmp, x) : T(&tmp, y), b ? cleanup1 : cleanup2)

but that seems difficult, and I'm not sure it would work.

By the way, here's a simpler test case:

struct A {
  A();
  ~A();
};

struct S {
  S(const A&);
  ~S();
};

void foo(bool b)
{
  throw b ? S(A()) : throw 0;
}

Comment 13 Zdenek Dvorak 2006-01-23 08:53:09 UTC
One more question; how is the throw code supposed to work?
We allocate D.2069, initialize it, if there is a problem
during it initialization, we deallocate it, then
pass it to __cxa_throw anyway?  This does not make much
sense to me.

TARGET_EXPR <D.2069, __cxa_allocate_exception (1)>;
try
  {
    *(struct logic_error *) D.2069 = exp;
  }
catch
  {
    __cxa_free_exception (D.2069);
  };
__cxa_throw (D.2069, (void *) &_ZTI11logic_error, 0B);
Comment 14 Zdenek Dvorak 2006-01-26 23:51:35 UTC
Created attachment 10738 [details]
Possible patch
Comment 15 Zdenek Dvorak 2006-01-26 23:52:49 UTC
The patch fixes the problem by making gimplification of cleanups much more robust, and able to handle nested statements, at the expense of producing a bit worse code.
Comment 16 Richard Biener 2006-01-31 16:14:05 UTC
Can you quantify "a bit worse" code?  Also, did you bootstrap/test the patch?

Btw., now that it's properly analyzed I don't think this should be P1 - in fact
the patch looks too invasive for 4.1.0, but it should probably go to trunk and
considered for backporting to 4.1.1.
Comment 17 Mark Mitchell 2006-02-01 03:27:43 UTC
Zdenek, have you submitted the patch yet for mainline?
Comment 18 Zdenek Dvorak 2006-02-01 08:18:45 UTC
Subject: Re:  [4.0/4.1/4.2 Regression] ICE on throw code

Hello,

> Zdenek, have you submitted the patch yet for mainline?

no, I was waiting for reactions on my questions, so that I am sure

1) there is not a better way
2) the patch I propose is correct

Zdenek
Comment 19 Mark Mitchell 2006-02-01 08:21:59 UTC
Subject: Re:  [4.0/4.1/4.2 Regression] ICE on throw code

rakdver at atrey dot karlin dot mff dot cuni dot cz wrote:

>>Zdenek, have you submitted the patch yet for mainline?
> 
> no, I was waiting for reactions on my questions, so that I am sure
> 
> 1) there is not a better way
> 2) the patch I propose is correct

I'm sorry; I'm not well enough qualified to comment.  I would suggest
you post the questions and patch to the gcc-patches mailing list for
comment.

Thanks,

Comment 20 Michael Matz 2006-02-02 16:56:47 UTC
I've put the patch to testing.
Comment 21 Zdenek Dvorak 2006-02-02 17:57:26 UTC
I have posted the patch, let's see what the reactions will be.

http://gcc.gnu.org/ml/gcc-patches/2006-02/msg00146.html
Comment 22 Steven Bosscher 2006-02-09 22:59:32 UTC
Fascinating how quickly review happens for a patch for a P1 bug report blocking a release....
Comment 23 Jason Merrill 2006-02-12 07:58:59 UTC
Subject: Re:  [4.0/4.1/4.2 Regression] ICE on throw code

I think I have a better patch that I'll check in soon.

Jason
Comment 24 Jason Merrill 2006-02-12 16:02:08 UTC
Subject: Bug 24996

Author: jason
Date: Sun Feb 12 16:02:00 2006
New Revision: 110889

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=110889
Log:
        PR c++/24996
        * except.c (build_throw): Add a CLEANUP_POINT_EXPR inside the
        TRY_CATCH_EXPR or MUST_NOT_THROW_EXPR.

Added:
    trunk/gcc/testsuite/g++.dg/eh/cond3.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/except.c

Comment 25 Jason Merrill 2006-02-12 16:07:10 UTC
Subject: Bug 24996

Author: jason
Date: Sun Feb 12 16:07:04 2006
New Revision: 110890

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=110890
Log:
        PR c++/24996
        * except.c (build_throw): Add a CLEANUP_POINT_EXPR inside the
        TRY_CATCH_EXPR or MUST_NOT_THROW_EXPR.

Added:
    branches/gcc-4_1-branch/gcc/testsuite/g++.dg/eh/cond3.C
Modified:
    branches/gcc-4_1-branch/gcc/cp/ChangeLog
    branches/gcc-4_1-branch/gcc/cp/except.c

Comment 26 Andrew Pinski 2006-02-12 16:20:33 UTC
Fixed by a simplier patch.
Comment 27 Andrew Pinski 2006-02-12 17:19:13 UTC
Reopening as 4.0.x is not fixed yet.
Comment 28 Andrew Pinski 2006-02-12 17:20:22 UTC
Assigning to Jason since he patched 4.1.0 and 4.2.0.
Comment 29 Jason Merrill 2006-02-12 20:19:35 UTC
Subject: Bug 24996

Author: jason
Date: Sun Feb 12 20:19:32 2006
New Revision: 110896

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=110896
Log:
        PR c++/24996
        * except.c (build_throw): Add a CLEANUP_POINT_EXPR inside the
        TRY_CATCH_EXPR or MUST_NOT_THROW_EXPR.

Added:
    branches/gcc-4_0-branch/gcc/testsuite/g++.dg/eh/cond3.C
Modified:
    branches/gcc-4_0-branch/gcc/cp/ChangeLog
    branches/gcc-4_0-branch/gcc/cp/except.c

Comment 30 Andrew Pinski 2006-02-12 20:28:36 UTC
Fixed, thanks Jason for fixing this one.
Comment 31 Steven Bosscher 2006-02-12 23:35:33 UTC
Yes, thanks Jason.
But could you add a comment explaining what that CLEANUP_POINT_EXPR is needed for?  It obviously confused more than just a few people, or someone else would have been able to come up with this nice one-liner fix :-)