Microsoft X64 Mangling

[Richard Smith]

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/20130509/8fb8a014/attachment.html>