wrenCall -> foreign call causes memory corruption about wren HOT 9 OPEN

Quick question: Did you run in debug (assertions are currently only active in debug, which guards against some misplaced uses).

One other thing to try: call wrenEnsureSlots later. You should likely read the values for your arguments and receiver (slot 0-N) before calling ensure slots.

Edit: also thanks for the detailed post and reproduction ✨

from wren.

Quick question: Did you run in debug (assertions are currently only active in debug, which guards against some misplaced uses).

I tried enabling debug assertions with #define DEBUG in wren_common.h:
I don't see any different results with that however. It doesn't seem like it catches any assertions.

One other thing to try: call wrenEnsureSlots later. You should likely read the values for your arguments and receiver (slot 0-N) before calling ensure slots.

I just tried this: I read all the arguments out of slots first.

But even with the list processing commented out, simply that wrenEnsureSlots call alone seems to cause all the issues.

static void fcall_cool_func_impl(WrenVM* vm) {
  // read the arguments to make a CoolMeta struct
  // if we can parse successfully, return true

  // get name
  size_t arg_name_slot = 1;
  assert_msg(wrenGetSlotType(vm, arg_name_slot) == WREN_TYPE_STRING, "name must be a string");
  const char* name = wrenGetSlotString(vm, arg_name_slot);

  // get platforms
  size_t arg_platforms_slot = 2;
  assert_msg(wrenGetSlotType(vm, arg_platforms_slot) == WREN_TYPE_LIST, "platforms must be a list");
  WrenHandle* platforms_list = wrenGetSlotHandle(vm, arg_platforms_slot);
  std::vector<std::string> platforms;

  // get hashes
  size_t arg_hashes_slot = 3;
  assert_msg(wrenGetSlotType(vm, arg_hashes_slot) == WREN_TYPE_MAP, "hashes must be a map");
  WrenHandle* hashes_map = wrenGetSlotHandle(vm, arg_hashes_slot);
  std::unordered_map<std::string, std::string> hashes;

  // ensure that we have plenty of slots
  wrenEnsureSlots(vm, WREN_NEEDED_SLOTS);
  size_t slot_temp = 10;

  // size_t platforms_count = wrenGetListCount(vm, arg_platforms_slot);
  // for (size_t i = 0; i < platforms_count; i++) {
  //   size_t platform_list_el_slot = slot_temp;
  //   wrenGetListElement(vm, arg_platforms_slot, i, platform_list_el_slot);
  //   assert_msg(wrenGetSlotType(vm, platform_list_el_slot) == WREN_TYPE_STRING, "platform must be a string");
  //   const char* platform = wrenGetSlotString(vm, platform_list_el_slot);
  //   platforms.push_back(platform);
  // }

  // release handles
  wrenReleaseHandle(vm, platforms_list);
  wrenReleaseHandle(vm, hashes_map);

  // return true
  wrenSetSlotBool(vm, 0, true);
}

Edit: also thanks for the detailed post and reproduction ✨

You're welcome! I like to make your job easier, I know what it's like to be on your end :)

from wren.

from wren.

Not quite right,

// When your foreign function is called, you are given one slot for the receiver
// and each argument to the method. The receiver is in slot 0 and the arguments
// are in increasingly numbered slots after that. You are free to read and
// write to those slots as you want. If you want more slots to use as scratch
// space, you can call wrenEnsureSlots() to add more.

...

// Ensures that the foreign method stack has at least [numSlots] available for
// use, growing the stack if needed.
//
// Does not shrink the stack if it has more than enough slots.
//
// It is an error to call this from a finalizer.
WREN_API void wrenEnsureSlots(WrenVM* vm, int numSlots);

This API is and has always been FOR foreign functions use to add more to the scratch space for your methods. I have several thousand foreign methods using this function this way in my engine just fine.

There are times when using it can definitely create problems, though, and needs to be addressed with either proper assertions or a fix. e.g if you call it in a finalizer and outside of a foreign.

So reproducible examples like this are plenty helpful in isolating stuff.

from wren.

I was also under the impression that it was explicitly stated in the documentation that wrenEnsureSlots is for use in foreign functions.

