ruby-changes:70230
From: Koichi <ko1@a...>
Date: Wed, 15 Dec 2021 15:04:58 +0900 (JST)
Subject: [ruby-changes:70230] 0eafba3610 (master): use `RB_VM_LOCK_ENTER()`
https://git.ruby-lang.org/ruby.git/commit/?id=0eafba3610 From 0eafba36103f5526c489fc5dd3d958d97e11a2c2 Mon Sep 17 00:00:00 2001 From: Koichi Sasada <ko1@a...> Date: Tue, 14 Dec 2021 17:28:25 +0900 Subject: use `RB_VM_LOCK_ENTER()` We found that we need to make Ruby objects while locking the environ to ENV operation atomically, so we decided to use `RB_VM_LOCK_ENTER()` instead of `env_lock`. --- common.mk | 2 + hash.c | 712 +++++++++++++++++++++++++++++++++----------------------------- 2 files changed, 377 insertions(+), 337 deletions(-) diff --git a/common.mk b/common.mk index e2b513f0f31..a2fbac18aa2 100644 --- a/common.mk +++ b/common.mk @@ -7180,6 +7180,8 @@ hash.$(OBJEXT): {$(VPATH)}symbol.h https://github.com/ruby/ruby/blob/trunk/common.mk#L7180 hash.$(OBJEXT): {$(VPATH)}thread_native.h hash.$(OBJEXT): {$(VPATH)}transient_heap.h hash.$(OBJEXT): {$(VPATH)}util.h +hash.$(OBJEXT): {$(VPATH)}vm_debug.h +hash.$(OBJEXT): {$(VPATH)}vm_sync.h inits.$(OBJEXT): $(hdrdir)/ruby.h inits.$(OBJEXT): $(hdrdir)/ruby/ruby.h inits.$(OBJEXT): $(top_srcdir)/internal/compilers.h diff --git a/hash.c b/hash.c index 9b21f746548..8127751a49b 100644 --- a/hash.c +++ b/hash.c @@ -45,6 +45,7 @@ https://github.com/ruby/ruby/blob/trunk/hash.c#L45 #include "transient_heap.h" #include "ruby/thread_native.h" #include "ruby/ractor.h" +#include "vm_sync.h" #ifndef HASH_DEBUG #define HASH_DEBUG 0 @@ -4810,16 +4811,8 @@ extern char **environ; https://github.com/ruby/ruby/blob/trunk/hash.c#L4811 #define ENVNMATCH(s1, s2, n) (memcmp((s1), (s2), (n)) == 0) #endif -static rb_nativethread_lock_t env_lock; - -static const char* -getenv_with_lock(const char *name) -{ - rb_native_mutex_lock(&env_lock); - char *result = getenv(name); - rb_native_mutex_unlock(&env_lock); - return result; -} +#define ENV_LOCK() RB_VM_LOCK_ENTER() +#define ENV_UNLOCK() RB_VM_LOCK_LEAVE() static inline rb_encoding * env_encoding(void) @@ -4840,12 +4833,6 @@ env_enc_str_new(const char *ptr, long len, rb_encoding *enc) https://github.com/ruby/ruby/blob/trunk/hash.c#L4833 return str; } -static VALUE -env_enc_str_new_cstr(const char *ptr, rb_encoding *enc) -{ - return env_enc_str_new(ptr, strlen(ptr), enc); -} - static VALUE env_str_new(const char *ptr, long len) { @@ -4859,14 +4846,35 @@ env_str_new2(const char *ptr) https://github.com/ruby/ruby/blob/trunk/hash.c#L4846 return env_str_new(ptr, strlen(ptr)); } -static const char TZ_ENV[] = "TZ"; - static VALUE -env_name_new(const char *name, const char *ptr) +getenv_with_lock(const char *name) { - return env_enc_str_new_cstr(ptr, env_encoding()); + VALUE ret; + ENV_LOCK(); + { + const char *val = getenv(name); + ret = env_str_new2(val); + } + ENV_UNLOCK(); + return ret; +} + +static bool +has_env_with_lock(const char *name) +{ + const char *val; + + ENV_LOCK(); + { + val = getenv(name); + } + ENV_UNLOCK(); + + return val ? true : false; } +static const char TZ_ENV[] = "TZ"; + static void * get_env_cstr( VALUE str, @@ -4919,17 +4927,13 @@ static VALUE https://github.com/ruby/ruby/blob/trunk/hash.c#L4927 env_delete(VALUE name) { const char *nam = env_name(name); - const char *val = getenv_with_lock(nam); - reset_by_modified_env(nam); + VALUE val = getenv_with_lock(nam); - if (val) { - VALUE value = env_str_new2(val); - - ruby_setenv(nam, 0); - return value; + if (!NIL_P(val)) { + ruby_setenv(nam, 0); } - return Qnil; + return val; } /* @@ -4982,14 +4986,9 @@ env_delete_m(VALUE obj, VALUE name) https://github.com/ruby/ruby/blob/trunk/hash.c#L4986 static VALUE rb_f_getenv(VALUE obj, VALUE name) { - const char *nam, *env; - - nam = env_name(name); - env = getenv_with_lock(nam); - if (env) { - return env_name_new(nam, env); - } - return Qnil; + const char *nam = env_name(name); + VALUE env = getenv_with_lock(nam); + return env; } /* @@ -5022,7 +5021,8 @@ env_fetch(int argc, VALUE *argv, VALUE _) https://github.com/ruby/ruby/blob/trunk/hash.c#L5021 { VALUE key; long block_given; - const char *nam, *env; + const char *nam; + VALUE env; rb_check_arity(argc, 1, 2); key = argv[0]; @@ -5032,14 +5032,15 @@ env_fetch(int argc, VALUE *argv, VALUE _) https://github.com/ruby/ruby/blob/trunk/hash.c#L5032 } nam = env_name(key); env = getenv_with_lock(nam); - if (!env) { + + if (NIL_P(env)) { if (block_given) return rb_yield(key); if (argc == 1) { rb_key_err_raise(rb_sprintf("key not found: \"%"PRIsVALUE"\"", key), envtbl, key); } return argv[1]; } - return env_name_new(nam, env); + return env; } int @@ -5064,17 +5065,17 @@ in_origenv(const char *str) https://github.com/ruby/ruby/blob/trunk/hash.c#L5065 static int envix(const char *nam) { + // should be locked + register int i, len = strlen(nam); char **env; - rb_native_mutex_lock(&env_lock); env = GET_ENVIRON(environ); for (i = 0; env[i]; i++) { if (ENVNMATCH(env[i],nam,len) && env[i][len] == '=') break; /* memcmp must come first to avoid */ } /* potential SEGV's */ FREE_ENVIRON(environ); - rb_native_mutex_unlock(&env_lock); return i; } #endif @@ -5175,13 +5176,17 @@ ruby_setenv(const char *name, const char *value) https://github.com/ruby/ruby/blob/trunk/hash.c#L5176 wname[len-1] = L'='; #endif } - rb_native_mutex_lock(&env_lock); + + ENV_LOCK(); + { #ifndef HAVE__WPUTENV_S - failed = _wputenv(wname); + failed = _wputenv(wname); #else - failed = _wputenv_s(wname, wvalue); + failed = _wputenv_s(wname, wvalue); #endif - rb_native_mutex_unlock(&env_lock); + } + ENV_UNLOCK(); + ALLOCV_END(buf); /* even if putenv() failed, clean up and try to delete the * variable from the system area. */ @@ -5196,21 +5201,31 @@ ruby_setenv(const char *name, const char *value) https://github.com/ruby/ruby/blob/trunk/hash.c#L5201 } #elif defined(HAVE_SETENV) && defined(HAVE_UNSETENV) if (value) { - rb_native_mutex_lock(&env_lock); - bool setenv_fail = setenv(name, value, 1); - rb_native_mutex_unlock(&env_lock); - if (setenv_fail) - rb_sys_fail_str(rb_sprintf("setenv(%s)", name)); + int ret; + ENV_LOCK(); + { + ret = setenv(name, value, 1); + } + ENV_UNLOCK(); + + if (ret) rb_sys_fail_str(rb_sprintf("setenv(%s)", name)); } else { #ifdef VOID_UNSETENV - unsetenv(name); + ENV_LOCK(); + { + unsetenv(name); + } + ENV_UNLOCK(); #else - rb_native_mutex_lock(&env_lock); - bool unsetenv_fail = unsetenv(name); - rb_native_mutex_unlock(&env_lock); - if (unsetenv_fail) - rb_sys_fail_str(rb_sprintf("unsetenv(%s)", name)); + int ret; + ENV_LOCK(); + { + ret = unsetenv(name); + } + ENV_UNLOCK(); + + if (ret) rb_sys_fail_str(rb_sprintf("unsetenv(%s)", name)); #endif } #elif defined __sun @@ -5218,76 +5233,91 @@ ruby_setenv(const char *name, const char *value) https://github.com/ruby/ruby/blob/trunk/hash.c#L5233 /* The below code was tested on Solaris 10 by: % ./configure ac_cv_func_setenv=no ac_cv_func_unsetenv=no */ - rb_native_mutex_lock(&env_lock); size_t len, mem_size; char **env_ptr, *str, *mem_ptr; check_envname(name); len = strlen(name); if (value) { - mem_size = len + strlen(value) + 2; - mem_ptr = malloc(mem_size); - if (mem_ptr == NULL) - rb_sys_fail_str(rb_sprintf("malloc("PRIuSIZE")", mem_size)); - snprintf(mem_ptr, mem_size, "%s=%s", name, value); - } - for (env_ptr = GET_ENVIRON(environ); (str = *env_ptr) != 0; ++env_ptr) { - if (!strncmp(str, name, len) && str[len] == '=') { - if (!in_origenv(str)) free(str); - while ((env_ptr[0] = env_ptr[1]) != 0) env_ptr++; - break; - } + mem_size = len + strlen(value) + 2; + mem_ptr = malloc(mem_size); + if (mem_ptr == NULL) + rb_sys_fail_str(rb_sprintf("malloc("PRIuSIZE")", mem_size)); + snprintf(mem_ptr, mem_size, "%s=%s", name, value); + } + + ENV_LOCK(); + { + for (env_ptr = GET_ENVIRON(environ); (str = *env_ptr) != 0; ++env_ptr) { + if (!strncmp(str, name, len) && str[len] == '=') { + if (!in_origenv(str)) free(str); + while ((env_ptr[0] = env_ptr[1]) != 0) env_ptr++; + break; + } + } } + ENV_UNLOCK(); + if (value) { - bool putenv_fail = putenv(mem_ptr); - rb_native_mutex_unlock(&env_lock); - if (putenv_fail) { + int ret; + ENV_LOCK(); + { + ret = putenv(mem_ptr); + } + ENV_UNLOCK(); + + if (ret) { free(mem_ptr); - rb_sys_fail_str(rb_sprintf("putenv(%s)", name)); - } - } - else { - rb_native_mutex_unlock(&env_lock); + rb_sys_fail_str(rb_sprintf("putenv(%s)", name)); + } } #else /* WIN32 */ size_t len; int i; - i=envix(name); /* where does it go? */ - - if (environ == origenviron) { /* need we copy environment? */ - int j; - int max; - char **tmpenv; - - for (max = i; environ[max]; max++) ; - tmpenv = ALLOC_N(char*, max+2); - for (j=0; j<max; j++) /* copy environment */ - tmpenv[j] = ruby_strdup(environ[j]); - tmpenv[max] = 0; - environ = tmpenv; /* tell exec where it is now */ - } - if (environ[i]) { - char **envp = origenviron; - while (*envp && *envp != environ[i]) envp++; - if (!*envp) - xfree(environ[i]); - if (!value) { - while (environ[i]) { - environ[i] = environ[i+1]; - i++; - } - return; - } - } - else { /* does not exist yet */ - if (!value) return; - REALLOC_N(environ, char*, i+2); /* just expand it a bit */ - environ[i+1] = 0; /* make sure it's null terminated */ + ENV_LOCK(); + { + i = envix(name); /* where does it go? (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/