Skip to content

opcache, distinguishing segments on stats tools for Mac. #6628

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

Closed
wants to merge 1 commit into from

Conversation

devnexen
Copy link
Member

No description provided.

@devnexen
Copy link
Member Author

sample output :

image

@@ -87,6 +91,12 @@ static int create_segments(size_t requested_size, zend_shared_segment ***shared_
if (p != MAP_FAILED) {
goto success;
}
#elif defined(VM_MAKE_TAG)
/* allows tracking segments via tools such as vmmap */
p = mmap(NULL, requested_size, flags, MAP_SHARED|MAP_ANONYMOUS, VM_MAKE_TAG(251), 0);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to use something like

int fd = -1;
#if defined(VM_MAKE_TAG)
fd = VM_MAKE_TAG(251)
#endif

and then use fd in the mmap calls. This makes sure this code continues working even if macos supports MAP_32BIT or MAP_HUGETLB in the future.

@devnexen devnexen force-pushed the opcache_segments_id_on_mac branch from 6afbb97 to 86314b8 Compare January 25, 2021 12:06
@php-pulls php-pulls closed this in 4414fd9 Jan 26, 2021
@morrisonlevi
Copy link
Contributor

morrisonlevi commented Jan 30, 2021

FYI, this does not pass a build with UndefinedBehaviorSanitizer:

ext/opcache/shared_alloc_mmap.c:53:7: runtime error: left shift of 251 by 24 places cannot be represented in type 'int'

My guess is that it should be saved as unsigned, then cast/memcpy'd to signed or something?

Edit: it looks like it may be as simple as saying 251u instead of 251.

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.

3 participants