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

[Charles Davis]

On Jul 25, 2012, at 10:19 AM, Timur Iskhodzhanov wrote:

> On Wed, Jul 25, 2012 at 8:15 PM, Timur Iskhodzhanov <timurrrr at google.com> 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 ...
>> 
>>> 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]
> 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?
> 
> ... 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.
> - FunctionNoProto - ??
Should never ever happen, because all functions have a prototype in C++ (if you omit the parameter list, 'void' is assumed).

Chip
> 
> 
>>> 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.
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits