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

NativeAOT-LLVM: Do not use mmap/munmap #1510

Merged
merged 2 commits into from
Sep 6, 2021

Conversation

yowl
Copy link
Contributor

@yowl yowl commented Sep 5, 2021

This PR adds an emscripten friendly VirtualReserve/Release implementation. Emscripten does not fully implement mmap/munmap in particular it does not zero the memory in mmap for the MAP_ANONYMOUS flag. This continues from dotnet/corert#8330.

When trying to merge the latest into NativeAOT-LLVM random memory crashes, usually inside the GC, have started to appear, so now seems a good time to ask for feedback on this change. This change does stop those crashes appearing so far at least.

Emscripten reports the page size as 0x4000, whereas CommonMacros.h has

#elif defined(HOST_WASM)

#define DATA_ALIGNMENT  4
#ifndef OS_PAGE_SIZE
#define OS_PAGE_SIZE    0x4
#endif

I don't know what the implications are there, if anything maybe just performance as posix_memalign does not refer to the page size.

typedef struct _reserved reserved;

static int nextPos = 0;
static reserved blocks[1000]; // better to allocate and grow this as needed (until malloc fails)?
Copy link
Member

Choose a reason for hiding this comment

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

This is really only used for argument validation. The GC should not be ever calling these APIs with invalid arguments.

I think you can omit this structure, and instead directly call posix_memalign / free / memset to implement reserve / release / decommit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, works with without that.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@jkotas jkotas merged commit dcba38b into dotnet:feature/NativeAOT-LLVM Sep 6, 2021
@yowl yowl deleted the wasm-alloc branch September 6, 2021 15:17
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