Re: fix for PR 4447: is this really correct?

Re: fix for PR 4447: is this really correct?

[mike stump]

> From: Joe Buck <jbuck@synopsys.com>
> To: mrs@windriver.com (mike stump)
> Date: Sat, 1 Dec 2001 16:14:30 -0800 (PST)
> Cc: jbuck@synopsys.com, gcc@gcc.gnu.org, lerdsuwa@users.sourceforge.net

> I wrote:
> > > I'd prefer to have neither the ICE nor the ABI breakage, but I'd
> > > prefer the latter to the former.

> However, it turns out that the strange mangling is exactly what the
> ABI specifies, according to Kriang, so the issue is moot.

Ok, I withdraw all my comments.  Certainly more is going on than I
ever imagined.

Re: fix for PR 4447: is this really correct?

[Mark Mitchell]

--On Monday, December 03, 2001 09:53:13 AM -0800 Joe Buck 
<jbuck@synopsys.COM> wrote:

> This issue is moot, Mark.  Kriang already pointed out that the odd
> behavior is evidently mandated by the ABI standard.
>

Good, thanks.  I wish that when I can't read mail for a few days,
everyone else would stop thinking so that there's not so much
catching up to when I get back. :-)

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

Re: fix for PR 4447: is this really correct?

[Joe Buck]

This issue is moot, Mark.  Kriang already pointed out that the odd
behavior is evidently mandated by the ABI standard.

