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

[Timur Iskhodzhanov]

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?

>>>>> 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.
>>>
>>> Right now, LLVM's byval definitely does not promise to not move the argument.
>> Intersting. Can you come up with an example when this is a concern?
>> [I assume it doesn't block this patch, just want to have a heads-up
>> understanding of what's next]
>
> Any class which is not address-invariant — for example, if it potentially
> contains a pointer to one of its members, or it registers its address with
> some other object.  IIRC, the former is common in std::string implementations.
>
> John.