[cfe-commits] [Windows] Use thiscall as the default calling convention for class methods

[Timur Iskhodzhanov]

On Thu, Jul 12, 2012 at 12:33 AM, John McCall <rjmccall at apple.com> wrote:
> On Jul 11, 2012, at 1:29 PM, Timur Iskhodzhanov wrote:
>> On Wed, Jul 11, 2012 at 10:00 PM, John McCall <rjmccall at apple.com> wrote:
>>> On Jul 11, 2012, at 4:53 AM, Timur Iskhodzhanov wrote:
>>>> On Mon, Jul 9, 2012 at 9:46 PM, John McCall <rjmccall at apple.com> wrote:
>>>>> On Jul 9, 2012, at 6:52 AM, Timur Iskhodzhanov wrote:
>>>>>> On Sat, Jul 7, 2012 at 10:44 AM, John McCall <rjmccall at apple.com> wrote:
>>>>>>> On Jun 28, 2012, at 1:39 PM, Timur Iskhodzhanov wrote:
>>>>>>>> Part one is attached - it propagates the "isInsance" boolean to the
>>>>>>>> two places where it should be read afterwards [in part 2].
>>>>>>>
>>>>>>> I didn't really like this approach, so I committed a different one as r159893.
>>>>>>> Let me know if this helps.
>>>>>> You've meant r159894.
>>>>>> Yeah, I like your approach much more than mine :)
>>>>>>
>>>>>>>> The only logic change is around lib/CodeGen/CGCall.cpp:150.
>>>>>>>> Without this change, the definition of `void __cdecl cdecl_method() {}`
>>>>>>>> ends up using "thiscall" (and the call remains "cdecl" - ouch!).
>>>>>>>> Probably it was a bug in the first place.
>>>>>>>
>>>>>>> Let me know if this is still a problem arising in your second patch.
>>>>>> It still is!
>>>>>> See the FIXME in the attached patch.
>>>>>> Other than that, the tests pass OK and it was easy to achieve this.
>>>>>
>>>>> Aha, that's interesting.  Okay, I think the isVariadic bit should probably be
>>>>> passed down to getDefaultMethodCallConv().  While you're at it, could
>>>>> you rename that to getDefaultCXXMethodCallConv()?
>>>> Done.
>>>> Actually, I've found and fixed a mangler bug when adding this argument :)
>>>> See the attached patch
>>>
>>> Looks good, thanks!
>> bug_12785p2_2.patch is OK
>
> Yes.
>
>> On Wed, Jul 11, 2012 at 9:57 PM, John McCall <rjmccall at apple.com> wrote:
>>> On Jul 11, 2012, at 5:34 AM, Timur Iskhodzhanov wrote:
>>>> With the attached patch the cdecl test passes now.
>>>>
>>>> However, I'm not very sure what I'm doing here and why it doesn't work
>>>> in the first place - can you please take a look?
>>>
>>> Hmm.  The problem here is that getCanonicalCallConv is turning CC_C
>>> into CC_Default.  That needs to be disabled in Microsoft mode, just like
>>> it apparently is in MRTD mode.
>>>
>>> ...also, the way that MRTD mode is implemented is wrong, and we should
>>> be handling this in IR-generation the same that we're handling the
>>> thiscall rules.
>> a) bug_12785p3.patch is OK as a short-term solution and
>>    getCanonicalCallConv can be fixed later
>> OR
>> b) I should just fix getCanonicalCallConv as a part of my patch?
>>
>> If (b) - should I fix it completely (with fixing "is implemented wrong")
>> or just alter one condition in the Microsoft mode?
>> I'm not sure it's clear to me how to fix it "completely" at this point
>> (to be honest, I haven't looked at its code yet - just checking e-mail)
>
> (b).  Just alter the one condition in Microsoft mode;  this will make it
> so that function types that are explicitly cdecl are canonically different
> from function types that do not have an explicit CC.  This should make
> your proposed patch unnecessary.
Got it.
Attached is bug_12785p3_2.patch which does pretty much what you've
asked for instead of bug_12785p3.patch
For some reason, LangOpts.MicrosoftMode is false with "-cxx-abi
microsoft", so I'm just checking for the ABI.
I can change that post-commit if that doesn't sound good for you.

> If you want to fix the existing MRTD stuff, I'd be happy to provide guidance,
> but that's not really "on you", so to speak.
I can do this if you believe that'd benefit me (like knowing the
codebase better).
Otherwise, I have a lot of "-cxx-abi microsoft" work to do :)

> John.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_12785p3_2.patch
Type: application/octet-stream
Size: 1738 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120712/634bcb14/attachment.obj>