Fix big endian serialization #269
Conversation
Big endian serialization was broken because: - it partially relied on `WORDS_ENDIAN` (unconditionally undef'd in cutils.h) - endianness was not handled at all in the bc reader. Modifications: - remove `WORDS_ENDIAN` - use `bc_put_u32()` / `bc_put_u64()` in `JS_WriteBigInt()` - use `bc_get_u32()` / `bc_get_u64()` in `JS_ReadBigInt()` - handle host endianness in `bc_get_u16()`, `bc_get_u32()`, `bc_get_u64()` and `JS_ReadFunctionBytecode()` - handle optional littleEndian argument as specified in `js_dataview_getValue()` and `js_dataview_setValue()`
Big endian serialization was broken because: - it partially relied on `WORDS_ENDIAN` (unconditionally undef'd in cutils.h) - endianness was not handled at all in the bc reader. - `bc_tag_str` was missing the `"RegExp"` string - `lre_byte_swap()` was broken for `REOP_range` and `REOP_range32` Modifications: - remove `WORDS_ENDIAN` - use `bc_put_u32()` / `bc_put_u64()` in `JS_WriteBigInt()` - use `bc_get_u32()` / `bc_get_u64()` in `JS_ReadBigInt()` - handle host endianness in `bc_get_u16()`, `bc_get_u32()`, `bc_get_u64()` and `JS_ReadFunctionBytecode()` - handle optional littleEndian argument as specified in `js_dataview_getValue()` and `js_dataview_setValue()` - fix `bc_tag_str` and `lre_byte_swap()`
|
Thanks @chqrlie ! Is any test currently testing this behavior? Any chance one could be written? |
There are no big endian architectures in the current CI tests. This actually explains why the partial implementation did not trigger any errors as all the big endian support essentially expands to nothing. Test cases can be added to Maybe we can add |
|
Yep, that would be very useful. Do you want to give it a try? Once you add anything to the yaml file and push the new CI will run. |
|
Do you mind doing this yourself? I am not familiar enough with the yaml stuff yet, I don't want to break anything. |
|
No worries! I'll take a look in the upcoming days, I'm traveling this week. |
There was a problem hiding this comment.
LGTM
@saghul can I go and merge this or should I wait for your CI work? Either is fine by me.
Actually, I would like to refine this patch and clarify the methodology. I shall post another commit later today (after I get some sleep :) |
Big endian serialization was broken because: - it partially relied on `WORDS_ENDIAN` (unconditionally undef'd in cutils.h) - endianness was not handled at all in the bc reader. - `bc_tag_str` was missing the `"RegExp"` string - `lre_byte_swap()` was broken for `REOP_range` and `REOP_range32` Modifications: - remove `WORDS_ENDIAN` - use `bc_put_u32()` / `bc_put_u64()` in `JS_WriteBigInt()` - use `bc_get_u32()` / `bc_get_u64()` in `JS_ReadBigInt()` - handle host endianness in `bc_get_u16()`, `bc_get_u32()`, `bc_get_u64()` and `JS_ReadFunctionBytecode()` - handle optional littleEndian argument as specified in `js_dataview_getValue()` and `js_dataview_setValue()` - fix `bc_tag_str` and `lre_byte_swap()`
| @@ -50512,7 +50513,8 @@ static JSValue js_dataview_setValue(JSContext *ctx, | |||
| { | |||
| JSTypedArray *ta; | |||
| JSArrayBuffer *abuf; | |||
| int is_swap, size; | |||
| BOOL littleEndian, is_swap; | |||
There was a problem hiding this comment.
I know it's nitpicking but for consistency I would suggest calling it little_endian.
There was a problem hiding this comment.
I used littleEndian for consistency with the ECMA document, otherwise I would definitely use little_endian.
Thank you for proposing. I appreciate. I guess I would also get reviewing rights? |
|
Anyone can review, committee or not. In general we open PRs for every change and it should be reviewed by at least 1 person before merging. |
|
Just sent you an invite @chqrlie ! |
I merged the new code, but we still do not have a big-endian CI target... |
|
Life happened 😅 I think I'll have time this week 💪 |
Big endian serialization was broken because:
WORDS_ENDIAN(unconditionally undef'd in cutils.h)bc_tag_strwas missing the"RegExp"stringlre_byte_swap()was broken forREOP_rangeandREOP_range32Modifications:
WORDS_ENDIANbc_put_u32()/bc_put_u64()inJS_WriteBigInt()bc_get_u32()/bc_get_u64()inJS_ReadBigInt()bc_get_u16(),bc_get_u32(),bc_get_u64()andJS_ReadFunctionBytecode()js_dataview_getValue()andjs_dataview_setValue()bc_tag_strandlre_byte_swap()