Skip to content

wasm2c: Mmap+guard on big-endian won't move memory (fix #2599) #2602

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shravanrn
Copy link
Collaborator

No description provided.

@shravanrn shravanrn requested review from keithw, sbc100 and SoniEx2 May 17, 2025 02:06
@shravanrn shravanrn force-pushed the w2c-s390x branch 5 times, most recently from 3ee91d7 to e1c6a86 Compare May 17, 2025 09:22
@shravanrn shravanrn marked this pull request as draft May 17, 2025 09:52
@shravanrn shravanrn marked this pull request as ready for review May 17, 2025 13:19
@shravanrn
Copy link
Collaborator Author

Sorry, it took a few iterations to get all the tests passing. Looks like things are working now.

@shravanrn
Copy link
Collaborator Author

@sbc100 @keithw any chance one of you could review this pr?

const uint64_t mmap_size =
get_alloc_size_for_mmap_default32(memory->max_pages);
void* addr = os_mmap(mmap_size);
unsigned char* addr = os_mmap(mmap_size);
Copy link
Member

Choose a reason for hiding this comment

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

Why not use MEMORY_CELL_TYPE here and avoid the cast below?

Copy link
Member

Choose a reason for hiding this comment

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

Or just uint8_t*?

@@ -492,6 +496,8 @@ typedef struct {
* volatile.
*/
_Atomic volatile uint8_t* data;
/** The location after the the reserved space for the linear memory data. */
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth clarifying if is the address after the last byte of memory, or if it can also include padding/page alignment. From the usage it looks like former?

}
#endif

memory->data = calloc(byte_length, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Why is an explicit cast to MEMORY_CELL_TYPE need above but not here?

@@ -771,7 +771,7 @@ wasm_rt_func_type_t wasm2c_fac_get_func_type(uint32_t param_count, uint32_t resu

if (param_count == 1 && result_count == 1) {
va_start(args, result_count);
if (true && va_arg(args, wasm_rt_type_t) == WASM_RT_I32 && va_arg(args, wasm_rt_type_t) == WASM_RT_I32) {
if (true && va_arg(args, int) == WASM_RT_I32 && va_arg(args, int) == WASM_RT_I32) {
Copy link
Member

Choose a reason for hiding this comment

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

Are these change from a different PR? If so, can they be landed separately to avoid confusion.

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.

2 participants