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

[John McCall]

On Apr 9, 2013, at 12:37 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:

> 2013/4/9 John McCall <rjmccall at apple.com>:
>> On Apr 9, 2013, at 11:17 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>> 2013/3/28 John McCall <rjmccall at apple.com>:
>>>> On Mar 26, 2013, at 7:23 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>>> 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?
>>>>> What should the interface method return, an ABIArgInfo?
>>>> 
>>>> I'm thinking:
>>>> 
>>>> bool isReturnTypeIndirect(const CXXRecordDecl *) const;
>>> Done for X86_32ABIInfo, please see the attached patch.
>>> 
>>> If that looks OK - I assume I should do the same for other ABIInfos ?
>>> (I hope they are covered with tests well...)
>>> 
>>>> /// How should we pass a particular record type as an argument?
>>>> enum RecordArgABI {
>>>>   /// Pass it using the normal C aggregate rules for the ABI, potentially
>>>>   /// introducing extra copies and passing some or all of it in registers.
>>>>   RAA_DirectInRegisters,
>>>> 
>>>>   /// Pass it on the stack using its defined layout.  The argument must be
>>>>   /// evaluated directly into the correct stack position in the arguments area,
>>>>   /// and the call machinery must not move it or introduce extra copies.
>>>>   RAA_DirectInMemory,
>>>> 
>>>>   /// Pass it as a pointer to temporary memory.
>>>>   RAA_Indirect
>>>> };
>>> I'm not sure I can effectively use the first two cases right now and
>>> there should also be RAA_IndirectByval (see what I do for Microsoft
>>> ABI), so I've created a slightly simpler enum.
>> 
>> Despite how it looks in IR, "byval" is direct in memory.
>> 
>> Just to be clear, the Itanium ABI would use RAA_Indirect if the copy constructor
>> or destructor is non-trivial and RAA_DirectInRegisters otherwise, and the
>> MS ABI would use RAA_DirectInMemory if there's any non-trivial constructor or
>> destructor and RAA_DirectInRegisters otherwise.
> Oh, I see.
> Will do the change.
> 
> Is everything else OK?
> Should I need another round of review when I change the other spots to
> use these two new CGCXXABI methods?

Yeah, I haven't looked at the rest of your patch;  I'll just review the new one
when it's ready.

John.