Pointer and length loads and stores in generated JS use signed methods

[bennetthardwick]

I've noticed that the JS code generated by JCO uses getInt32 instead of getUint32. I believe it stems from here:

jco/crates/js-component-bindgen/src/function_bindgen.rs

Lines 1238 to 1243 in 146986d

	Instruction::LengthStore { offset } => self.store("setInt32", *offset, operands),
	Instruction::LengthLoad { offset } => self.load("getInt32", *offset, operands, results),
	Instruction::PointerStore { offset } => self.store("setInt32", *offset, operands),
	Instruction::PointerLoad { offset } => {
	self.load("getInt32", *offset, operands, results)
	}

Is this on purpose or should it be using the unsigned methods instead?

[bennetthardwick]

Here's the code that gets generated when returning a string:

  var ptr2 = dataView(memory0).getInt32(ret + 0, true);
  var len2 = dataView(memory0).getInt32(ret + 4, true);
  var result2 = utf8Decoder.decode(new Uint8Array(memory0.buffer, ptr2, len2));

[vados-cosmonic]

Hey @bennetthardwick that's a good question!

If you take a look at the canonical ABI documentation, strings (see load_string) generally are represented as two i32 pointers -- a pointer and a number of code units.

I'm inclined to believe here that the code is doing the right thing -- did you run into a specific problem with the code as present?

[bennetthardwick]

Thanks for that reference @vados-cosmonic - definitely not a bug then based on the component model spec.

I guess I'm just wondering why an i32 would be chosen for a pointer since it effectively halves the usable WASM memory? If I allocate memory larger than about 2GB I get an error. I can't seem to find anywhere that justifies it in that document.

[bennetthardwick]

@vados-cosmonic I've just had another read of the doc and I now think this is a bug. It seems that while WASM only has an i32 type, loading an integer should interpret the first bit depending on whether it is considered signed or not (by the application).

Integers are loaded directly from memory, with their high-order bit interpreted according to the signedness of the type.

def load_int(cx, ptr, nbytes, signed = False):
  return int.from_bytes(cx.opts.memory[ptr : ptr+nbytes], 'little', signed = signed)

Since it is signed = False by default, the following call for load_string should be loading the ptr and length as unsigned ints / u32:

def load_string(cx, ptr) -> String:
  begin = load_int(cx, ptr, 4)
  tagged_code_units = load_int(cx, ptr + 4, 4)
  return load_string_from_range(cx, begin, tagged_code_units)

For example here are how the types are canonically loaded using the load_int method from the document:

   case U8Type()           : return load_int(cx, ptr, 1)
   case U16Type()          : return load_int(cx, ptr, 2)
   case U32Type()          : return load_int(cx, ptr, 4)
   case U64Type()          : return load_int(cx, ptr, 8)
   case S8Type()           : return load_int(cx, ptr, 1, signed = True)
   case S16Type()          : return load_int(cx, ptr, 2, signed = True)
   case S32Type()          : return load_int(cx, ptr, 4, signed = True)
   case S64Type()          : return load_int(cx, ptr, 8, signed = True)

Edit: it seems like the first i32 in load string should be an unsigned int. But the second isn't because the high bit is apparently used to represent the encoding. It is still first interpreted as an unsigned int though.

[vados-cosmonic]

Hey @bennetthardwick good point -- I think I agree with you now, this seems like a bug, specifically that first unsigned int.

Another issue here was the support for this at the lower level, but V8 has had support for using the full 4GB space for a while now, so there is no reason to limit the pointer to be an i32 at this point.

If you'd like to submit a PR I'd be happy to review it!

[bennetthardwick]

Sweet, I'll look at preparing a PR! Thanks 👍🏻