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

Timur Iskhodzhanov timurrrr at google.com
Tue Apr 9 11:17:48 PDT 2013 

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.

We may add the RAA_DirectInRegisters and/or RAA_DirectInMemory when we
need to and/or have better understanding of how different ABIs work (I
don't want to break Itanium ABI on x86-32 now :)).
Please tell me if you have any concerns about my approach.

If all OK, I'll do the same for other ABIInfos.
(hoping for a good test coverage)
Should the switch be extracted out as a method of ABIInfo?

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

> 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.
I'm afraid they'll need to do some abstraction either way as we have
some code that has Itanium-ABI-only assumptions and gradually rewrite
it into something that is aware of the Itanium+Microsoft ABIs. I agree
we can make life easier for then but it won't be a super-easy walk.

> Also note that there's presumably an MS ABI implementation on ARM now. :)
Oh, I won't be surprised if the difference between MS-ABI-ARM and
MS-ABI is much much larger than that between Itanium-ABI-ARM and
Itanium-ABI...

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

Thanks!

> John.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_13676_p1_4.patch
Type: application/octet-stream
Size: 12624 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130409/b087db88/attachment.obj>

More information about the cfe-commits mailing list