[ms-cxxabi] Set proper SRet flags for most functions; also handle empty struct arguments correctly
Timur Iskhodzhanov
timurrrr at google.com
Thu Mar 28 11:10:15 PDT 2013
ping?
2013/3/27 Timur Iskhodzhanov <timurrrr at google.com>:
> 2013/3/26 Timur Iskhodzhanov <timurrrr at google.com>:
>> 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?
> Or just like isReturnTypeIndirect() / isArgumentTypeIndirect() ?
>
>> What should the interface method return, an ABIArgInfo?
>>
>>> There's a pointer to it on the CodeGenTypes.
>> Thanks for the hint!
>> Just for my own reference, CGT is defined in ABIInfo and CGCXXABI is
>> available as "CGT.getCXXABI()".
>>
>>> 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 ?
>>
>>> 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.
>>
>>> John.
More information about the cfe-commits
mailing list