Skip to content

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

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
@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 commit d1c7f67 Jun 2, 2025
22 checks passed
soutaro added a commit that referenced this pull request Oct 6, 2025
Fix unavailable MAP_ANONYMOUS
@soutaro soutaro mentioned this pull request Oct 6, 2025
soutaro added a commit that referenced this pull request Oct 6, 2025
Fix unavailable MAP_ANONYMOUS
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