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

[Timur Iskhodzhanov]

John,
I've decided to split the patch into two to simplify the review.

Part one is attached - it propagates the "isInsance" boolean to the
two places where it should be read afterwards [in part 2].

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.

Repeating the question I originally had:
  Can you please tell me if the extra isInstance bool parameter to
  arrangeFunctionType is fine or I should rather add this boolean
  into FunctionType::ExtInfo and set it in AST?

--
Timur

On Thu, May 17, 2012 at 7:49 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> Attached is an updated version.
>
> It's not clear to me if I did the right thing near CGCall.cpp:136 and :160
> I don't fully understand what was going on there but without these two
> changes the explicitly-__cdecl function definition's ExtInfo had its
> CC reset to CC_Default. This in turn forced my code at :353 to make
> the wrong decision to use thiscall instead.
> I'd be glad if you could check if I did stuff correctly there.
>
> One more question - what do you think about this extra "bool isInstance" ?
> Maybe this flag should be stored in ExtInfo and initialized in AST instead?
>
> [ btw, I've never mentioned this review is about fixing
> http://llvm.org/bugs/show_bug.cgi?id=12785 ]
>
> --
> Thanks,
> Timur
>
> On Tue, May 15, 2012 at 10:34 PM, John McCall <rjmccall at apple.com> wrote:
>> On May 15, 2012, at 11:09 AM, Timur Iskhodzhanov wrote:
>>> On Fri, May 11, 2012 at 9:55 PM, John McCall <rjmccall at apple.com> wrote:
>>>> On May 11, 2012, at 6:58 AM, Timur Iskhodzhanov wrote:
>>>>> Can you please review the attached patch?
>>>>>
>>>>> In two words what it does is
>>>>> it overrides the CC_Default calling convention with
>>>>> context.getDefaultMethodCallConv()
>>>>> for all the used method definitions and calls.
>>>>>
>>>>> What I personally don't like is that the method declarations are
>>>>> stored with CC_Default and we have to replace that with
>>>>> context.getDefaultMethodCallConv() wherever we use it.
>>>>> Unfortunately, we can't change the CC in the global declarations storage.
>>>>> I'd prefer to put the declarations into the storage with the right CC
>>>>> but not sure:
>>>>> a) if we should do it
>>>>> b) how to do it
>>>>> c) it's possible at all
>>>>
>>>> I don't think modifying the AST is the right thing to do here anyway;
>>>> CC_Default is really the right way to model this.
>>> ok
>>>
>>>>> OTOH, it looks like there are only two places we need to alter the
>>>>> calling convention: method call and method definition :)
>>>>> So maybe I'm trying too hard to generalize...
>>>> I mostly like your patch, but I think it'd be slightly better to make the
>>>> terminal arrangeFunctionType take some sort of "function kind"
>>>> parameter (function vs. C++ instance method) and then have
>>>> ClangCallConvToLLVMCallConv consider that when mapping
>>>> CC_Default.
>>>>
>>>> Also, please put this in a more generically-named test, something like
>>>> microsoft-abi-methods.cpp.  I'm sure there will be other changes that
>>>> we'll want to test for as time goes on.
>>> I got your point.
>>> I'll try doing that - I've started with tests and this has
>>> uncovered a few bugs in ctor/dtor handling.
>>> Will update as soon as it's fixed.
>>
>> Thanks!
>>
>>>> Also, please test:
>>>>  - declarations of instance methods using CC_Default
>>> you mean, declarations without explicitly specified CC ?
>>
>> Yes.
>>
>>>>  - declarations of instance methods with an explicit CC
>>> Would you prefer __thiscall/__cdecl or __attribute__ ?
>>
>> If we predefine __thiscall in MS mode, then testing that way is fine.
>>
>> John.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_12785p1.patch
Type: application/octet-stream
Size: 5080 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120628/cadf5d25/attachment.obj>