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

[John McCall]

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.  There's
a pointer to it on the CodeGenTypes.

Please refactor all the TargetInfos to do that.

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?

John.