PR 8134: C++ crash

PR 8134: C++ crash

[Mark Mitchell]

PR 8134 is a crash in force_store_init_value on the branch; it is a
regression.

The root problem here is that we were not handling zero-initialization
of pointers to members correctly.  Alexandre tried to fix the problem,
but in the process introduced the crashes above.  This was resolved on
the mainline with a rather substantial reworking of class layout code;
that was my patch to create a separate base class variant of each type.

What should we do on the branch?

The obvious choices are:

a) Nothing

   In this case, stuff blows up badly.

b) Move my changes over.

   As far as we know, these are correct -- but they are substantial,
   and therefore risky.

c) Revert Alexandre's patch.

   In this case, we get back to GCC 3.0-like behavior; incorrect
   zero-initialization of some pointers-to-members.

I don't like any of these choices.  I think I lean towards (c),
merely as a "devil you know" kind of choice.  I can also do (b), if
people feel that's the right thing, but I'm nervous about somehow
making an inadvertant ABI change in the minor release.

Thoughts?

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

Re: PR 8134: C++ crash

[Gabriel Dos Reis]

Mark Mitchell <mark@codesourcery.com> writes:

[...]

| b) Move my changes over.
| 
|    As far as we know, these are correct -- but they are substantial,
|    and therefore risky.

I would vote for (b) for the fix.

-- Gaby

Re: PR 8134: C++ crash

[Nathan Sidwell]

Mark Mitchell wrote:
> PR 8134 is a crash in force_store_init_value on the branch; it is a
> regression.

> a) Nothing
> b) Move my changes over.
> c) Revert Alexandre's patch.
None of these are ideal are they :) Was Alexandre's patch fixing a
regression? If it was, then (c) is not really better than (a), either
way we reintroduce a regression. Though (c) has the slight advantage
of not changing the regression.

If the patch is not fixing a regression, then (c) is the best of the
bunch. Mark, you're the one who really knows how big (b) is, but as
an observer, it seems rather large and risky for a dot release.

nathan

-- 
Dr Nathan Sidwell   ::   http://www.codesourcery.com   ::   CodeSourcery LLC
          'But that's a lie.' - 'Yes it is. What's your point?'
nathan@codesourcery.com : http://www.cs.bris.ac.uk/~nathan/ : nathan@acm.org

Re: PR 8134: C++ crash

[Mark Mitchell]

--On Tuesday, October 15, 2002 07:48:06 PM +0100 Nathan Sidwell 
<nathan@codesourcery.com> wrote:

> Mark Mitchell wrote:
>> PR 8134 is a crash in force_store_init_value on the branch; it is a
>> regression.
>
>> a) Nothing
>> b) Move my changes over.
>> c) Revert Alexandre's patch.

Jason wishes there was another choice, but thus far hasn't been able
to come up with one.

Gaby and Benjamin think that my large patch is bug-free.

Nathan isn't so optimistic.

I'm not so optimistic either.  I know my new patch is right in concept,
but I'm not sure it's bugless.  I don't think it's prudent to move over
a patch that substantial when it hasn't received more than a week or two
of testing.

So, I'm going to go with (c).  I'll be updating the caveats HTML page
with information about this.  The good news is that pointers-to-members
are rare, programs relying on zero-initialization of them are rarer,
and there is always a work-around -- explicitly initialize the
pointer-to-member.

I hope people use pointers-to-members a lot on IPF.  The whole deal here
is that we're trying to save an addition per use of a pointer to data
member on a machine that doesn't have base+offset addressing.  I have
this bad feeling that I've spent more time implementing this stuff than
will ever be saved by all the programs in the world running for all time
on architectures where this will help...)

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

Re: PR 8134: C++ crash

[Nathan Sidwell]

Mark Mitchell wrote:
[option (c)]

Another data point. We didn't move over Jason's and my virtual base patches
from the mainline to the 3.0.x branch, for fear of incompatibility.
*Even though we knew they fixed KDE builds.*

I think those patches are comparable with your patch.

nathan


-- 
Dr Nathan Sidwell   ::   http://www.codesourcery.com   ::   CodeSourcery LLC
          'But that's a lie.' - 'Yes it is. What's your point?'
nathan@codesourcery.com : http://www.cs.bris.ac.uk/~nathan/ : nathan@acm.org

Re: PR 8134: C++ crash

[Mark Mitchell]

--On Wednesday, October 16, 2002 07:48:03 PM +0100 Nathan Sidwell 
<nathan@codesourcery.com> wrote:

> Mark Mitchell wrote:
> [option (c)]
>
> Another data point. We didn't move over Jason's and my virtual base
> patches from the mainline to the 3.0.x branch, for fear of
> incompatibility. *Even though we knew they fixed KDE builds.*
>
> I think those patches are comparable with your patch.

I agree.  Thanks for reminding me of the precedent.

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

Re: PR 8134: C++ crash

[Jason Merrill]

Is

  d) fix the crash in a different way on the branch

not feasible?

Jason

Re: PR 8134: C++ crash

[Mark Mitchell]

--On Tuesday, October 15, 2002 05:45:18 PM -0400 Jason Merrill 
<jason@redhat.com> wrote:

> Is
>
>   d) fix the crash in a different way on the branch
>
> not feasible?

It depends.

Can you fix it a different way in a way that looks more reliable?

I can't.  I tried, on the mainline.

The basic problem is that you need to generate a static intializer
(i.e., a CONSTRUTOR to put in DECL_INITIAL) that has entries for
all the places you have pointers-to-members.  But, some of them
have no corresponding FIELD_DECL in your class; they're from base
classes, and we didn't create FIELD_DECLs for base classes before
my patch.

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

Re: PR 8134: C++ crash

[Benjamin Kosnik]

... as specified, there is only one fix:

> b) Move my changes over.
>
>  As far as we know, these are correct -- but they are substantial,
>  and therefore risky.

I'm for this. 

You can try the 'make check-abi' to see if any library changes are
introduced. This is not especially helpful for this issue, probably.

The other option would be for you to run your C++ ABI testers against
the patched gcc-3_2-branch code, and let us know if it works. 

I don't have to feel the holes, mate. I'll believe you if you say it works.

-benjamin

Re: PR 8134: C++ crash

[Reichelt]

Hi,

FYI: PR 7911 seems to be same problem as PR 8134. So there's not
only one user who trips over that bug.

Greetings,
Volker Reichelt