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

[John McCall]

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;

  /// 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
  };
  
>> 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 ?

I definitely don't want the CXXABI to be intimately tied to the ABIInfo classes
making extremely architecture-specific decisions.  But I don't think we have
to — we can make an abstraction (like the above) where the CXXABI tells
the ABIInfo the general pattern of what it should do, and the ABIInfo just makes
sure it implements that.

It's understood that there might be bugs in unsupported corner cases like
MS ABI on PPC, but it's good to make a general effort for consistency, so that
if (e.g.) somebody wants to add their own awesome new platform-independent
C++ ABI, they don't have to piece-by-piece abstract a bunch of stuff that should
have been abstracted properly to begin with.

Also note that there's presumably an MS ABI implementation on ARM now. :)

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

John.