From patchwork Thu Apr 13 04:04:55 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sergio Durigan Junior X-Patchwork-Id: 20014 Received: (qmail 29484 invoked by alias); 13 Apr 2017 04:05:16 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 15279 invoked by uid 89); 13 Apr 2017 04:05:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 13 Apr 2017 04:04:59 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 68276EEF0A for ; Thu, 13 Apr 2017 04:04:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 68276EEF0A Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=sergiodj@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 68276EEF0A Received: from psique.yyz.redhat.com (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4D68260F86; Thu, 13 Apr 2017 04:04:57 +0000 (UTC) From: Sergio Durigan Junior To: GDB Patches Cc: Sergio Durigan Junior Subject: [PATCH] C++ify gdb/common/environ.c Date: Thu, 13 Apr 2017 00:04:55 -0400 Message-Id: <20170413040455.23996-1-sergiodj@redhat.com> X-IsSubscribed: yes Disclaimer: this patch depends on to be fully testable. As part of the preparation necessary for my upcoming task, I'd like to propose that we turn gdb_environ into a class. The approach taken here is simple: the class gdb_environ contains everything that is needed to manipulate the environment variables. These variables are stored in two data structures: an unordered_set, good because lookups are O(n), and an std::vector, which can be converted to a 'char **' and passed as argument to functions that need it. The usage has not changed much. One has to initialize the class and call its member functions if any operation needs to be done with the environment variables. As mentioned before, this is a preparation for an upcoming work that I will be posting in the next few weeks or so. For that work, I'll probably create another data structure that will contain all the environment variables that were set by the user using the 'set environment' command, because I'll need access to them. This will be much easier with the class-ification of gdb_environ. As noted, this has been regression-tested with the new version of environ.exp and no regressions were found. gdb/ChangeLog: yyyy-mm-dd Sergio Durigan Junior * charset.c (find_charset_names): Use a std::unique_ptr. Update code to use 'iconv_env' object. Remove call to 'free_environ'. * common/environ.c: Include . (make_environ): Delete function. (gdb_environ::init_environ): New function. (free_environ): Delete function. (gdb_environ::clear_environ): New function. (gdb_environ::gdb_environ): New function. (init_environ): Delete function. (gdb_environ::~gdb_environ): New function. (gdb_environ::reinit_environ): Likewise. (gdb_environ::get_in_environ): Likewise. (environ_vector): Delete function. (unset_in_environ_vector): New function. (set_in_environ): Delete function. (gdb_environ::set_in_environ): New function. (unset_in_environ): Delete function. (gdb_environ::unset_in_environ): New function. (gdb_environ::get_environ_char_vector): Likewise. (gdb_environ::get_environ_vector): Likewise. * common/environ.h: Include and . (struct gdb_environ): Delete; transform into... (class gdb_environ): ... this class. (free_environ): Delete prototype. (init_environ, get_in_environ, set_in_environ, unset_in_environ, environ_vector): Likewise. * infcmd.c (run_command_1): Update code to call 'get_environ_char_vector' from 'gdb_environ' class. (environment_info): Update code to call methods from 'gdb_environ' class. (unset_environment_command): Likewise. (path_info): Likewise. (path_command): Likewise. * inferior.c (free_inferior): Delete call to 'free_environ'. (add_inferior_silent): Create 'inf->environment' from std::unique_ptr. * inferior.h: Include "environ.h". (struct inferior) : Change type from 'struct gdb_environ' to 'std::unique_ptr'. * mi/mi-cmd-env.c (mi_cmd_env_path): Update code to call methods from 'gdb_environ' class. (mi_cmd_inferior_tty_show): Use 'std::unique_ptr'. (_initialize_mi_cmd_env): Update code to call methods from 'gdb_environ' class. * solib.c (solib_find_1): Likewise --- gdb/charset.c | 11 +-- gdb/common/environ.c | 228 +++++++++++++++++++++++++-------------------------- gdb/common/environ.h | 62 +++++++++----- gdb/infcmd.c | 31 ++++--- gdb/inferior.c | 4 +- gdb/inferior.h | 5 +- gdb/mi/mi-cmd-env.c | 13 ++- gdb/solib.c | 8 +- 8 files changed, 187 insertions(+), 175 deletions(-) diff --git a/gdb/charset.c b/gdb/charset.c index f55e482..a5ab383 100644 --- a/gdb/charset.c +++ b/gdb/charset.c @@ -794,16 +794,14 @@ find_charset_names (void) int err, status; int fail = 1; int flags; - struct gdb_environ *iconv_env; + std::unique_ptr iconv_env (new gdb_environ); char *iconv_program; /* Older iconvs, e.g. 2.2.2, don't omit the intro text if stdout is not a tty. We need to recognize it and ignore it. This text is subject to translation, so force LANGUAGE=C. */ - iconv_env = make_environ (); - init_environ (iconv_env); - set_in_environ (iconv_env, "LANGUAGE", "C"); - set_in_environ (iconv_env, "LC_ALL", "C"); + iconv_env->set_in_environ ("LANGUAGE", "C"); + iconv_env->set_in_environ ("LC_ALL", "C"); child = pex_init (PEX_USE_PIPES, "iconv", NULL); @@ -827,7 +825,7 @@ find_charset_names (void) /* Note that we simply ignore errors here. */ if (!pex_run_in_environment (child, flags, args[0], const_cast (args), - environ_vector (iconv_env), + iconv_env->get_environ_char_vector (), NULL, NULL, &err)) { FILE *in = pex_read_output (child, 0); @@ -901,7 +899,6 @@ find_charset_names (void) xfree (iconv_program); pex_free (child); - free_environ (iconv_env); if (fail) { diff --git a/gdb/common/environ.c b/gdb/common/environ.c index 3145d01..bd3c964 100644 --- a/gdb/common/environ.c +++ b/gdb/common/environ.c @@ -18,165 +18,161 @@ #include "common-defs.h" #include "environ.h" #include - +#include -/* Return a new environment object. */ +/* See common/environ.h. */ -struct gdb_environ * -make_environ (void) +void +gdb_environ::init_environ (void) { - struct gdb_environ *e; + extern char **environ; + + if (environ == NULL) + return; + + for (int i = 0; environ[i] != NULL; ++i) + { + std::string v = std::string (environ[i]); + std::size_t pos = v.find ('='); + std::string var = v.substr (0, pos); + std::string value = v.substr (pos + 1); - e = XNEW (struct gdb_environ); + this->env.insert (std::make_pair (var, value)); + this->environ_vector.push_back (xstrdup (environ[i])); + } - e->allocated = 10; - e->vector = (char **) xmalloc ((e->allocated + 1) * sizeof (char *)); - e->vector[0] = 0; - return e; + /* The last element of the vector is always going to be NULL. */ + this->environ_vector.push_back (NULL); } -/* Free an environment and all the strings in it. */ +/* See common/environ.h. */ void -free_environ (struct gdb_environ *e) +gdb_environ::clear_environ (void) { - char **vector = e->vector; + this->env.clear (); + for (char *v : this->environ_vector) + xfree (v); + this->environ_vector.clear (); +} - while (*vector) - xfree (*vector++); +/* See common/environ.h. */ - xfree (e->vector); - xfree (e); +gdb_environ::gdb_environ (void) +{ + this->init_environ (); } -/* Copy the environment given to this process into E. - Also copies all the strings in it, so we can be sure - that all strings in these environments are safe to free. */ +/* See common/environ.h. */ -void -init_environ (struct gdb_environ *e) +gdb_environ::~gdb_environ (void) { - extern char **environ; - int i; + this->clear_environ (); +} - if (environ == NULL) - return; +/* See common/environ.h. */ - for (i = 0; environ[i]; i++) /*EMPTY */ ; +void +gdb_environ::reinit_environ (void) +{ + this->clear_environ (); + this->init_environ (); +} - if (e->allocated < i) +/* See common/environ.h. */ + +char * +gdb_environ::get_in_environ (const char *var) const +{ + try { - e->allocated = std::max (i, e->allocated + 10); - e->vector = (char **) xrealloc ((char *) e->vector, - (e->allocated + 1) * sizeof (char *)); + return (char *) this->env.at (std::string (var)).c_str (); } - - memcpy (e->vector, environ, (i + 1) * sizeof (char *)); - - while (--i >= 0) + catch (const std::out_of_range &ex) { - int len = strlen (e->vector[i]); - char *newobj = (char *) xmalloc (len + 1); - - memcpy (newobj, e->vector[i], len + 1); - e->vector[i] = newobj; + return NULL; } } -/* Return the vector of environment E. - This is used to get something to pass to execve. */ - -char ** -environ_vector (struct gdb_environ *e) -{ - return e->vector; -} - -/* Return the value in environment E of variable VAR. */ +/* Unset (delete) the environment variable VAR on the environment + vector V. */ -char * -get_in_environ (const struct gdb_environ *e, const char *var) +static void +unset_in_environ_vector (std::vector &v, const char *var) { - int len = strlen (var); - char **vector = e->vector; - char *s; - - for (; (s = *vector) != NULL; vector++) - if (strncmp (s, var, len) == 0 && s[len] == '=') - return &s[len + 1]; - - return 0; + std::string match = std::string (var + std::string ("=")); + const char *match_str = match.c_str (); + + for (std::vector::const_iterator el = v.cbegin (); + el != v.cend (); + ++el) + if (startswith (*el, match_str)) + { + xfree (*el); + v.erase (el); + break; + } } -/* Store the value in E of VAR as VALUE. */ +/* See common/environ.h. */ void -set_in_environ (struct gdb_environ *e, const char *var, const char *value) +gdb_environ::set_in_environ (const char *var, const char *value) { - int i; - int len = strlen (var); - char **vector = e->vector; - char *s; + std::unordered_map::iterator el; + bool needs_update = true; - for (i = 0; (s = vector[i]) != NULL; i++) - if (strncmp (s, var, len) == 0 && s[len] == '=') - break; + el = this->env.find (var); - if (s == 0) + if (el != this->env.end ()) { - if (i == e->allocated) + if (el->second.compare (value) == 0) + needs_update = false; + else { - e->allocated += 10; - vector = (char **) xrealloc ((char *) vector, - (e->allocated + 1) * sizeof (char *)); - e->vector = vector; + /* If we found this item, it means that we have to update + its value on the map. */ + el->second = std::string (value); + /* And we also have to update its value on the + environ_vector. For that, we just delete the item here + and recreate it (with the new value) later. */ + unset_in_environ_vector (this->environ_vector, var); } - vector[i + 1] = 0; } else - xfree (s); - - s = (char *) xmalloc (len + strlen (value) + 2); - strcpy (s, var); - strcat (s, "="); - strcat (s, value); - vector[i] = s; - - /* This used to handle setting the PATH and GNUTARGET variables - specially. The latter has been replaced by "set gnutarget" - (which has worked since GDB 4.11). The former affects searching - the PATH to find SHELL, and searching the PATH to find the - argument of "symbol-file" or "exec-file". Maybe we should have - some kind of "set exec-path" for that. But in any event, having - "set env" affect anything besides the inferior is a bad idea. - What if we want to change the environment we pass to the program - without afecting GDB's behavior? */ - - return; + { + this->env.insert (std::make_pair (std::string (var), + std::string (value))); + } + + if (needs_update) + this->environ_vector.insert (this->environ_vector.end () - 1, + xstrdup (std::string (var + + std::string ("=") + + value).c_str ())); } -/* Remove the setting for variable VAR from environment E. */ +/* See common/environ.h. */ void -unset_in_environ (struct gdb_environ *e, const char *var) +gdb_environ::unset_in_environ (const char *var) { - int len = strlen (var); - char **vector = e->vector; - char *s; + this->env.erase (var); + unset_in_environ_vector (this->environ_vector, var); +} - for (; (s = *vector) != NULL; vector++) - { - if (strncmp (s, var, len) == 0 && s[len] == '=') - { - xfree (s); - /* Walk through the vector, shuffling args down by one, including - the NULL terminator. Can't use memcpy() here since the regions - overlap, and memmove() might not be available. */ - while ((vector[0] = vector[1]) != NULL) - { - vector++; - } - break; - } - } +/* See common/environ.h. */ + +char ** +gdb_environ::get_environ_char_vector (void) const +{ + return (char **) &this->environ_vector[0]; +} + +/* See common/environ.h. */ + +std::vector +gdb_environ::get_environ_vector (void) const +{ + return this->environ_vector; } diff --git a/gdb/common/environ.h b/gdb/common/environ.h index 3ace69e..c630425 100644 --- a/gdb/common/environ.h +++ b/gdb/common/environ.h @@ -17,33 +17,55 @@ #if !defined (ENVIRON_H) #define ENVIRON_H 1 -/* We manipulate environments represented as these structures. */ +#include +#include -struct gdb_environ - { - /* Number of usable slots allocated in VECTOR. - VECTOR always has one slot not counted here, - to hold the terminating zero. */ - int allocated; - /* A vector of slots, ALLOCATED + 1 of them. - The first few slots contain strings "VAR=VALUE" - and the next one contains zero. - Then come some unused slots. */ - char **vector; - }; +/* Class that represents the environment variables as seeing by the + inferior. */ -extern struct gdb_environ *make_environ (void); +class gdb_environ +{ +public: + /* Regular constructor and destructor. */ + gdb_environ (); + ~gdb_environ (); -extern void free_environ (struct gdb_environ *); + /* Reinitialize the environment stored. This is used when the user + wants to delete all environment variables added by him/her. */ + void reinit_environ (void); -extern void init_environ (struct gdb_environ *); + /* Return the value in the environment for the variable VAR. */ + char *get_in_environ (const char *var) const; -extern char *get_in_environ (const struct gdb_environ *, const char *); + /* Store VAR=VALUE in the environment. */ + void set_in_environ (const char *var, const char *value); -extern void set_in_environ (struct gdb_environ *, const char *, const char *); + /* Unset VAR in environment. */ + void unset_in_environ (const char *var); -extern void unset_in_environ (struct gdb_environ *, const char *); + /* Return the environment vector represented as a 'char **'. */ + char **get_environ_char_vector (void) const; -extern char **environ_vector (struct gdb_environ *); + /* Return the environment vector. */ + std::vector get_environ_vector (void) const; + +private: + /* Helper function that initializes our data structures with the + environment variables. */ + void init_environ (void); + + /* Helper function that clears our data structures. */ + void clear_environ (void); + + /* A map representing the environment variables and their values. + It is easier to store them using a map because of the faster + lookups. */ + std::unordered_map env; + + /* A vector containing the environment variables. This is useful + for when we need to obtain a 'char **' with all the existing + variables. */ + std::vector environ_vector; +}; #endif /* defined (ENVIRON_H) */ diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 22b2c7a..b1d0823 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -609,7 +609,8 @@ run_command_1 (char *args, int from_tty, int tbreak_at_main) the value now. */ run_target->to_create_inferior (run_target, exec_file, std::string (get_inferior_args ()), - environ_vector (current_inferior ()->environment), + current_inferior () + ->environment->get_environ_char_vector (), from_tty); /* to_create_inferior should push the target, so after this point we shouldn't refer to run_target again. */ @@ -2141,7 +2142,7 @@ environment_info (char *var, int from_tty) { if (var) { - char *val = get_in_environ (current_inferior ()->environment, var); + char *val = current_inferior ()->environment->get_in_environ (var); if (val) { @@ -2159,11 +2160,12 @@ environment_info (char *var, int from_tty) } else { - char **vector = environ_vector (current_inferior ()->environment); + std::vector vector = + current_inferior ()->environment->get_environ_vector (); - while (*vector) + for (char *&elem : vector) { - puts_filtered (*vector++); + puts_filtered (elem); puts_filtered ("\n"); } } @@ -2225,10 +2227,10 @@ set_environment_command (char *arg, int from_tty) printf_filtered (_("Setting environment variable " "\"%s\" to null value.\n"), var); - set_in_environ (current_inferior ()->environment, var, ""); + current_inferior ()->environment->set_in_environ (var, ""); } else - set_in_environ (current_inferior ()->environment, var, val); + current_inferior ()->environment->set_in_environ (var, val); xfree (var); } @@ -2240,13 +2242,10 @@ unset_environment_command (char *var, int from_tty) /* If there is no argument, delete all environment variables. Ask for confirmation if reading from the terminal. */ if (!from_tty || query (_("Delete all environment variables? "))) - { - free_environ (current_inferior ()->environment); - current_inferior ()->environment = make_environ (); - } + current_inferior ()->environment->reinit_environ (); } else - unset_in_environ (current_inferior ()->environment, var); + current_inferior ()->environment->unset_in_environ (var); } /* Handle the execution path (PATH variable). */ @@ -2257,8 +2256,8 @@ static void path_info (char *args, int from_tty) { puts_filtered ("Executable and object file path: "); - puts_filtered (get_in_environ (current_inferior ()->environment, - path_var_name)); + puts_filtered (current_inferior ()->environment->get_in_environ + (path_var_name)); puts_filtered ("\n"); } @@ -2271,13 +2270,13 @@ path_command (char *dirname, int from_tty) const char *env; dont_repeat (); - env = get_in_environ (current_inferior ()->environment, path_var_name); + env = current_inferior ()->environment->get_in_environ (path_var_name); /* Can be null if path is not set. */ if (!env) env = ""; exec_path = xstrdup (env); mod_path (dirname, &exec_path); - set_in_environ (current_inferior ()->environment, path_var_name, exec_path); + current_inferior ()->environment->set_in_environ (path_var_name, exec_path); xfree (exec_path); if (from_tty) path_info ((char *) NULL, from_tty); diff --git a/gdb/inferior.c b/gdb/inferior.c index 4ae265e..2529f1a 100644 --- a/gdb/inferior.c +++ b/gdb/inferior.c @@ -99,7 +99,6 @@ free_inferior (struct inferior *inf) inferior_free_data (inf); xfree (inf->args); xfree (inf->terminal); - free_environ (inf->environment); target_desc_info_free (inf->tdesc_info); xfree (inf->priv); xfree (inf); @@ -147,8 +146,7 @@ add_inferior_silent (int pid) last->next = inf; } - inf->environment = make_environ (); - init_environ (inf->environment); + inf->environment = std::unique_ptr (new gdb_environ); inferior_alloc_data (inf); diff --git a/gdb/inferior.h b/gdb/inferior.h index bf06ac1..c1d2f87 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -40,6 +40,9 @@ struct target_desc_info; /* For struct frame_id. */ #include "frame.h" +/* For gdb_environ. */ +#include "environ.h" + #include "progspace.h" #include "registry.h" @@ -356,7 +359,7 @@ struct inferior /* Environment to use for running inferior, in format described in environ.h. */ - struct gdb_environ *environment; + std::unique_ptr environment; /* Nonzero if this child process was attached rather than forked. */ diff --git a/gdb/mi/mi-cmd-env.c b/gdb/mi/mi-cmd-env.c index 4093178..0a909c9 100644 --- a/gdb/mi/mi-cmd-env.c +++ b/gdb/mi/mi-cmd-env.c @@ -167,7 +167,7 @@ mi_cmd_env_path (const char *command, char **argv, int argc) else { /* Otherwise, get current path to modify. */ - env = get_in_environ (current_inferior ()->environment, path_var_name); + env = current_inferior ()->environment->get_in_environ (path_var_name); /* Can be null if path is not set. */ if (!env) @@ -178,9 +178,9 @@ mi_cmd_env_path (const char *command, char **argv, int argc) for (i = argc - 1; i >= 0; --i) env_mod_path (argv[i], &exec_path); - set_in_environ (current_inferior ()->environment, path_var_name, exec_path); + current_inferior ()->environment->set_in_environ (path_var_name, exec_path); xfree (exec_path); - env = get_in_environ (current_inferior ()->environment, path_var_name); + env = current_inferior ()->environment->get_in_environ (path_var_name); uiout->field_string ("path", env); } @@ -270,7 +270,7 @@ mi_cmd_inferior_tty_show (const char *command, char **argv, int argc) void _initialize_mi_cmd_env (void) { - struct gdb_environ *environment; + std::unique_ptr environment (new gdb_environ); const char *env; /* We want original execution path to reset to, if desired later. @@ -278,13 +278,10 @@ _initialize_mi_cmd_env (void) current_inferior ()->environment. Also, there's no obvious place where this code can be moved such that it surely run before any code possibly mangles original PATH. */ - environment = make_environ (); - init_environ (environment); - env = get_in_environ (environment, path_var_name); + env = environment->get_in_environ (path_var_name); /* Can be null if path is not set. */ if (!env) env = ""; orig_path = xstrdup (env); - free_environ (environment); } diff --git a/gdb/solib.c b/gdb/solib.c index af94383..6260df9 100644 --- a/gdb/solib.c +++ b/gdb/solib.c @@ -350,16 +350,16 @@ solib_find_1 (const char *in_pathname, int *fd, int is_solib) /* If not found, next search the inferior's $PATH environment variable. */ if (found_file < 0 && sysroot == NULL) - found_file = openp (get_in_environ (current_inferior ()->environment, - "PATH"), + found_file = openp (current_inferior ()->environment->get_in_environ + ("PATH"), OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH, in_pathname, O_RDONLY | O_BINARY, &temp_pathname); /* If not found, and we're looking for a solib, next search the inferior's $LD_LIBRARY_PATH environment variable. */ if (is_solib && found_file < 0 && sysroot == NULL) - found_file = openp (get_in_environ (current_inferior ()->environment, - "LD_LIBRARY_PATH"), + found_file = openp (current_inferior ()->environment->get_in_environ + ("LD_LIBRARY_PATH"), OPF_TRY_CWD_FIRST | OPF_RETURN_REALPATH, in_pathname, O_RDONLY | O_BINARY, &temp_pathname);