[ms-cxxabi] Set proper SRet flags for most functions; also handle empty struct arguments correctly

[Timur Iskhodzhanov]

2013/3/26 Timur Iskhodzhanov <timurrrr at google.com>:
> 2013/3/26 John McCall <rjmccall at apple.com>:
>> On Mar 26, 2013, at 6:27 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>> 2013/3/26 Timur Iskhodzhanov <timurrrr at google.com>:
>>>> 2013/3/22 John McCall <rjmccall at apple.com>:
>>>>>
>>>>> On Mar 21, 2013, at 1:45 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>>>
>>>>>> Hi John,
>>>>>>
>>>>>> Please see the attached patch.
>>>>>>
>>>>>> It addresses most of the Clang-side change needed to fix http://llvm.org/PR13676
>>>>>
>>>>> +  bool IsMicrosoftABI = getContext().getTargetInfo().getCXXABI().isMicrosoft();
>>>>> +
>>>>>   if (isAggregateTypeForABI(RetTy)) {
>>>>>     if (const RecordType *RT = RetTy->getAs<RecordType>()) {
>>>>>       // Structures with either a non-trivial destructor or a non-trivial
>>>>>       // copy constructor are always indirect.
>>>>> -      if (hasNonTrivialDestructorOrCopyConstructor(RT))
>>>>> +      if (hasNonTrivialDestructorOrCopyConstructor(RT) ||
>>>>> +          (IsMicrosoftABI && hasNonTrivialDefaultConstructor(RT)))
>>>>>
>>>>> Please don't compute IsMicrosoftABI before it's needed.
>>>> Done.
>>>>
>>>>> Are you sure it's just that the *default* constructor is non-trivial, or is it
>>>>> the presence of *any* non-trivial constructor, or possibly even any
>>>>> explicit constructor?
>>>> I've made some more experiments and it seems the condition is rather
>>>> "is this a POD?", e.g. we should use SRet if A has a virtual function
>>>> or a destructor or an assignment operator (but not "void
>>>> operator=(int);") or a private member or a base class.
>>
>> Interesting, okay.  I guess we'll assume that that means the C++98
>> definition of POD.
>>
>>>> See the attached updated patch.
>>>>
>>>> I was a bit lazy to write tests for all these cases, will add them
>>>> later if we ever find new incompatibilities.
>>>> I've also added a couple of tests for the upcoming byval compatibility
>>>> cleanup with FIXMEs.
>>
>> +      bool IsMicrosoftABI = getContext().getTargetInfo().getCXXABI().isMicrosoft();
>>        // Structures with either a non-trivial destructor or a non-trivial
>>        // copy constructor are always indirect.
>> -      if (hasNonTrivialDestructorOrCopyConstructor(RT))
>> +      if (hasNonTrivialDestructorOrCopyConstructor(RT) ||
>> +          (IsMicrosoftABI && !isPOD(RT)))
>>
>> Let's just abstract this decision into the C++ ABI.  In both the argument
>> and the return-type cases, when you're working with a RecordType and
>> the decl is a CXXRecordDecl, just ask the current CGCXXABI.
> Should I abstract out only the isAggregateTypeForABI() branches or the
> classifyReturnType/classifyArgumentType functions completely?
Or just like isReturnTypeIndirect() / isArgumentTypeIndirect() ?

> What should the interface method return, an ABIArgInfo?
>
>> There's a pointer to it on the CodeGenTypes.
> Thanks for the hint!
> Just for my own reference, CGT is defined in ABIInfo and CGCXXABI is
> available as "CGT.getCXXABI()".
>
>> Please refactor all the TargetInfos to do that.
> You mean, in all the subclassesof ABIInfo, including ARM, PPC, etc?
> I hope they are covered well with tests? :)
>
> Most of the TargetInfos will only work with ItaniumCXXABI, right?
> How will ItaniumCXXABI decide what to do differently in e.g. X86 vs ARM vs PPC ?
>
>> Incidentally, IIRC the MS ABI does pass non-POD objects byval — that is,
>> it passes them on the stack, not as a pointer to an object on the stack.  LLVM
>> doesn't really support that, because it assumes that it can do extra memcpys
>> of byval arguments.  Are you doing work on the LLVM side to make this work?
> I'm working on the SRet support from LLVM;
> I haven't seen a need for any specific LLVM handling of byvals so far,
> will be aware of possible problems.
>
>> John.