[cfe-commits] [Windows] Fix PR13389 - wrong mangling of return type qualifiers

[Timur Iskhodzhanov]

On Mon, Jul 23, 2012 at 10:00 PM, John McCall <rjmccall at apple.com> wrote:
> On Jul 20, 2012, at 7:16 AM, Timur Iskhodzhanov wrote:
>> See attached:
>> I've generated some more test cases for more coverage - all are OK
>> with the bug_13389_2 patch.
>> Also slightly re-factored the code to be ... well, a bit more readable.
>>
>> Some thoughts:
>> I've noticed there are a few places in the code like this:
>> -------------
>>  if (!PointeeTy.hasQualifiers())
>>    // Lack of qualifiers is mangled as 'A'.
>>    Out << 'A';
>> -------------
>> which cover Pointer and Reference types.
>>
>> That said, when there are no qualifiers,
>> a) Pointer and Reference types add 'A' (already OK)
>> b) built-ins shouldn't have extra 'A' (already OK)
>> c) the others seem to be needing 'A'.
>>
>> So there are two ways to fix (c):
>> 1) Add an extra 'A' when mangling return types (see patch; works OK)
>>   This roughly follows the logic of Pointer/Reference return type mangling.
>> 2) Always add extra 'A' when mangling
>>  non-builtin-non-pointer-non-reference type (e.g. in mangleQualifiers).
>>    I'm pretty sure (2) will break something else.
>>    I've tried a few things (like refactoring) here and there but the
>>    deeper I went the more broken it was, so I've currently abandoned it.
>
> Okay, so it sounds like the rule we have so far is this:  when we're
> mangling a type as the pointee type of a pointer or reference, or as a
> return type, and the type is unqualified, and it isn't a builtin type, then
> we should add 'A'.
Yes

> There are two things to investigate here that will help illuminate how
> to design the solution.
>
> First, is the rule really "not a builtin type", or is it "is a class type"?  For
> example, if you have a const char **, is "const char *" mangled with an A?
> It doesn't look like this is tested right now.
Added a test for this ...

> It should be pretty easy to
> explore this with MSVC by checking out the manglings of references to
> every possible type kind.  Maybe we've already checked this for a few
> types, but it's easy to flesh the list out.  You should literally be scanning
> down TypeNodes.td and testing every thing you can. :)
... as well as a few tests for enums, structs and function pointers.
Only "const function_pointer* foo()" fails right now,
most likely the same cause as
http://llvm.org/bugs/show_bug.cgi?id=13444 which I wanted to address
separately.

See the attached patch [no code change, only new tests]

> Second, this seems like a pretty long list of places where we need an 'A'.
> It seems like it's more likely to be the general rule, and there are maybe
> some exceptions (parameter types?).  I would suggest adding a required
> parameter for whether to use it to the qualified-type-mangling routine;
> then, as you fix the ensuing compiler errors, you can come up with
> a declaration that would send an unqualified type through that path.
I think this is what I've tried before but found myself digging way too deep.
If possible, I'd like to postpone this until it is really needed (see
also PR13182 where something like this is needed) or at least work on
this as a separate patch.

> John.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_13389_4.patch
Type: application/octet-stream
Size: 5580 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120725/732dcb16/attachment.obj>