[Bug objc/101718] New: Objective-C frontend emits wrong code to call methods returning _Complex types returned in memory

[Bug objc/101718] New: Objective-C frontend emits wrong code to call methods returning _Complex types returned in memory

[mhjacobson at me dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

            Bug ID: 101718
           Summary: Objective-C frontend emits wrong code to call methods
                    returning _Complex types returned in memory
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: objc
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mhjacobson at me dot com
  Target Milestone: ---

When a method returns a type that the platform ABI says is returned in memory,
the Objective-C frontend (for the NeXT runtime ABIs) emits a call to
`objc_msgSend_stret`; the `_stret` variant of the message dispatcher knows to
look at the *second* argument for `self`, and so on, since the first argument
is the pointer to where the return value will go.

On some platforms, `_Complex double` is too large to be returned through
registers, so it's returned through memory.

But the Objective-C frontend still insists on using `objc_msgSend`, not
`objc_msgSend_stret`.

`objc_msgSend` is thoroughly confused when the first argument, which it expects
to be `self`, is actually a pointer to garbage-filled stack.

I believe this happens because this check:

```
  /* If we are returning a struct in memory, and the address
     of that memory location is passed as a hidden first
     argument, then change which messenger entry point this
     expr will call.  NB: Note that sender_cast remains
     unchanged (it already has a struct return type).  */
  if (!targetm.calls.struct_value_rtx (0, 0)
      && (TREE_CODE (ret_type) == RECORD_TYPE
          || TREE_CODE (ret_type) == UNION_TYPE)
      && targetm.calls.return_in_memory (ret_type, 0))
    {
```

limits the use of `stret` to record/union types.  Primitive types that are
returned in memory should also be made to use `stret` (the name's meaning
notwithstanding).

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

--- Comment #1 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Matt Jacobson from comment #0)
> When a method returns a type that the platform ABI says is returned in
> memory, the Objective-C frontend (for the NeXT runtime ABIs) emits a call to
> `objc_msgSend_stret`; the `_stret` variant of the message dispatcher knows
> to look at the *second* argument for `self`, and so on, since the first
> argument is the pointer to where the return value will go.
> 
> On some platforms, `_Complex double` is too large to be returned through
> registers, so it's returned through memory.

can you give me some idea of the platform? it would help with testing at least
(assuming that there is a suitable machine on the cfarm).

Having said that, there could be some types for which this would fire even on
Darwin.

> But the Objective-C frontend still insists on using `objc_msgSend`, not
> `objc_msgSend_stret`.
> 
> `objc_msgSend` is thoroughly confused when the first argument, which it
> expects to be `self`, is actually a pointer to garbage-filled stack.
> 
> I believe this happens because this check:
> 
> ```
>   /* If we are returning a struct in memory, and the address
>      of that memory location is passed as a hidden first
>      argument, then change which messenger entry point this
>      expr will call.  NB: Note that sender_cast remains
>      unchanged (it already has a struct return type).  */
>   if (!targetm.calls.struct_value_rtx (0, 0)
>       && (TREE_CODE (ret_type) == RECORD_TYPE
> 	  || TREE_CODE (ret_type) == UNION_TYPE)
>       && targetm.calls.return_in_memory (ret_type, 0))
>     {
> ```
> 
> limits the use of `stret` to record/union types.  Primitive types that are
> returned in memory should also be made to use `stret` (the name's meaning
> notwithstanding).

makes sense, but the fix might be interesting given the "NB: Note that
sender_cast remains unchanged (it already has a struct return type)." which
would no longer be true for a primitive type.

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[mhjacobson at me dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

--- Comment #2 from Matt Jacobson <mhjacobson at me dot com> ---
(In reply to Iain Sandoe from comment #1)
> can you give me some idea of the platform? it would help with testing at
> least (assuming that there is a suitable machine on the cfarm).

Sure, but I'm guessing it's not going to be very helpful!  The platform I'm
using is an AVR microcontroller, with my own custom NeXT-v2-ABI-compliant
runtime.

<https://mjacobson.net/blog/2021-07-objc-avr.html>
<https://github.com/mhjacobson/avr-objc>

> Having said that, there could be some types for which this would fire even
> on Darwin.

Hm -- I'm not so sure.

The x86_64 calling convention allocates *two* (64-bit) registers to
integer/pointer return values, so `__int128` doesn't require memory. 
Similarly, it can return `_Complex double` via multiple SSE registers. 
Finally, there's also `_Complex long double`, which is returned via the x87
stack (and for which there is yet another messenger routine,
`objc_msgSend_fp2ret`).

In fact, a close reading of the ABI spec suggests that the *only* types that
would ever be returned through memory are structs and unions.  So the code here
would work.

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-08-16
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #3 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Matt Jacobson from comment #2)
> (In reply to Iain Sandoe from comment #1)
> > can you give me some idea of the platform? it would help with testing at
> > least (assuming that there is a suitable machine on the cfarm).
> 
> Sure, but I'm guessing it's not going to be very helpful!  The platform I'm
> using is an AVR microcontroller, with my own custom NeXT-v2-ABI-compliant
> runtime.
> 
> <https://mjacobson.net/blog/2021-07-objc-avr.html>
> <https://github.com/mhjacobson/avr-objc>

As you say, maybe not immediately helpful - but coul even so.

> > Having said that, there could be some types for which this would fire even
> > on Darwin.
> 
> Hm -- I'm not so sure.

It is possible to find an example - if AVX is disabled, GCC returns __m256 in
memory (so can be tried for manual testing).  However, the clang version of
this on Darwin platforms actually returns in xmm0/xmm1 (which has an ABI bug
filed against it on LLVM IIRC) - so not useful outside manual testing.

My guess is that there are no [other] non-struct cases for the powerpc(64b) and
x86 cases, I've not poked at the aarch64 case yet, so the bug went unnoticed
all this time.

initial patch under test now.

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |iains at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
Created attachment 51310
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51310&action=edit
trial fix

So it seems that the logic was slightly off, but in a way that would not fire
unless a non-aggregate return-in-memory occurred, and those are very rare
(non-existent, with the clang ABI) for Darwin which is the only NeXT v2 user
with test coverage.

the change is:
  if (ret_type && !VOID_TYPE_P (ret_type)
      && targetm.calls.return_in_memory (ret_type, 0)
      && !(targetm.calls.struct_value_rtx (0, 0)
           && (TREE_CODE (ret_type) == RECORD_TYPE
               || TREE_CODE (ret_type) == UNION_TYPE)))

So this says, "if there's a return value and it's returned in memory and the
target makes no special provision for returning aggregates then use the _stret
versions".

Probably some scope for factoring some of that code - but bug fixes first.

Does this work for your platform?

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Iain D Sandoe <iains@gcc.gnu.org>:

https://gcc.gnu.org/g:1cef3039b880a21fbdf4153e6fc42026619fd4ad

commit r12-3292-g1cef3039b880a21fbdf4153e6fc42026619fd4ad
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Mon Aug 16 21:22:13 2021 +0100

    Objective-C, NeXT: Fix messenging non-aggregate return-in-memory.

    When a method returns a type that the platform ABI says should be
    returned in memory, and that is done by a hidden 'sret' parameter,
    the message send calls must be adjusted to inform the runtime that
    the sret parameter is present.  As reported in the PR, this is not
    working for non-aggregate types that use this mechanism.  The fix
    here is to adjust the logic such that all return values that flag
    'in memory' are considered to use the mechanism *unless* they
    provide a struct_value_rtx *and* the return object is an aggregate.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

    PR objc/101718 - Objective-C frontend emits wrong code to call methods
returning scalar types returned in memory

            PR objc/101718

    gcc/objc/ChangeLog:

            * objc-next-runtime-abi-02.c (build_v2_build_objc_method_call):
            Revise for cases where scalar objects use an sret parameter.
            (next_runtime_abi_02_build_objc_method_call): Likwise.

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Iain D Sandoe
<iains@gcc.gnu.org>:

https://gcc.gnu.org/g:903c18c65c4eb135eb3c67a3c14fb6c20f537feb

commit r10-10807-g903c18c65c4eb135eb3c67a3c14fb6c20f537feb
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Mon Aug 16 21:22:13 2021 +0100

    Objective-C, NeXT: Fix messenging non-aggregate return-in-memory.

    When a method returns a type that the platform ABI says should be
    returned in memory, and that is done by a hidden 'sret' parameter,
    the message send calls must be adjusted to inform the runtime that
    the sret parameter is present.  As reported in the PR, this is not
    working for non-aggregate types that use this mechanism.  The fix
    here is to adjust the logic such that all return values that flag
    'in memory' are considered to use the mechanism *unless* they
    provide a struct_value_rtx *and* the return object is an aggregate.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

    PR objc/101718 - Objective-C frontend emits wrong code to call methods
returning scalar types returned in memory

            PR objc/101718

    gcc/objc/ChangeLog:

            * objc-next-runtime-abi-02.c (build_v2_build_objc_method_call):
            Revise for cases where scalar objects use an sret parameter.
            (next_runtime_abi_02_build_objc_method_call): Likwise.

    (cherry picked from commit 1cef3039b880a21fbdf4153e6fc42026619fd4ad)

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

--- Comment #7 from Iain Sandoe <iains at gcc dot gnu.org> ---
still needed on 11.x.

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

--- Comment #8 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Iain D Sandoe
<iains@gcc.gnu.org>:

https://gcc.gnu.org/g:106cfc476a55c7423dca23be1eb0a5fb5da736b5

commit r11-11302-g106cfc476a55c7423dca23be1eb0a5fb5da736b5
Author: Iain Sandoe <iain@sandoe.co.uk>
Date:   Mon Aug 16 21:22:13 2021 +0100

    Objective-C, NeXT: Fix messenging non-aggregate return-in-memory.

    When a method returns a type that the platform ABI says should be
    returned in memory, and that is done by a hidden 'sret' parameter,
    the message send calls must be adjusted to inform the runtime that
    the sret parameter is present.  As reported in the PR, this is not
    working for non-aggregate types that use this mechanism.  The fix
    here is to adjust the logic such that all return values that flag
    'in memory' are considered to use the mechanism *unless* they
    provide a struct_value_rtx *and* the return object is an aggregate.

    Signed-off-by: Iain Sandoe <iain@sandoe.co.uk>

    PR objc/101718 - Objective-C frontend emits wrong code to call methods
returning scalar types returned in memory

            PR objc/101718

    gcc/objc/ChangeLog:

            * objc-next-runtime-abi-02.c (build_v2_build_objc_method_call):
            Revise for cases where scalar objects use an sret parameter.
            (next_runtime_abi_02_build_objc_method_call): Likwise.

    (cherry picked from commit 1cef3039b880a21fbdf4153e6fc42026619fd4ad)

[Bug objc/101718] Objective-C frontend emits wrong code to call methods returning scalar types returned in memory

[iains at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101718

Iain Sandoe <iains at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #9 from Iain Sandoe <iains at gcc dot gnu.org> ---
fixed on open branches (and gcc-10)