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

[John McCall]

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.

>>>> 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.