Microsoft X64 Mangling

[Richard Smith]

LGTM

As we discussed offline, we should check whether the E is present on a
pointer-to-member-function mangling (if so, maybe it should be added when
mangling the function type, not when mangling the function decl).

On Fri, May 10, 2013 at 2:37 PM, Warren Hunt <whunt at google.com> wrote:

> Addressing BNF comments request.
>
> -Warren
>
>
> On Thu, May 9, 2013 at 3:49 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>> On Thu, May 9, 2013 at 11:32 AM, Warren Hunt <whunt at google.com> wrote:
>>
>>> I updated the white space.
>>>
>>> Are these are the checks for pointers/references to functions that you
>>> were looking for? (lines 128-134 of mangle-ms-arg-qualifiers.cpp)
>>>
>>
>> Ah, yes, thanks :)
>>
>>
>>> void foo_p6ahxz(int x()) {}
>>> // CHECK: "\01?foo_p6ahxz@@YAXP6AHXZ at Z"
>>> // X64: "\01?foo_p6ahxz@@YAXP6AHXZ at Z"
>>>
>>> void foo_a6ahxz(int (&x)()) {}
>>> // CHECK: "\01?foo_a6ahxz@@YAXA6AHXZ at Z"
>>> // X64: "\01?foo_a6ahxz@@YAXA6AHXZ at Z"
>>>
>>> void foo_q6ahxz(int (&&x)()) {}
>>> // CHECK: "\01?foo_q6ahxz@@YAX$$Q6AHXZ at Z"
>>> // X64: "\01?foo_q6ahxz@@YAX$$Q6AHXZ at Z"
>>>
>>>
>>>
>>> On Wed, May 8, 2013 at 3:39 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>>
>>>> @@ -1277,6 +1284,8 @@ void
>>>> MicrosoftCXXNameMangler::mangleFunctionClass(const FunctionDecl *FD) {
>>>>          else
>>>>            Out << 'Q';
>>>>      }
>>>> +  if (PointersAre64Bit && !MD->isStatic())
>>>> +    Out << 'E';
>>>>    } else
>>>>      Out << 'Y';
>>>>  }
>>>>
>>>> Needs more indentation.
>>>>
>>>> You have special cases for pointers/references to functions (where no E
>>>> is added) but I don't see any test cases for that.
>>>>
>>>> The BNF comments should be updated in various places to indicate the
>>>> presence of this specifier. It might also be useful to factor out the "if
>>>> (PointersAre64Bit) Out << 'E';" code into a function, and make up a name
>>>> for it to use in the BNF descriptions.
>>>>
>>>
>> I'd still like these comments updated. Other than that, patch looks fine.
>>
>>
>>>  On Wed, May 8, 2013 at 1:36 PM, Warren Hunt <whunt at google.com> wrote:
>>>>
>>>>>  ping
>>>>>
>>>>>
>>>>> On Fri, May 3, 2013 at 12:07 PM, Warren Hunt <whunt at google.com> wrote:
>>>>>
>>>>>> Sounds good.  I added a local that caches the 64-bitness of pointers.
>>>>>>
>>>>>> -Warren
>>>>>>
>>>>>>
>>>>>> On Thu, May 2, 2013 at 2:51 PM, Charles Davis <cdavis5x at gmail.com>wrote:
>>>>>>
>>>>>>>
>>>>>>> On May 2, 2013, at 3:19 PM, Warren Hunt wrote:
>>>>>>>
>>>>>>> > I've put together a patch that fixes all of the issues I found
>>>>>>> with Microsoft name mangling in 64-bit mode.  Mostly it involves adding an
>>>>>>> 'E' note to all pointers that are 64-bit (including 'this' pointers).  This
>>>>>>> is done in MicrosoftMangle.cpp.  I've updated 3 of the mangling test files
>>>>>>> with X64 tests, which provides some reasonable amount of coverage.
>>>>>>> >
>>>>>>> > Please review.
>>>>>>> >
>>>>>>> > Thanks,
>>>>>>> > -Warren
>>>>>>> >
>>>>>>> > (Note: This is my first patch.)
>>>>>>> Welcome to Clang development!
>>>>>>>
>>>>>>> > @@ -1277,6 +1278,9 @@ void
>>>>>>> MicrosoftCXXNameMangler::mangleFunctionClass(const FunctionDecl *FD) {
>>>>>>> >          else
>>>>>>> >            Out << 'Q';
>>>>>>> >      }
>>>>>>> > +  if (getASTContext().getTargetInfo().getPointerWidth(0) == 64 &&
>>>>>>> > +      !MD->isStatic())
>>>>>>> > +    Out << 'E';
>>>>>>> >    } else
>>>>>>> >      Out << 'Y';
>>>>>>> >  }
>>>>>>> The indentation on this looks wrong. (You can't tell from here, but
>>>>>>> it's definitely wrong in the patch itself.) Also, I think we should handle
>>>>>>> this where we mangle in the 'this' qualifiers--the 'E' is because the
>>>>>>> 'this' pointer is Extended (__ptr64 instead of __ptr32). (The relevant line
>>>>>>> is 1172.)
>>>>>>>
>>>>>>> > @@ -1390,6 +1394,8 @@ void
>>>>>>> MicrosoftCXXNameMangler::mangleDecayedArrayType(const ArrayType *T,
>>>>>>> >      manglePointerQualifiers(T->getElementType().getQualifiers());
>>>>>>> >    } else {
>>>>>>> >      Out << 'Q';
>>>>>>> > +    if (getASTContext().getTargetInfo().getPointerWidth(0) == 64)
>>>>>>> > +      Out << 'E';
>>>>>>> >    }
>>>>>>> >    mangleType(T->getElementType(), SourceRange());
>>>>>>> >  }
>>>>>>> Why only here? What does VC do in the global case?
>>>>>>>
>>>>>>> There's a lot of code duplication (i.e. checks against the pointer
>>>>>>> size sprinkled throughout the code). Most of those can be collapsed into
>>>>>>> mangleQualifiers().
>>>>>>>
>>>>>>> Someone else wrote a patch a while back. You can find it here:
>>>>>>>
>>>>>>> http://llvm-reviews.chandlerc.com/D101
>>>>>>>
>>>>>>> I didn't finish reviewing it because real life got in the way. :\.
>>>>>>> You might find some ideas in there.
>>>>>>>
>>>>>>> Chip
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130510/a59149a4/attachment.html>