DelSkayn / rquickjs

High level bindings to the quickjs javascript engine
MIT License
504 stars 63 forks source link

[Question] What's byte_swap param in `Module::write_object`? #139

Open RReverser opened 1 year ago

RReverser commented 1 year ago

Module::write_object accepts a byte_swap param, but the documentation doesn't describe what it does.

I dug into QuickJS itself, which names the param in the same way, but couldn't find any docs either.

I'm guessing it has to do with endianness, but it's unclear exactly what it does - does true imply it will switch to big-endian, to little-endian, or to a platform-dependant opposite of whatever endian is native?

If it's the latter, then, since this is a higher-level API, perhaps it would be better to change the param to big_endian: bool and choose the correct value of byte_swap internally based on #[cfg(little_endian)] so that the API is platform-independant?

DelSkayn commented 1 year ago

byte_swap indeed swaps the bytes of the module bytecode for endianness. If it is set it will always swap the bytes, so it should be used if the bytecode to be loaded is from a platform with a different endianness then the current.

I agree that your proposed method is probably a better way to do it. I will be changing the method.

RReverser commented 1 year ago

Interesting, thanks for explanation / confirmation!

It raises a follow-up question: is endianness encoded in the bytecode? I noticed that the loader doesn't have similar byte_swap param so I wonder how it knows which endianness to choose when parsing bytecode back.

DelSkayn commented 1 year ago

The bytecode doesn't have any direct way to determine the endianness. Loader currently just assumes that the bytecode is of the native endianness.

RReverser commented 1 year ago

Huh, I see. Seems like an oversight in quickjs as it sounds like it's quite easy to make a mistake. Would probably be safer to always assume little-endian and write little-endian, or to store a single byte indicating endianness at the beginning of the bytecode.

RReverser commented 1 year ago

Might be a good candidate for 0.4 to group breaking changes together?

DelSkayn commented 1 year ago

I somewhat reversed my opinion on this issue as QuickJS has no support for loading bytecode with a different endianess then the native one. So I felt that by default the write_object function should return bytecode which is native to the current platform.

The most common use case for bytecode loading should be parsing code in advance and then later loading it on the same platform. You would have the call write_object with big_endian parameter to cfg!(big_endian) for that to work.

I have also introduced write_object_be and write_object_le for if you need to write the bytecode to a specific endianess. Both of which are already included in 0.3.

RReverser commented 1 year ago

So I felt that by default the write_object function should return bytecode which is native to the current platform.

The most common use case for bytecode loading should be parsing code in advance and then later loading it on the same platform.

Hmm if that's the main scenario what is even the use of byte_swap param in the API? Should it be removed then instead?