examples(allocation): ensure message outlives host call #1436
Conversation
Also, incorporate guidance from TinyGo to use unsafe.String rather than unsafe.SliceHeader. Signed-off-by: Nuno Cruces <[email protected]>
|
Oh jeez. This is what I get for going off grid: managed to miss #1434. |
There was a problem hiding this comment.
approved, except you want to regen the wasm with make build.examples.tinygo as it seems to be a lot bigger than before, probably from different flags.
I added notes to save the days around special casing, especially why we aren't doing it and why it is complicated to explain. Having the notes here is ok, and the only other place it might make sense is a new file examples/allocation/tinygo/RATIONALE.md or in the site doc, as the explanation is extremely weedy and also we aren't special casing anyway!
| ptr := &buf[0] | ||
| unsafePtr := uintptr(unsafe.Pointer(ptr)) | ||
| return uint32(unsafePtr), uint32(len(buf)) | ||
| ptr := unsafe.Pointer(unsafe.StringData(s)) |
There was a problem hiding this comment.
summary of a chat with @aykevl on this is that even though StringData can return nil the latter cast to uintptr is safe.
Also, special casing len(s) zero to return early with 0, 0 is both unnecessary and may slow down the code. Plus, we'd have to clarify that empty isn't always ptr 0, for example a result of a function that reslices a non-empty string to zero length. If we did early exit, we could say "the input ptr might not really be zero due to reslicing, however a read of zero bytes is noop on the host for any ptr value."
There was a problem hiding this comment.
Also, special casing
len(s)zero to return early with 0, 0 is both unnecessary and may slow down the code.
Yes, but the slowdown will probably so small it's not even noticeable. We're talking about just a few wasm instructions.
Plus, we'd have to clarify that empty isn't always ptr 0, for example a result of a function that reslices a non-empty string to zero length. If we did early exit, we could say "the input ptr might not really be zero due to reslicing, however a read of zero bytes is noop on the host for any ptr value."
One way to not have to clarify this is to return 0, 0 when len(s) == 0. That makes the interface simpler on the other end, in case people accidentally check the pointer value instead of the length.
Anyway, while both ways are correct, I think checking for len(s) == 0 will lead to a more robust API for users. But this is your call, of course, so so what you think is best.
| // compatible with WebAssembly numeric types. The pointer is not automatically | ||
| // managed by TinyGo hence it must be freed by the host. | ||
| // compatible with WebAssembly numeric types. | ||
| // The pointer is not automatically managed by TinyGo hence it must be freed by the host. | ||
| func stringToLeakedPtr(s string) (uint32, uint32) { | ||
| size := C.ulong(len(s)) |
There was a problem hiding this comment.
we aren't special casing len(s) == 0 here (to return ptr=0, size=0), and I don't recommend doing so as it is so complicated to explain. So fine as-is.
I'll explain anyway just to save the research on the special casing which was hard to track down.
Unlike stringToPtr, the host must call free on the ptr result of stringToLeakedPtr. If we returned ptr=0 manually, instead of proceeding to the logic below on len(s)==0, it would be ok even though it might actually slow the code.
When the host calls the "free" function exported by the guest with wasm i32(0), it ends up calling TinyGo's libc_free function.
TinyGo's compiler converts zero to unsafe.Pointer(nil) on all architectures, including wasm32, so this means libc_free is called with nil on i32(0), same as any other function export that has an unsafe.Pointer param. An unrelated side effect is, you can't represent memory offset 0 with an unsafe.Pointer, as it cannot be differentiated from nil.
Anyway, TinyGo's libc_free is following the C specification for free which says "ptr is NULL, no operation is performed". This means the passing zero will work not just with the default libc_free, but any other "free" export someone might have wired in.
There was a problem hiding this comment.
@ncruces mentioned that being unable to access offset 0 is a feature not a bug, and it could be a good practice to actually enforce code never tries to use offset zero.
If anyone ends up writing a reference based on these notes in TinyGo, our TinyGo site doc, tinymem RATIONALE.md, or just a personal site, this seems solid advice to consider.
There was a problem hiding this comment.
I should point out this quirk of malloc:
If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().
TinyGo will return nil for malloc(0) and I don't see a reason to change this, but there is the possibility that other heap implementations will do something different. (In fact, the internal TinyGo memory allocator runtime.alloc returns a non-nil pointer when allocating a zero-length object).
Also, unrelated:
@ncruces mentioned that being unable to access offset 0 is a feature not a bug, and it could be a good practice to actually enforce code never tries to use offset zero.
There is actually a more subtle reason to enforce this:
- Dereferencing a nil pointer will lead to a panic in TinyGo, as the Go language specification says it should.
- If you work around this (using
unsafeor whatever), you may find that LLVM also assumes nil pointers are invalid. If it sees you are dereferencing a nil pointer, it will consider this undefined behavior and act accordingly (probably by deleting code as it assumes undefined behavior is unreachable).
|
rebuilt with results |
Signed-off-by: Edoardo Vacchi <[email protected]>
|
pushing the updated wasm binary. @lburgazzoli since you don't mind we are merging this PR because the discussion is more linear. Thanks for your help 🫡 |
Also, incorporate guidance from TinyGo to use unsafe.String rather than unsafe.SliceHeader.