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

[Timur Iskhodzhanov]

On Wed, Jul 25, 2012 at 10:45 PM, Charles Davis <cdavis at mymail.mines.edu> wrote:
> On Jul 25, 2012, at 10:19 AM, Timur Iskhodzhanov wrote:
>> Are there any important entries in TypeNodes.td which I've skipped?
>> I looked only at TYPE lines, skipped a few things intentionally:
>> - BlockPointer's - we can mangle them the way we want as it's a Clang extension
>> - MemberPointer - not supported now anyways
>> - *Array's -> PR13182
>> - ObjC*
> This is also a Clang extension.
>> - Atomic
> Don't know how. Does VC even support C11 _Atomic?
MicrosoftMangle.cpp doesn't know about atomics at all...

>> ... and a few I don't fully undersand:
>> - Vector - ??
> Can't for the general case. For certain cases (i.e. the ones corresponding to the __mXXX types from the mmintrin headers), we can mangle it.
Probably that's a candidate for "later if needed" too.

>> - FunctionNoProto - ??
> Should never ever happen, because all functions have a prototype in C++ (if you omit the parameter list, 'void' is assumed).
Thanks for clarification!

On Wed, Jul 25, 2012 at 10:58 PM, John McCall <rjmccall at apple.com> wrote:
> 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.
r160780

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

Thanks!
--
Timur