> --On Friday, November 30, 2001 03:21:50 PM -0800 mike stump 
> <mrs@windriver.com> wrote:
> 
> >> From: Joe Buck <jbuck@synopsys.COM>
> >> To: gcc@gcc.gnu.org, lerdsuwa@users.sourceforge.net
> >> Date: Fri, 30 Nov 2001 11:20:51 -0800 (PST)
> >
> >> Mark approved, assuming the usual testing requirements are met.
> >
> > :-(
> 
> I assume that if the patch went in on the mainline it was
> already reviewed for correctness.
> 
> >> I've now verified that this fix doesn't break any C++ or libstdc++
> >> tests (other tests aren't relevant since this only affects cc1plus).
> >
> > If I understand the fix, it is worse than not having it, as it hides a
> > real bug?
> >
> >> But I am now not sure that this fix is quite correct, though it does
> >> improve things.
> >
> > I think the ICE is preferable, as otherwise you have to explain that
> > you have to break the ABI, which is worse.
> 
> Did this change affect the mangling of functions that we were already
> able to compile successfully?
> 
> --
> Mark Mitchell                   mark@codesourcery.com
> CodeSourcery, LLC               http://www.codesourcery.com
> 

Re: fix for PR 4447: is this really correct?

[Mark Mitchell]

--On Friday, November 30, 2001 03:21:50 PM -0800 mike stump 
<mrs@windriver.com> wrote:

>> From: Joe Buck <jbuck@synopsys.COM>
>> To: gcc@gcc.gnu.org, lerdsuwa@users.sourceforge.net
>> Date: Fri, 30 Nov 2001 11:20:51 -0800 (PST)
>
>> Mark approved, assuming the usual testing requirements are met.
>
> :-(

I assume that if the patch went in on the mainline it was
already reviewed for correctness.

>> I've now verified that this fix doesn't break any C++ or libstdc++
>> tests (other tests aren't relevant since this only affects cc1plus).
>
> If I understand the fix, it is worse than not having it, as it hides a
> real bug?
>
>> But I am now not sure that this fix is quite correct, though it does
>> improve things.
>
> I think the ICE is preferable, as otherwise you have to explain that
> you have to break the ABI, which is worse.

Did this change affect the mangling of functions that we were already
able to compile successfully?

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

Re: fix for PR 4447: is this really correct?

[Mark Mitchell]

> I think it's not so important for 3.0.3.  However, I plan to submit an
> implementation for 3.1.

I didn't think reinterpret_cast was legal in a template parameter.

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

Re: fix for PR 4447: is this really correct?

[Kriang Lerdsuwanakij]

Joe Buck wrote:
> 
> OK.  Mark had approved applying this patch to the 3_0_release branch,
> so I guess it is the thing to do.  I don't understand why the ABI is
> written this way, though.  It seems odd, and since the expression must
> be constant, I don't see why it was thought better to leave it unevaluated.
> 
> But, are you confident that all operators are now covered, or is it
> possible that there's another ICE lurking out there if a user tries a
> complicated expression?
> 
> Joe

The only operator missing is the reinterpret_cast operator.  Handling it
is somewhat messy especially when converting pointers and references - 
like mangling reinterpret_cast'ing pointer should be encoded as casting 
to void *  followed by casting to the desired type (in conformance with 
functional equivalent rule in 14.5.5.1 para 5-8).  

I think it's not so important for 3.0.3.  However, I plan to submit an 
implementation for 3.1.

--Kriang

Re: fix for PR 4447: is this really correct?

[Alexandre Oliva]

On Dec  1, 2001, Kriang Lerdsuwanakij <lerdsuwa@users.sourceforge.net> wrote:

> I have checked the ABI specifications
> (http://www.codesourcery.com/cxx-abi/abi.html)
> before starting on this patch.  And this bloat mangling behavior is
> what the ABI intended.

Right.  The C++ Standard paragraph referenced in that paragraph of the
ABI requires this behavior in the mangling of overloads of function
templates.  Constant folding is still applied to template arguments
*referenced* within the definition of the template function, before
overload resolution (in case of references to template functions) or
specialization resolution (in case of template classes), and encodings
of such referenced symbols may be constant-folded.  But the template
function names must enable one to tell the sequence of tokens used in
the template function definition, except for template argument names.

> (C++ Standard reference 14.5.5.1 p. 5.)

-- 
Alexandre Oliva   Enjoy Guarana', see http://www.ic.unicamp.br/~oliva/
Red Hat GCC Developer                  aoliva@{cygnus.com, redhat.com}
CS PhD student at IC-Unicamp        oliva@{lsd.ic.unicamp.br, gnu.org}
Free Software Evangelist    *Please* write to mailing lists, not to me

Re: fix for PR 4447: is this really correct?

[Joe Buck]

I wrote:
> > I'd prefer to have neither the ICE nor the ABI breakage, but I'd
> > prefer the latter to the former.

However, it turns out that the strange mangling is exactly what the ABI
specifies, according to Kriang, so the issue is moot.

> > News flash: 3.1 will have a couple of minor ABI bug fixes
> 
> :-(  Such is life.
> 
> > so it seems that we're already in a position to break the ABI,
> 
> We don't just doom 3.1 to breaking, but some random future version of
> the compiler, with luck, it will just be 3.1.

We are converging to a written spec, which is supposed to be supported
by multiple compilers.  If we leave the 3.0 deviations from this spec,
we don't keep our word about implementing the spec.  We had an argument
about whether we should have fixed the first error discovered in 3.0.1,
and we decided to wait until 3.1.  Just as well, since one more was
found.  In practice, the main 3.0 -> 3.1 incompatibility will be 
in the standard library.

Re: fix for PR 4447: is this really correct?

[Joe Buck]

Kriang writes:
> I have checked the ABI specifications 
> (http://www.codesourcery.com/cxx-abi/abi.html)
> before starting on this patch.  And this bloat mangling behavior is what 
> the ABI intended.

OK.  Mark had approved applying this patch to the 3_0_release branch,
so I guess it is the thing to do.  I don't understand why the ABI is
written this way, though.  It seems odd, and since the expression must
be constant, I don't see why it was thought better to leave it unevaluated.

But, are you confident that all operators are now covered, or is it
possible that there's another ICE lurking out there if a user tries a
complicated expression?

Joe

Re: fix for PR 4447: is this really correct?

[Kriang Lerdsuwanakij]

Hi
I have checked the ABI specifications 
( http://www.codesourcery.com/cxx-abi/abi.html )
before starting on this patch.  And this bloat mangling behavior is what 
the ABI intended.
Following is from the specification with relevant clause capitalized:

   An expression, e.g., "B<(J+1)/2>", is encoded with a prefix traversal of 
the operators
   involved, delimited by "X...E". The operators are encoded using their 
two letter mangled
   names. For example, "B<(J+1)/2>", if J is the third template parameter, 
becomes
   "1BI Xdv pl T1_ Li1E Li2E E E" (the blanks are present only to visualize 
the decomposition).
   Note that the expression is mangled WITHOUT CONSTANT FOLDING OR OTHER
   SIMPLIFICATION, and without parentheses, which are implicit in the 
prefix representation.
   Except for the parentheses, therefore, it represents the source token 
stream. (C++ Standard
   reference 14.5.5.1 p. 5.)

In fact, expressions (involving any operator except type casts) are already 
mangled that way
without my patch.  I can't think of an example that causes ambiguity if 
constant folding is
performed but it looks possible :)

--Kriang

At 11:20 30/11/01 -0800, Joe Buck wrote:
>But I am now not sure that this fix is quite correct, though it does
>improve things.
>
>Here's the testcase:
>----------------------------------------------
>template<bool B,int I>
>class T {
>public:
>     T(int);
>};
>
>template<bool B1,bool B2,int I1,int I2>
>T<(B1&&B2),I1+I2-int(B1&&B2)>
>func(T<B1,I1> a, T<B2,I2> b) {
>     return T<(B1&&B2),I1+I2-int(B1&&B2)> (I1+I2);
>}
>
>void foo(T<true,3> a, T<true,4>b) {
>     func(a, b);
>}
>----------------------------------------------
>
>gcc 3.0 through 3.0.2 get an ICE in the name mangler.  With Kriang's
>patch, the code compiles, but doing
>
>nm -p crash.o | c++filt
>
>on Solaris gives
>
>
>0000000000 T foo(T<true, 3>, T<true, 4>)
>0000000000 T T<(true)&&(true), ((3)+(4))-(operator int((true)&&(true)))> 
>func<true, true, 3, 4>(T<true, 3>, T<true, 4>)
>
>rather than
>
>0000000000 T foo(T<true, 3>, T<true, 4>)
>0000000000 T T<true, 6> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)
>
>as I would expect.  It may be that the oddball expression for the return
>type will not affect correctness and will just contribute to bloat instead
>(for the purpose of "linkonce" directives, instantiations that are really
>the same will look different).  But it looks wrong.
>
>Comments?  I may still want this for 3.0.3, because it does make some
>cases that ICE'd before work correctly (in my fixed-point computation
>examples the relevant functions are inline so these oddball symbols
>probably won't even appear).

Re: fix for PR 4447: is this really correct?

[mike stump]

> From: Joe Buck <jbuck@synopsys.com>
> To: mrs@windriver.com (mike stump)
> Date: Fri, 30 Nov 2001 16:20:32 -0800 (PST)

> > > But I am now not sure that this fix is quite correct, though it does
> > > improve things.
> > 
> > I think the ICE is preferable, as otherwise you have to explain that
> > you have to break the ABI, which is worse.

> I'd prefer to have neither the ICE nor the ABI breakage, but I'd
> prefer the latter to the former.

Personally, I didn't think it would be possible to maintain the abi,
and I previously said as much.  We can get close, but it is fairly
hard.  We have to decide, create more abi headaches for the future
now, or not.  The benefit of the headache, is, more programs can be
compiled.  It we are very serious about the abi, the answer must be
no.  If we are not as serious about it, we can put the fix into the
compiler, and create the abi headache.  I leave the final decision to
those folks that want to make it.  I just wanted to point out the
consequence of the action and ensure that everone knew that we were
going to purposefully create a new abi incompatibility that didn't
previously exist.

> Yes.  Maybe it's possible to fix the bug by applying a
> constant-folding operation to template arguments before the mangler
> is called.

They should be folded way early.

> News flash: 3.1 will have a couple of minor ABI bug fixes

:-(  Such is life.

> so it seems that we're already in a position to break the ABI,

We don't just doom 3.1 to breaking, but some random future version of
the compiler, with luck, it will just be 3.1.

Re: fix for PR 4447: is this really correct?

[Joe Buck]

I wrote:
> > Mark approved, assuming the usual testing requirements are met.

Mike Stump writes:
> :-(

Note that this is already in the trunk, and I'm temporarily withdrawing
my request to put this into 3.0.3 until we understand the consequences
better.  I overcame the temptation to slip it in anyway. :-)

> > I've now verified that this fix doesn't break any C++ or libstdc++
> > tests (other tests aren't relevant since this only affects cc1plus).
> 
> If I understand the fix, it is worse than not having it, as it hides a
> real bug?

Well, there is a bug that is being hidden, but that's not the same as
saying that it's worse than not having the fix.  The problem is that
there's no way to work around this ICE without degrading performance
in my application (attempting to speed up a class library that does
fixed-point arithmetic).

> > But I am now not sure that this fix is quite correct, though it does
> > improve things.
> 
> I think the ICE is preferable, as otherwise you have to explain that
> you have to break the ABI, which is worse.

I'd prefer to have neither the ICE nor the ABI breakage, but I'd prefer
the latter to the former.

> > on Solaris gives
> 
> > 0000000000 T foo(T<true, 3>, T<true, 4>)
> > 0000000000 T T<(true)&&(true), ((3)+(4))-(operator int((true)&&(true)))> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)
> 
> Ick!  Looks like a bug.

Yes.  Maybe it's possible to fix the bug by applying a constant-folding
operation to template arguments before the mangler is called.

> > Comments?  I may still want this for 3.0.3, because it does make
> > some cases that ICE'd before work correctly
> 
> Is that working correctly?  I don't think so.  From an, I don't care
> about the abi, yes, it works fine, but from an, gosh, what do you mean
> you totally broke the abi, I thought you said you weren't going to do
> that, it looks like absolute horror.

News flash: 3.1 will have a couple of minor ABI bug fixes, so it seems
that we're already in a position to break the ABI, despite our promise.
The cases in question are corner cases, so they may not have a substantial
effect, but they exist.

In this case, the generated symbol will be exactly the symbol that is
called, so even if the bug is fixed I think that a 3.0 <-> 3.1+fix
link will work.

Re: fix for PR 4447: is this really correct?

[mike stump]

> From: Joe Buck <jbuck@synopsys.COM>
> To: gcc@gcc.gnu.org, lerdsuwa@users.sourceforge.net
> Date: Fri, 30 Nov 2001 11:20:51 -0800 (PST)

> Mark approved, assuming the usual testing requirements are met.

:-(

> I've now verified that this fix doesn't break any C++ or libstdc++
> tests (other tests aren't relevant since this only affects cc1plus).

If I understand the fix, it is worse than not having it, as it hides a
real bug?

> But I am now not sure that this fix is quite correct, though it does
> improve things.

I think the ICE is preferable, as otherwise you have to explain that
you have to break the ABI, which is worse.

> on Solaris gives

> 0000000000 T foo(T<true, 3>, T<true, 4>)
> 0000000000 T T<(true)&&(true), ((3)+(4))-(operator int((true)&&(true)))> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)

Ick!  Looks like a bug.

> Comments?  I may still want this for 3.0.3, because it does make
> some cases that ICE'd before work correctly

Is that working correctly?  I don't think so.  From an, I don't care
about the abi, yes, it works fine, but from an, gosh, what do you mean
you totally broke the abi, I thought you said you weren't going to do
that, it looks like absolute horror.

fix for PR 4447: is this really correct?

[Joe Buck]

I wrote:

> I'd like to see the fix to PR #4447 back-ported from the trunk to 3.0.3.
> Actually I don't think any porting is needed, as it's a fix to mangle.c,
> which hasn't had many changes; no, it doesn't change the ABI because
> hitting the relevant code caused a core dump before.
>
> This PR is an ICE and a regression from 2.95.x.

> I believe that the relevant patch is
> 
> 2001-11-17  Kriang Lerdsuwanakij  <lerdsuwa@users.sourceforge.net>
> 
> 	* mangle.c (write_expression): Handle CAST_EXPR, STATIC_CAST_EXPR,
> 	CONST_CAST_EXPR.
> 	* operators.def: Add CAST_EXPR, STATIC_CAST_EXPR, CONST_CAST_EXPR.

Mark approved, assuming the usual testing requirements are met.

I've now verified that this fix doesn't break any C++ or libstdc++ tests
(other tests aren't relevant since this only affects cc1plus).

But I am now not sure that this fix is quite correct, though it does
improve things.

Here's the testcase:
----------------------------------------------
template<bool B,int I>
class T {
public:
    T(int);
};

template<bool B1,bool B2,int I1,int I2>
T<(B1&&B2),I1+I2-int(B1&&B2)> 
func(T<B1,I1> a, T<B2,I2> b) {
    return T<(B1&&B2),I1+I2-int(B1&&B2)> (I1+I2);
}

void foo(T<true,3> a, T<true,4>b) {
    func(a, b);
}
----------------------------------------------

gcc 3.0 through 3.0.2 get an ICE in the name mangler.  With Kriang's
patch, the code compiles, but doing

nm -p crash.o | c++filt

on Solaris gives


0000000000 T foo(T<true, 3>, T<true, 4>)
0000000000 T T<(true)&&(true), ((3)+(4))-(operator int((true)&&(true)))> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)

rather than

0000000000 T foo(T<true, 3>, T<true, 4>)
0000000000 T T<true, 6> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)

as I would expect.  It may be that the oddball expression for the return
type will not affect correctness and will just contribute to bloat instead
(for the purpose of "linkonce" directives, instantiations that are really
the same will look different).  But it looks wrong.

Comments?  I may still want this for 3.0.3, because it does make some
cases that ICE'd before work correctly (in my fixed-point computation
examples the relevant functions are inline so these oddball symbols
probably won't even appear).

Re: fix for PR 4447: is this really correct?

[Kriang Lerdsuwanakij]

Hi
I have checked the ABI specifications 
(http://www.codesourcery.com/cxx-abi/abi.html)
before starting on this patch.  And this bloat mangling behavior is what 
the ABI intended.
Following is from the specification with relevant clause capitalized:

   An expression, e.g., "B<(J+1)/2>", is encoded with a prefix traversal of 
the operators
   involved, delimited by "X...E". The operators are encoded using their 
two letter mangled
   names. For example, "B<(J+1)/2>", if J is the third template parameter, 
becomes
   "1BI Xdv pl T1_ Li1E Li2E E E" (the blanks are present only to visualize 
the decomposition).
   Note that the expression is mangled WITHOUT CONSTANT FOLDING OR OTHER
   SIMPLIFICATION, and without parentheses, which are implicit in the 
prefix representation.
   Except for the parentheses, therefore, it represents the source token 
stream. (C++ Standard
   reference 14.5.5.1 p. 5.)

In fact, expressions (involving any operator except type casts) are already 
mangled that way
without my patch.  I can't think of an example that causes ambiguity if 
constant folding is
performed but it looks possible :)

--Kriang

At 11:20 30/11/01 -0800, Joe Buck wrote:
>But I am now not sure that this fix is quite correct, though it does
>improve things.
>
>Here's the testcase:
>----------------------------------------------
>template<bool B,int I>
>class T {
>public:
>     T(int);
>};
>
>template<bool B1,bool B2,int I1,int I2>
>T<(B1&&B2),I1+I2-int(B1&&B2)>
>func(T<B1,I1> a, T<B2,I2> b) {
>     return T<(B1&&B2),I1+I2-int(B1&&B2)> (I1+I2);
>}
>
>void foo(T<true,3> a, T<true,4>b) {
>     func(a, b);
>}
>----------------------------------------------
>
>gcc 3.0 through 3.0.2 get an ICE in the name mangler.  With Kriang's
>patch, the code compiles, but doing
>
>nm -p crash.o | c++filt
>
>on Solaris gives
>
>
>0000000000 T foo(T<true, 3>, T<true, 4>)
>0000000000 T T<(true)&&(true), ((3)+(4))-(operator int((true)&&(true)))> 
>func<true, true, 3, 4>(T<true, 3>, T<true, 4>)
>
>rather than
>
>0000000000 T foo(T<true, 3>, T<true, 4>)
>0000000000 T T<true, 6> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)
>
>as I would expect.  It may be that the oddball expression for the return
>type will not affect correctness and will just contribute to bloat instead
>(for the purpose of "linkonce" directives, instantiations that are really
>the same will look different).  But it looks wrong.
>
>Comments?  I may still want this for 3.0.3, because it does make some
>cases that ICE'd before work correctly (in my fixed-point computation
>examples the relevant functions are inline so these oddball symbols
>probably won't even appear).

Re: fix for PR 4447: is this really correct?

[mike stump]

> From: Joe Buck <jbuck@synopsys.com>
> To: mrs@windriver.com (mike stump)
> Date: Fri, 30 Nov 2001 16:20:32 -0800 (PST)

> > > But I am now not sure that this fix is quite correct, though it does
> > > improve things.
> > 
> > I think the ICE is preferable, as otherwise you have to explain that
> > you have to break the ABI, which is worse.

> I'd prefer to have neither the ICE nor the ABI breakage, but I'd
> prefer the latter to the former.

Personally, I didn't think it would be possible to maintain the abi,
and I previously said as much.  We can get close, but it is fairly
hard.  We have to decide, create more abi headaches for the future
now, or not.  The benefit of the headache, is, more programs can be
compiled.  It we are very serious about the abi, the answer must be
no.  If we are not as serious about it, we can put the fix into the
compiler, and create the abi headache.  I leave the final decision to
those folks that want to make it.  I just wanted to point out the
consequence of the action and ensure that everone knew that we were
going to purposefully create a new abi incompatibility that didn't
previously exist.

> Yes.  Maybe it's possible to fix the bug by applying a
> constant-folding operation to template arguments before the mangler
> is called.

They should be folded way early.

> News flash: 3.1 will have a couple of minor ABI bug fixes

:-(  Such is life.

> so it seems that we're already in a position to break the ABI,

We don't just doom 3.1 to breaking, but some random future version of
the compiler, with luck, it will just be 3.1.

Re: fix for PR 4447: is this really correct?

[Joe Buck]

I wrote:
> > Mark approved, assuming the usual testing requirements are met.

Mike Stump writes:
> :-(

Note that this is already in the trunk, and I'm temporarily withdrawing
my request to put this into 3.0.3 until we understand the consequences
better.  I overcame the temptation to slip it in anyway. :-)

> > I've now verified that this fix doesn't break any C++ or libstdc++
> > tests (other tests aren't relevant since this only affects cc1plus).
> 
> If I understand the fix, it is worse than not having it, as it hides a
> real bug?

Well, there is a bug that is being hidden, but that's not the same as
saying that it's worse than not having the fix.  The problem is that
there's no way to work around this ICE without degrading performance
in my application (attempting to speed up a class library that does
fixed-point arithmetic).

> > But I am now not sure that this fix is quite correct, though it does
> > improve things.
> 
> I think the ICE is preferable, as otherwise you have to explain that
> you have to break the ABI, which is worse.

I'd prefer to have neither the ICE nor the ABI breakage, but I'd prefer
the latter to the former.

> > on Solaris gives
> 
> > 0000000000 T foo(T<true, 3>, T<true, 4>)
> > 0000000000 T T<(true)&&(true), ((3)+(4))-(operator int((true)&&(true)))> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)
> 
> Ick!  Looks like a bug.

Yes.  Maybe it's possible to fix the bug by applying a constant-folding
operation to template arguments before the mangler is called.

> > Comments?  I may still want this for 3.0.3, because it does make
> > some cases that ICE'd before work correctly
> 
> Is that working correctly?  I don't think so.  From an, I don't care
> about the abi, yes, it works fine, but from an, gosh, what do you mean
> you totally broke the abi, I thought you said you weren't going to do
> that, it looks like absolute horror.

News flash: 3.1 will have a couple of minor ABI bug fixes, so it seems
that we're already in a position to break the ABI, despite our promise.
The cases in question are corner cases, so they may not have a substantial
effect, but they exist.

In this case, the generated symbol will be exactly the symbol that is
called, so even if the bug is fixed I think that a 3.0 <-> 3.1+fix
link will work.

Re: fix for PR 4447: is this really correct?

[mike stump]

> From: Joe Buck <jbuck@synopsys.COM>
> To: gcc@gcc.gnu.org, lerdsuwa@users.sourceforge.net
> Date: Fri, 30 Nov 2001 11:20:51 -0800 (PST)

> Mark approved, assuming the usual testing requirements are met.

:-(

> I've now verified that this fix doesn't break any C++ or libstdc++
> tests (other tests aren't relevant since this only affects cc1plus).

If I understand the fix, it is worse than not having it, as it hides a
real bug?

> But I am now not sure that this fix is quite correct, though it does
> improve things.

I think the ICE is preferable, as otherwise you have to explain that
you have to break the ABI, which is worse.

> on Solaris gives

> 0000000000 T foo(T<true, 3>, T<true, 4>)
> 0000000000 T T<(true)&&(true), ((3)+(4))-(operator int((true)&&(true)))> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)

Ick!  Looks like a bug.

> Comments?  I may still want this for 3.0.3, because it does make
> some cases that ICE'd before work correctly

Is that working correctly?  I don't think so.  From an, I don't care
about the abi, yes, it works fine, but from an, gosh, what do you mean
you totally broke the abi, I thought you said you weren't going to do
that, it looks like absolute horror.

fix for PR 4447: is this really correct?

[Joe Buck]

I wrote:

> I'd like to see the fix to PR #4447 back-ported from the trunk to 3.0.3.
> Actually I don't think any porting is needed, as it's a fix to mangle.c,
> which hasn't had many changes; no, it doesn't change the ABI because
> hitting the relevant code caused a core dump before.
>
> This PR is an ICE and a regression from 2.95.x.

> I believe that the relevant patch is
> 
> 2001-11-17  Kriang Lerdsuwanakij  <lerdsuwa@users.sourceforge.net>
> 
> 	* mangle.c (write_expression): Handle CAST_EXPR, STATIC_CAST_EXPR,
> 	CONST_CAST_EXPR.
> 	* operators.def: Add CAST_EXPR, STATIC_CAST_EXPR, CONST_CAST_EXPR.

Mark approved, assuming the usual testing requirements are met.

I've now verified that this fix doesn't break any C++ or libstdc++ tests
(other tests aren't relevant since this only affects cc1plus).

But I am now not sure that this fix is quite correct, though it does
improve things.

Here's the testcase:
----------------------------------------------
template<bool B,int I>
class T {
public:
    T(int);
};

template<bool B1,bool B2,int I1,int I2>
T<(B1&&B2),I1+I2-int(B1&&B2)> 
func(T<B1,I1> a, T<B2,I2> b) {
    return T<(B1&&B2),I1+I2-int(B1&&B2)> (I1+I2);
}

void foo(T<true,3> a, T<true,4>b) {
    func(a, b);
}
----------------------------------------------

gcc 3.0 through 3.0.2 get an ICE in the name mangler.  With Kriang's
patch, the code compiles, but doing

nm -p crash.o | c++filt

on Solaris gives


0000000000 T foo(T<true, 3>, T<true, 4>)
0000000000 T T<(true)&&(true), ((3)+(4))-(operator int((true)&&(true)))> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)

rather than

0000000000 T foo(T<true, 3>, T<true, 4>)
0000000000 T T<true, 6> func<true, true, 3, 4>(T<true, 3>, T<true, 4>)

as I would expect.  It may be that the oddball expression for the return
type will not affect correctness and will just contribute to bloat instead
(for the purpose of "linkonce" directives, instantiations that are really
the same will look different).  But it looks wrong.

Comments?  I may still want this for 3.0.3, because it does make some
cases that ICE'd before work correctly (in my fixed-point computation
examples the relevant functions are inline so these oddball symbols
probably won't even appear).