8000 Fix unavailable MAP_ANONYMOUS by Morriar · Pull Request #2470 · ruby/rbs · GitHub
[go: up one dir, main page]

Skip to content

Fix unavailable MAP_ANONYMOUS #2470

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

Merged
merged 4 commits into from
Jun 2, 2025
Merged

Fix unavailable MAP_ANONYMOUS #2470

merged 4 commits into from
Jun 2, 2025

Conversation

Morriar
Copy link
Contributor
@Morriar Morriar commented May 9, 2025

Trying fix for ruby/ruby#13237 (comment)

@amomchilov
Copy link
Contributor

cc @tekknolagi

@tekknolagi
Copy link
Contributor

Nice catch. Some duplicated code - I'll check on Monday

@tekknolagi
Copy link
Contributor

Might be able to define _GNU_SOURCE to fix instead

@soutaro
Copy link
Member
soutaro commented May 12, 2025

Thanks. I'm waiting for the fix by @tekknolagi, and will release a new .dev version to test it in Ruby CI tomorrow.

@tekknolagi
Copy link
Contributor

@Morriar I recommend looking into a fix with _GNU_SOURCE--it's likely the platform has MAP_ANONYMOUS but is pretending not to

If not, then we should at least drop the _WIN32 branch of the ifdef (it's otherwise handled by map_memory) and inline this whole thing into map_memory

Comment on lines 33 to 35
#ifdef _WIN32
ptr = VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE);
if (ptr == NULL) return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#ifdef _WIN32
ptr = VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE);
if (ptr == NULL) return NULL;

ptr = VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE);
if (ptr == NULL) return NULL;
#elif defined(MAP_ANONYMOUS)
ptr = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ptr = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
return mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);

#elif defined(MAP_ANONYMOUS)
ptr = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
#elif defined(MAP_ANON)
ptr = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ptr = mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
return mmap(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);

#else
/* Fallback to /dev/zero for systems without anonymous mapping */
int fd = open("/dev/zero", O_RDWR);
if (fd == -1) return MAP_FAILED;
Copy link
Contributor

Choose a reason for hiding this comment

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

Masking as MAP_FAILED is a little gross imo; have it have its own assert

@st0012 st0012 force-pushed the at-fix-mmap branch 4 times, most recently from a23c761 to aa34584 Compare May 20, 2025 18:17
@tekknolagi
Copy link
Contributor

Nice!

@st0012
Copy link
Member
10000 st0012 commented May 20, 2025

@soutaro This PR should pass all compilation builds now 😄

@soutaro soutaro added this to the RBS 4.0 milestone May 27, 2025
Copy link
Member
@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Thanks!

@soutaro soutaro added this pull request to the merge queue May 27, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 27, 2025
Morriar and others added 4 commits May 28, 2025 09:41
@st0012
Copy link
Member
st0012 commented May 28, 2025

@soutaro I've rebased this branch. It should be good to go now

@soutaro soutaro added this pull request to the merge queue Jun 2, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 2, 2025
@soutaro soutaro added this pull request to the merge queue Jun 2, 2025
Merged via the queue into ruby:master with 792F commit d1c7f67 Jun 2, 2025
22 checks passed
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.

5 participants
0