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

[John McCall]

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.

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.

John.