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

[John McCall]

On Jul 25, 2012, at 9:15 AM, Timur Iskhodzhanov wrote:
> 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 ...

Okay.  So it does look like the query really is "is a fundamental type";
a pointer to non-const pointer does not get the 'A'.

>> 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]

Looks fine.

>> 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.

Okay.

John.