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

[Timur Iskhodzhanov]

2013/4/13 John McCall <rjmccall at apple.com>:
> On Apr 12, 2013, at 12:38 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>> 2013/4/12 John McCall <rjmccall at apple.com>:
>>> On Apr 12, 2013, at 8:44 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>> 2013/4/9 John McCall <rjmccall at apple.com>
>>>>>
>>>>>> If that looks OK - I assume I should do the same for other ABIInfos ?
>>>>>> (I hope they are covered with tests well...)
>>>> See the attached patch where I've applied the same approach to other ABIs.
>>>>
>>>>>>> /// 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,
>>>>
>>>> OK, after re-reading the RAA_DirectInRegisters comment a few times I
>>>> now understand it's exactly the same as I proposed for RAA_Default.
>>>> Not sure why I had not seen that before...
>>>
>>> RAA_Default is fine for the name, actually.  Go with that.
>> OK, done
>>
>>>> I now use the same enum you've suggested, but I also explicitly set
>>>> the RAA_DirectInRegisters's value to 0
>>>> so now I can write like this:
>>>>
>>>> if (RecordArgABI RAA = getRecordArgABI(...))
>>>>   ABIArgInfo::getIndirect(0, RAA == CGCXXABI::RAA_DirectInMemory);
>>>> // continue if RAA == RAA_DirectInRegisters.
>>>>
>>>> Is it OK to do so?
>>>>
>>>>> and the MS ABI would use RAA_DirectInMemory if there's any non-trivial
>>>>> constructor or destructor and RAA_DirectInRegisters otherwise.
>>>> I think you've meant just "MS ABI would use RAA_DirectInMemory.",
>>>> otherwise my tests fail.
>>>
>>> Are you saying that the MS ABI should just *always* use RAA_DirectInMemory?
>> Yes, at least I haven't seen counter-examples so far, though I have
>> like 20 different tests for passing structs as arguments that actually
>> run the code and verify it works.
>> (plus 30 more for returning structs)
>
> If that's the C ABI rule, it should be handled by the C ABI, i.e. the normal
> ABIInfo handling for the type.
Sounds reasonable - aaand I've found a bad test in Clang!
See test/CodeGen/x86_32-arguments-win32.c - these structs should be
passed via stack.

> But, for example, it seems like X64 does not
> follow this rule, and will happily pass structs of size 1, 2, 4, or 8 in a register.
Good catch!
I've tested my patch on Win64 and yeah, the small structs are passed
in registers unless they have non-trivial copy constructors.
Interestingly, the presence of a destructor or operator= don't seem to
move an argrument from a register to stack (weird).

I didn't test the "class method" part of the test much on Win64 as the
mangling is broken right now and it's very inconvenient to verify
stuff there.
I'll surely get back to this when we have Win64 more working in general.

-> Fixed both things, please review.

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