Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fast-interp "pre-compiled label offset out of range" issue #2659

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

wenyongh
Copy link
Contributor

When labels-as-values is enabled in a target which doesn't support
unaligned address access, 16-bit offset is used to store the relative
offset between two opcode labels. But it is a little small and the loader
may report "pre-compiled label offset out of range" error.

Emitting 32-bit data instead to resolve the issue: emit label address in
32-bit target and emit 32-bit relative offset in 64-bit target.

See also:
#2635

#define FETCH_OPCODE_AND_DISPATCH() \
do { \
const void *p_label_addr; \
bh_assert(((uintptr_t)frame_ip & 1) == 0); \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should & 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the frame_ip must be 2-byte aligned, but not necessarily 4-byte aligned. If we force the frame_ip to be 4-byte aligned in wasm_loader, we need to add padding byte if needed, and here, we need to add code like frame_ip = align_uint(frame_ip, 4) or if (frame_ip & 2) frame_ip += 2, I had a try, it impacts performance a lot.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, Thanks!

I originally thought the CPU requires 4-bytes align, if 2-bytes align is OK, then no problem here.

#else
#define emit_label(opcode) \
do { \
uint32 offset = (uint32)(uintptr_t)handle_table[opcode]; \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had better rename offset to position or label_addr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks.

Copy link
Collaborator

@xujuntwt95329 xujuntwt95329 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wenyongh wenyongh merged commit d6bba13 into bytecodealliance:main Oct 24, 2023
374 checks passed
@wenyongh wenyongh deleted the dev/fix_labels_as_values branch October 24, 2023 02:58
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
…odealliance#2659)

When labels-as-values is enabled in a target which doesn't support
unaligned address access, 16-bit offset is used to store the relative
offset between two opcode labels. But it is a little small and the loader
may report "pre-compiled label offset out of range" error.

Emitting 32-bit data instead to resolve the issue: emit label address in
32-bit target and emit 32-bit relative offset in 64-bit target.

See also:
bytecodealliance#2635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants