[L] Change in dart/sdk[main]: [vm,dyn_modules] Handle compound returns from FfiCalls.
Tess Strickland (Gerrit)
Tess Strickland voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland added 1 comment![]( "Open in Gerrit")
Okay, fixed the missing unimplemented/unused versions of the new FfiCallWithReturnedStructTrampoline, so PTAL.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
FfiCallArguments.Could you explain why we need a new stub? Why we cannot simply set `x8` for `FfiCallTrampoline` to the area allocated in C++ code for returning a struct? Why do we need to copy returned struct value twice (in a stub and in C++ code)?
Also consider how simulator implements this with a single `SimulatorFfiCalloutTrampoline`.
Related details
- Tess Strickland
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland added 2 comments![]( "Open in Gerrit")
Could you explain why we need a new stub? Why we cannot simply set `x8` for `FfiCallTrampoline` to the area allocated in C++ code for returning a struct? Why do we need to copy returned struct value twice (in a stub and in C++ code)?
Also consider how simulator implements this with a single `SimulatorFfiCalloutTrampoline`.
Because of my own confusion early on. I did figure out what I was missing later, but forgot to go back and take that into consideration.
I've cleaned this up and it now looks much more like you'd expect. I do copy back CallingConventions::kPointerToReturnStructRegisterReturn, which the simulator doesn't do, but I mostly do it to be sure that the C side is actually well-behaved and adhering to the expected ABI instead of returning an entirely different pointer than the one we passed in to the call.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 8 comments![]( "Open in Gerrit")
ldr x8, [x19, #80] // FfiCallArguments.cpu_registers[8]Nit: why is this load moved and not loaded among other CPU registers?
str x8, [x19, #80] // FfiCallArguments.cpu_registers[8]x8 is not guaranteed to be preserved and doesn't contain any useful information, so there is no point in copying it back.
for (; current <= end; ++current) {Should this be `<` to avoid out of bounds memory access?
buffer.Printf("%2x", *current);Nit: `%02x` to pad with zeros
const Register reg = pointer_loc.AsRegisters().reg_at(0);Nit: add `ASSERT((reg >= 0) && (reg < kNumberOfCpuRegisters));`
UNIMPLEMENTED();Nit: is this really a possible but unimplemented case? Maybe turn it into an assertion?
// The pointer returned should be the same one that was passed in.
ASSERT_EQUAL(
args.stack_area + marshaller.PassByPointerStackOffset(arg_index),
args.cpu_registers[reg]);ABI does not guarantee that R8 is preserved. So we should just use `args.stack_area + marshaller.PassByPointerStackOffset(arg_index)` as a `src`.
if (portion.IsStack()) {Can this really happen according to the ABI? We're not copying stack back to `args.stack_area` after the call, so this code will likely copy garbage.
I would suggest to make this case UNREACHABLE or UNIMPLEMENTED.
Related details
- Tess Strickland
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tess Strickland (Gerrit)
Tess Strickland added 9 comments![]( "Open in Gerrit")
ldr x8, [x19, #80] // FfiCallArguments.cpu_registers[8]Nit: why is this load moved and not loaded among other CPU registers?
I was just copying it to the same place as in the simulator version to keep them consistent (and wasn't sure if sticking a `ldr` in the middle of a block of `ldp`s has some effect on performance which is why it was written afterwards in the simulator version).
str x8, [x19, #80] // FfiCallArguments.cpu_registers[8]x8 is not guaranteed to be preserved and doesn't contain any useful information, so there is no point in copying it back.
See the comment on `runtime_entry.cc:1421`, but as mentioned there, removing it for now.
Should this be `<` to avoid out of bounds memory access?
Done
Nit: `%02x` to pad with zeros
Done
const Register reg = pointer_loc.AsRegisters().reg_at(0);Nit: add `ASSERT((reg >= 0) && (reg < kNumberOfCpuRegisters));`
Done
Nit: is this really a possible but unimplemented case? Maybe turn it into an assertion?
Done
// The pointer returned should be the same one that was passed in.
ASSERT_EQUAL(
args.stack_area + marshaller.PassByPointerStackOffset(arg_index),
args.cpu_registers[reg]);ABI does not guarantee that R8 is preserved. So we should just use `args.stack_area + marshaller.PassByPointerStackOffset(arg_index)` as a `src`.
Does it not? In `constants_arm64.h`:
```
// The native ABI uses R8 to pass the pointer to the memory preallocated for
// struct return values. Arm64 is the only ABI in which this pointer is _not_
// in ArgumentRegisters[0] or on the stack.
static constexpr Register kPointerToReturnStructRegisterCall = R8;
static constexpr Register kPointerToReturnStructRegisterReturn = R8;
```
I would assume there would be a `kNoRegister` definition for `CallingConventions:: kPointerToReturnStructRegisterReturn` if the ABI didn't specify that the pointer to the compound value must be passed back to the calling function via `R8`. That's why I figured that just using the information from the location would be sufficient (and the code looks the same as how the argument case is handled, outside of using `pointer_return_location()` instead of `pointer_location()`).
(All the expected tests pass on `vm-dyn-mac-debug-arm64` in the followup CL that also includes arguments handling, so I'd expect it must be appropriately preserved, but it could also just be that those tests just happen not to generate any uses of `R8` in the native function.)
I'm not averse to using the marshaller methods here to retrieve the information, so I'll make the change you suggest and remove the use of `pointer_return_location()` and the copying back of `x8` in the trampoline, I'm just surprised the marshaller claims that's where the info is located if it isn't.
Can this really happen according to the ABI? We're not copying stack back to `args.stack_area` after the call, so this code will likely copy garbage.
I would suggest to make this case UNREACHABLE or UNIMPLEMENTED.
Ah, thank you, _that's_ why my earlier versions of this were copying the stack back, because I'd thought this case needed to be handled and swore it showed up (that's probably from it showing up in arguments, and me just backporting the changes there).
Looking at the `CompoundResultLocation` definitions in `native_calling_convention.cc`, any `Stack` locations generated for the return case get replaced with `PointertoMemory` locations instead, and the only multiple cases are CPU/FPU register-related, so I'll mark any non-`Registers`/`FpuRegisters` case as `UNIMPLEMENTED` then.
Related details
- Alexander Markov
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |