Patchwork RFC [ABI]: C++ PATCH for c++/44540 - avoid mangling attribute noreturn

The ICE in PR 44540 called attention to a long-standing mangling bug: 
attributes noreturn and const are represented internally as 
TYPE_VOLATILE and TYPE_CONST on FUNCTION_TYPE, and we were emitting them 
as cv-quals on the mangled form of the function type in a parameter of 
pointer-to-function type.  Clearly this is wrong, as they aren't part of 
the type system, and a declaration which didn't use the attributes would 
mangle differently.

The testcase was derived from the systemtap sources, where the relevant 
declaration has C linkage, so it would not be affected by this change. 
But it is an ABI change.

Thoughts?

Jason

Jason Merrill wrote:

> The ICE in PR 44540 called attention to a long-standing mangling bug:
> attributes noreturn and const are represented internally as
> TYPE_VOLATILE and TYPE_CONST on FUNCTION_TYPE, and we were emitting them
> as cv-quals on the mangled form of the function type in a parameter of
> pointer-to-function type.

Oh, ugh.  I've always thought that using TYPE_{VOLATILE,CONST} for these
attributes was an ugly representation hack, but I didn't think about the
impact on mangling.

> The testcase was derived from the systemtap sources, where the relevant
> declaration has C linkage, so it would not be affected by this change.
> But it is an ABI change.

I guess I think this ought to require an -fabi-version=N option.  Your
change is clearly correct, but to the extent that this matters to
anyone, it is going to break existing working code.

On Wed, Jul 7, 2010 at 12:23 AM, Mark Mitchell <mark@codesourcery.com> wrote:
> Jason Merrill wrote:
>
>> The ICE in PR 44540 called attention to a long-standing mangling bug:
>> attributes noreturn and const are represented internally as
>> TYPE_VOLATILE and TYPE_CONST on FUNCTION_TYPE, and we were emitting them
>> as cv-quals on the mangled form of the function type in a parameter of
>> pointer-to-function type.
>
> Oh, ugh.  I've always thought that using TYPE_{VOLATILE,CONST} for these
> attributes was an ugly representation hack, but I didn't think about the
> impact on mangling.
>
>> The testcase was derived from the systemtap sources, where the relevant
>> declaration has C linkage, so it would not be affected by this change.
>> But it is an ABI change.
>
> I guess I think this ought to require an -fabi-version=N option.  Your
> change is clearly correct, but to the extent that this matters to
> anyone, it is going to break existing working code.

Wouldn't it be better to steal some other bits for this purpose and
"fix" it on the middle-end side?

Richard.

> --
> Mark Mitchell
> CodeSourcery
> mark@codesourcery.com
> (650) 331-3385 x713
>

Richard Guenther wrote:

>> I guess I think this ought to require an -fabi-version=N option.  Your
>> change is clearly correct, but to the extent that this matters to
>> anyone, it is going to break existing working code.
> 
> Wouldn't it be better to steal some other bits for this purpose and
> "fix" it on the middle-end side?

I think that at some point the mangling has to change.  It's valid to
declare these functions with and without an attribute.  So, we have to
do something.

I agree that an orthogonal improvement would be to stop abusing
TYPE_CONST and TYPE_VOLATILE for these attributes and record them on
TYPE_ATTRIBUTES.  But, that would be much bigger than Jason's patch.

On Wed, 7 Jul 2010, Mark Mitchell wrote:

> I agree that an orthogonal improvement would be to stop abusing
> TYPE_CONST and TYPE_VOLATILE for these attributes and record them on
> TYPE_ATTRIBUTES.  But, that would be much bigger than Jason's patch.

And, to stop sometimes putting them on a DECL but not on the type of that 
DECL; if you stop using the qualifiers as the internal representation, 
moving to recording them internally only on types should be easier.