In the meantime, is there an alternative way to accomplish the same thing?
One idea that looked promising as a hacky workaround was to pass in dummy parameters to provide additional temp slots. I don't like this however.

As for diagnosing this issue: Valgrind showed pretty clearly that Wren tries to reallocate stuff when I try to wrenEnsureSlots which then leads to invalid reads.

from wren.

from wren.

So, I did a little bit of looking into this.

Current code

I added some logs to wren, and here are the results for the current code:

wrenNewFiber: fiber->stackCapacity = 4
wrenEnsureStack: fiber->stackCapacity = 4, needed = 5
wrenEnsureStack: stack capacity is insufficient, growing
wrenEnsureStack: new capacity = 8
wrenEnsureStack: reallocating stack
wrenEnsureStack: fiber->stackCapacity = 8, needed = 6
wrenEnsureStack: fiber->stackCapacity = 8, needed = 9
wrenEnsureStack: stack capacity is insufficient, growing
wrenEnsureStack: new capacity = 16
wrenEnsureStack: reallocating stack
foreign call test
wren: ensure slots
wrenEnsureSlots: numSlots=12
wrenEnsureSlots: creating fiber
wrenNewFiber: fiber->stackCapacity = 1
wrenEnsureSlots: currentSize=0
wrenEnsureSlots: current size is insufficient, resizing
wrenEnsureSlots: wrenEnsureStack(needed=12)
wrenEnsureStack: fiber->stackCapacity = 1, needed = 12
wrenEnsureStack: stack capacity is insufficient, growing
wrenEnsureStack: new capacity = 16
wrenEnsureStack: reallocating stack
wren: get handle to Script class
wren: set slot 0 to Script class
wren: call Script.run()
wrenEnsureStack: fiber->stackCapacity = 16, needed = 2
wrenEnsureStack: fiber->stackCapacity = 16, needed = 4
wrenEnsureStack: fiber->stackCapacity = 16, needed = 6
wrenEnsureStack: fiber->stackCapacity = 16, needed = 9
script is now running
wrenEnsureStack: fiber->stackCapacity = 16, needed = 4
wrenEnsureStack: fiber->stackCapacity = 16, needed = 5
wrenEnsureStack: fiber->stackCapacity = 16, needed = 11
wrenEnsureStack: fiber->stackCapacity = 16, needed = 6
wrenEnsureStack: fiber->stackCapacity = 16, needed = 8
wrenEnsureStack: fiber->stackCapacity = 16, needed = 11
fcall_cool_func_impl: enter
wrenEnsureSlots: numSlots=12
wrenEnsureSlots: currentSize=4
wrenEnsureSlots: current size is insufficient, resizing
wrenEnsureSlots: wrenEnsureStack(needed=18)
wrenEnsureStack: fiber->stackCapacity = 16, needed = 18
wrenEnsureStack: stack capacity is insufficient, growing
wrenEnsureStack: new capacity = 32
wrenEnsureStack: reallocating stack
wren: Script.run() must return a boolean, but it returned 7
assertion failed: Script.run() returned invalid type

Notes

As we can see above, the wrenEnsureSlots(WREN_NEEDED_SLOTS) I do before the wrenCall ends up growing the stack to size 16. However, my call to the very same thing within the foreign function results in requesting a stack capacity of 18: I'm not sure what the cause of this discrepancy is, please explain it to me.

This got me thinking that if I can make my initial call allocate a large enough stack, then my wrenEnsureSlots in the foreign function won't have to reallocate the stack.

Potential workaround

So, if I do wrenEnsureSlots(WREN_NEEDED_SLOTS * 2) (or some other slack factor), I can get the initial stack reallocate prior to wrenCall to create a stack of size 32. Then, my wrenEnsureSlots call within the foreign function implementation does not need to reallocate, and no memory corruption happens.

So it looks like the issue can be worked around like this, but I would like someone to explain to me why the requested stack size differs from calls to ensure the same number of slots. Also, I think it would be beneficial to add some notes about this to the documentation. I can volunteer to do this as well, once I understand the root cause behind it.

Thank you all for your help in troubleshooting this issue.

from wren.