Skip to content

Use ZendMM in ext-gmp #16609

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.

Sign up for GitHub

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 2 commits into from
Closed

Use ZendMM in ext-gmp #16609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c32624
Use ZendMM in ext-gmp
nielsdos Oct 26, 2024
d816fc3
no return
nielsdos Oct 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 additions & 2 deletions ext/gmp/gmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@ zend_module_entry gmp_module_entry = {
ext_functions,
ZEND_MODULE_STARTUP_N(gmp),
NULL,
NULL,
ZEND_MODULE_ACTIVATE_N(gmp),
ZEND_MODULE_DEACTIVATE_N(gmp),
ZEND_MODULE_INFO_N(gmp),
PHP_GMP_VERSION,
ZEND_MODULE_GLOBALS(gmp),
ZEND_GINIT(gmp),
NULL,
NULL,
ZEND_MODULE_POST_ZEND_DEACTIVATE_N(gmp),
STANDARD_MODULE_PROPERTIES_EX
};
/* }}} */
Expand All @@ -81,6 +81,16 @@ ZEND_GET_MODULE(gmp)
static zend_class_entry *gmp_ce;
static zend_object_handlers gmp_object_handlers;

/* Allocator state: `gmp_request_allocator` is a true TLS global
* to indicate whether this thread is currently working on a PHP request.
* Just checking EG() is not enough because other modules may be loaded in the webserver. */
static TSRM_TLS bool gmp_request_allocator = false;
Comment on lines +84 to +87
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this be a gmp module global?

Copy link
Member Author

Choose a reason for hiding this comment

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

No.

Simplest counter-example:

Imagine you're in an embedded SAPI with multiple threads. If you only run a single PHP interpreter at a time you can safely use NTS in this scenario. When NTS is used, the variable will be a true global, which would break other threads trying to use libgmp.

That aside, AFAIK, a true TLS variable will be faster than having to go through the TSRM too. As other DSO's don't need access to this variable directly there's no reason to put it in the module globals.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thank you!

/* True globals for the old allocator, as these are global state in gmplib,
* this doesn't need to be TLS. */
static void *(*gmp_old_alloc)(size_t);
static void *(*gmp_old_realloc)(void *, size_t, size_t);
static void (*gmp_old_free)(void *, size_t);

PHP_GMP_API zend_class_entry *php_gmp_class_entry(void) {
return gmp_ce;
}
Expand Down Expand Up @@ -521,6 +531,33 @@ static int gmp_unserialize(zval *object, zend_class_entry *ce, const unsigned ch
}
/* }}} */

static void *gmp_alloc(size_t size)
{
if (gmp_request_allocator) {
return emalloc(size);
} else {
return gmp_old_alloc(size);
}
}

static void *gmp_realloc(void *ptr, size_t old_size, size_t new_size)
{
if (gmp_request_allocator) {
return erealloc(ptr, new_size);
} else {
return gmp_old_realloc(ptr, old_size, new_size);
}
}

static void gmp_free(void *ptr, size_t size)
{
if (gmp_request_allocator) {
efree_size(ptr, size);
} else {
gmp_old_free(ptr, size);
}
}

/* {{{ ZEND_GINIT_FUNCTION */
static ZEND_GINIT_FUNCTION(gmp)
{
Expand Down Expand Up @@ -551,10 +588,19 @@ ZEND_MINIT_FUNCTION(gmp)

register_gmp_symbols(module_number);

mp_get_memory_functions(&gmp_old_alloc, &gmp_old_realloc, &gmp_old_free);
mp_set_memory_functions(gmp_alloc, gmp_realloc, gmp_free);
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should be nice and restore the old functions on MSHUTDOWN?

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 that would be better yes.


return SUCCESS;
}
/* }}} */

ZEND_MODULE_ACTIVATE_D(gmp)
{
gmp_request_allocator = true;
return SUCCESS;
}

/* {{{ ZEND_RSHUTDOWN_FUNCTION */
ZEND_MODULE_DEACTIVATE_D(gmp)
{
Expand All @@ -567,6 +613,13 @@ ZEND_MODULE_DEACTIVATE_D(gmp)
}
/* }}} */

ZEND_MODULE_POST_ZEND_DEACTIVATE_D(gmp)
{
/* We have to deactivate it after all request state has been freed. */
gmp_request_allocator = false;
return SUCCESS;
}

/* {{{ ZEND_MINFO_FUNCTION */
ZEND_MODULE_INFO_D(gmp)
{
Expand Down
2 changes: 2 additions & 0 deletions ext/gmp/php_gmp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ extern zend_module_entry gmp_module_entry;
#define PHP_GMP_VERSION PHP_VERSION

ZEND_MODULE_STARTUP_D(gmp);
ZEND_MODULE_ACTIVATE_D(gmp);
ZEND_MODULE_DEACTIVATE_D(gmp);
ZEND_MODULE_POST_ZEND_DEACTIVATE_D(gmp);
ZEND_MODULE_INFO_D(gmp);

ZEND_BEGIN_MODULE_GLOBALS(gmp)
Expand Down