From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from barracuda.ebox.ca (barracuda.ebox.ca [96.127.255.19]) by sourceware.org (Postfix) with ESMTPS id A040A3864832 for ; Tue, 1 Feb 2022 14:07:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A040A3864832 X-ASG-Debug-ID: 1643724438-0c856e06ab2cc4e0001-fS2M51 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id FWLpGUoijarLDnUW (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 01 Feb 2022 09:07:18 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@polymtl.ca X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from simark.localdomain (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) by smtp.ebox.ca (Postfix) with ESMTP id 2CE83441D67; Tue, 1 Feb 2022 09:07:18 -0500 (EST) From: Simon Marchi X-Barracuda-RBL-IP: 192.222.157.6 X-Barracuda-Effective-Source-IP: 192-222-157-6.qc.cable.ebox.net[192.222.157.6] X-Barracuda-Apparent-Source-IP: 192.222.157.6 To: gdb-patches@sourceware.org Subject: [PATCH 4/4] gdb: make internalvar use a variant Date: Tue, 1 Feb 2022 09:07:17 -0500 X-ASG-Orig-Subj: [PATCH 4/4] gdb: make internalvar use a variant Message-Id: <20220201140717.3046952-5-simon.marchi@polymtl.ca> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20220201140717.3046952-1-simon.marchi@polymtl.ca> References: <20220201140717.3046952-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Barracuda-Connect: smtp.ebox.ca[96.127.255.82] X-Barracuda-Start-Time: 1643724438 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at ebox.ca X-Barracuda-Scan-Msg-Size: 19801 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 0.00 X-Barracuda-Spam-Status: No, SCORE=0.00 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests= X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.95723 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- X-Spam-Status: No, score=-3613.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_QUARANTINE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_SOFTFAIL, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Feb 2022 14:07:24 -0000 Change the union in internalvar to be a variant. This union holds different data, based on the internalvar kind, which is stored in internalvar::kind. The advantage of a variant here is that we can more easily use non-POD types in the alternatives. And since it remembers which alternative is the current one, accesses are checked so that accesses to the wrong alternative throw an exception (see question about that below). This is the first use of a variant in our code base, and it will probably be used as a template for future uses. So all suggestions about how the variant is used are welcome. Define one struct type for each kind of data we want to store in the variant (for each internalvar kind), that makes the alternatives clear. Remove the internalvar::kind field, as it becomes redundant with the variant. Keeping it just consumes more space, but most importantly we run the risk of having it and the variant out of sync. The variant's selector becomes the source of truth for the internalvar kind. However, I find that being able to get the kind of an internalvar as an enumerator quite handy. Plus, it would be quite a bit of work to rewrite all the code to replace if (var->kind == INTERNALVAR_VALUE) with if (nonstd::holds_alternative (var->v)) and switch (var->kind) with some functor + std::visit. I also think it makes the code less readable. So I added a kind method to internalvar, to get an internalvar_kind value from the variant's active alternative. That's perhaps not the most efficient thing, but it helps a lot. Open questions: - The library uses std::variant if it is available (so, if building with C++17). I think there are slight differences between the std and nonstd versions of variant. I can't really explain what they are, I am not expert enough in C++. But for example, when I had not made my visitor's methods const, the code would compile with std::variant but not nonstd::variant. This means we could get some occasional build failures if some people build in C++11/14 and others C++17. One way to avoid this would be to define the macro variant_CONFIG_SELECT_VARIANT so that we always use nonstd::variant, regardless of the C++ version. The small downside is that we wouldn't be testing against std::variant, so some day in the future we want to make the switch to use std::variant, we'll have some fixes to do. - The library has the option [2] of being built with or without exception. For example, that decides what to do if one tries to access an alternative that is not the active one: https://github.com/martinmoene/variant-lite/blob/f1af3518e4c28f12b09839b9d2ee37984cbf137a/include/nonstd/variant.hpp#L2107-L2114 With exceptions enabled, bad_variant_access is thrown. Without exceptions enabled, an assert() fails. In our context, it would be nice if we could call gdb_assert fails, such that we get the usual "internal error" message if that ever happens. I don't see an easy way to do this other than modifying the header file itself. [1] https://github.com/martinmoene/variant-lite#select-stdvariant-or-nonstdvariant [2] https://github.com/martinmoene/variant-lite/#disable-exceptions Change-Id: I15957e091f740441128301e2bc603771f18771b5 --- gdb/value.c | 334 ++++++++++++++++++++++++++++------------------------ 1 file changed, 183 insertions(+), 151 deletions(-) diff --git a/gdb/value.c b/gdb/value.c index 2c8af4b9d5b5..97ae469f670d 100644 --- a/gdb/value.c +++ b/gdb/value.c @@ -46,6 +46,7 @@ #include "cli/cli-style.h" #include "expop.h" #include "inferior.h" +#include "gdbsupport/variant/variant.hpp" /* Definition of a user function. */ struct internal_function @@ -1989,41 +1990,50 @@ enum internalvar_kind INTERNALVAR_STRING, }; -union internalvar_data +/* Used with INTERNALVAR_VOID. */ +struct internalvar_void +{}; + +/* Value object used with INTERNALVAR_VALUE. */ +struct internalvar_value { - /* A value object used with INTERNALVAR_VALUE. */ - struct value *value; + value_ref_ptr value; +}; - /* The call-back routine used with INTERNALVAR_MAKE_VALUE. */ - struct - { - /* The functions to call. */ - const struct internalvar_funcs *functions; +/* Call-back routine used with INTERNALVAR_MAKE_VALUE. */ +struct internalvar_make_value +{ + /* The functions to call. */ + const struct internalvar_funcs *functions; - /* The function's user-data. */ - void *data; - } make_value; + /* The function's user-data. */ + void *data; +}; - /* The internal function used with INTERNALVAR_FUNCTION. */ - struct - { - struct internal_function *function; - /* True if this is the canonical name for the function. */ - int canonical; - } fn; +/* The internal function used with INTERNALVAR_FUNCTION. */ +struct internalvar_function +{ + struct internal_function *function; - /* An integer value used with INTERNALVAR_INTEGER. */ - struct - { - /* If type is non-NULL, it will be used as the type to generate - a value for this internal variable. If type is NULL, a default - integer type for the architecture is used. */ - struct type *type; - LONGEST val; - } integer; + /* True if this is the canonical name for the function. */ + int canonical; +}; + +/* An integer value used with INTERNALVAR_INTEGER. */ + +struct internalvar_integer +{ + /* If type is non-NULL, it will be used as the type to generate + a value for this internal variable. If type is NULL, a default + integer type for the architecture is used. */ + struct type *type; + LONGEST val; +}; +struct internalvar_string +{ /* A string value used with INTERNALVAR_STRING. */ - char *string; + std::string string; }; /* Internal variables. These are variables within the debugger @@ -2033,16 +2043,45 @@ union internalvar_data struct internalvar { - struct internalvar *next; - char *name; + /* Return the kind of the internalvar. */ + internalvar_kind kind () const + { + struct visitor + { + internalvar_kind operator() (const internalvar_void &void_) const + { return INTERNALVAR_VOID; } + + internalvar_kind operator() (const internalvar_value &value) const + { return INTERNALVAR_VALUE; } + + internalvar_kind operator() (const internalvar_make_value &make_value) const + { return INTERNALVAR_MAKE_VALUE; } + + internalvar_kind operator() (const internalvar_function &void_) const + { return INTERNALVAR_FUNCTION; } + + internalvar_kind operator() (const internalvar_integer &integer) const + { return INTERNALVAR_INTEGER; } - /* We support various different kinds of content of an internal variable. - enum internalvar_kind specifies the kind, and union internalvar_data - provides the data associated with this particular kind. */ + internalvar_kind operator() (const internalvar_string &string) const + { return INTERNALVAR_STRING; } + }; - enum internalvar_kind kind; + return nonstd::visit (visitor {}, this->v); + } - union internalvar_data u; + struct internalvar *next = nullptr; + char *name = nullptr; + + /* Data associated to each kind of internalvar. The active variant + alternative indicates the kind of the internalvar. */ + nonstd::variant< + internalvar_void, + internalvar_value, + internalvar_make_value, + internalvar_function, + internalvar_integer, + internalvar_string> v; }; static struct internalvar *internalvars; @@ -2081,7 +2120,7 @@ init_if_undefined_command (const char* args, int from_tty) /* Only evaluate the expression if the lvalue is void. This may still fail if the expression is invalid. */ - if (intvar->kind == INTERNALVAR_VOID) + if (intvar->kind () == INTERNALVAR_VOID) evaluate_expression (expr.get ()); } @@ -2126,12 +2165,13 @@ complete_internalvar (completion_tracker &tracker, const char *name) struct internalvar * create_internalvar (const char *name) { - struct internalvar *var = XNEW (struct internalvar); + struct internalvar *var = new internalvar; var->name = xstrdup (name); - var->kind = INTERNALVAR_VOID; var->next = internalvars; internalvars = var; + var->v = internalvar_void {}; + return var; } @@ -2148,10 +2188,7 @@ create_internalvar_type_lazy (const char *name, void *data) { struct internalvar *var = create_internalvar (name); - - var->kind = INTERNALVAR_MAKE_VALUE; - var->u.make_value.functions = funcs; - var->u.make_value.data = data; + var->v = internalvar_make_value { funcs, data }; return var; } @@ -2162,12 +2199,15 @@ compile_internalvar_to_ax (struct internalvar *var, struct agent_expr *expr, struct axs_value *value) { - if (var->kind != INTERNALVAR_MAKE_VALUE - || var->u.make_value.functions->compile_to_ax == NULL) + if (var->kind () != INTERNALVAR_MAKE_VALUE) return 0; - var->u.make_value.functions->compile_to_ax (var, expr, value, - var->u.make_value.data); + const auto &make_value = nonstd::get (var->v); + + if (make_value.functions->compile_to_ax == nullptr) + return 0; + + make_value.functions->compile_to_ax (var, expr, value, make_value.data); return 1; } @@ -2213,7 +2253,7 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var) return val; } - switch (var->kind) + switch (var->kind ()) { case INTERNALVAR_VOID: val = allocate_value (builtin_type (gdbarch)->builtin_void); @@ -2224,28 +2264,45 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var) break; case INTERNALVAR_INTEGER: - if (!var->u.integer.type) - val = value_from_longest (builtin_type (gdbarch)->builtin_int, - var->u.integer.val); - else - val = value_from_longest (var->u.integer.type, var->u.integer.val); - break; + { + const auto &integer = nonstd::get (var->v); + + if (integer.type == nullptr) + val = value_from_longest (builtin_type (gdbarch)->builtin_int, + integer.val); + else + val = value_from_longest (integer.type, integer.val); + + break; + } case INTERNALVAR_STRING: - val = value_cstring (var->u.string, strlen (var->u.string), - builtin_type (gdbarch)->builtin_char); - break; + { + const auto &string = nonstd::get (var->v); + + val = value_cstring (string.string.c_str (), string.string.size (), + builtin_type (gdbarch)->builtin_char); + break; + } case INTERNALVAR_VALUE: - val = value_copy (var->u.value); - if (value_lazy (val)) - value_fetch_lazy (val); - break; + { + const auto &value = nonstd::get (var->v); + + val = value_copy (value.value.get ()); + if (value_lazy (val)) + value_fetch_lazy (val); + + break; + } case INTERNALVAR_MAKE_VALUE: - val = (*var->u.make_value.functions->make_value) (gdbarch, var, - var->u.make_value.data); - break; + { + const auto &make_value = nonstd::get (var->v); + + val = make_value.functions->make_value (gdbarch, var, make_value.data); + break; + } default: internal_error (__FILE__, __LINE__, _("bad kind")); @@ -2268,8 +2325,7 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var) altogether. At the moment, this seems like the behavior we want. */ - if (var->kind != INTERNALVAR_MAKE_VALUE - && val->lval != lval_computed) + if (var->kind () != INTERNALVAR_MAKE_VALUE && val->lval != lval_computed) { VALUE_LVAL (val) = lval_internalvar; VALUE_INTERNALVAR (val) = var; @@ -2281,19 +2337,21 @@ value_of_internalvar (struct gdbarch *gdbarch, struct internalvar *var) int get_internalvar_integer (struct internalvar *var, LONGEST *result) { - if (var->kind == INTERNALVAR_INTEGER) + if (var->kind () == INTERNALVAR_INTEGER) { - *result = var->u.integer.val; + const auto &integer = nonstd::get (var->v); + *result = integer.val; return 1; } - if (var->kind == INTERNALVAR_VALUE) + if (var->kind () == INTERNALVAR_VALUE) { - struct type *type = check_typedef (value_type (var->u.value)); + const auto &value = nonstd::get (var->v); + struct type *type = check_typedef (value_type (value.value.get ())); if (type->code () == TYPE_CODE_INT) { - *result = value_as_long (var->u.value); + *result = value_as_long (value.value.get ()); return 1; } } @@ -2305,11 +2363,14 @@ static int get_internalvar_function (struct internalvar *var, struct internal_function **result) { - switch (var->kind) + switch (var->kind ()) { case INTERNALVAR_FUNCTION: - *result = var->u.fn.function; - return 1; + { + const auto &function = nonstd::get (var->v); + *result = function.function; + return 1; + } default: return 0; @@ -2325,20 +2386,23 @@ set_internalvar_component (struct internalvar *var, struct gdbarch *arch; int unit_size; - switch (var->kind) + switch (var->kind ()) { case INTERNALVAR_VALUE: - addr = value_contents_writeable (var->u.value).data (); - arch = get_value_arch (var->u.value); - unit_size = gdbarch_addressable_memory_unit_size (arch); - - if (bitsize) - modify_field (value_type (var->u.value), addr + offset, - value_as_long (newval), bitpos, bitsize); - else - memcpy (addr + offset * unit_size, value_contents (newval).data (), - TYPE_LENGTH (value_type (newval))); - break; + { + const auto &value = nonstd::get (var->v); + addr = value_contents_writeable (value.value.get ()).data (); + arch = get_value_arch (value.value.get ()); + unit_size = gdbarch_addressable_memory_unit_size (arch); + + if (bitsize) + modify_field (value_type (value.value.get ()), addr + offset, + value_as_long (newval), bitpos, bitsize); + else + memcpy (addr + offset * unit_size, value_contents (newval).data (), + TYPE_LENGTH (value_type (newval))); + break; + } default: /* We can never get a component of any other kind. */ @@ -2349,29 +2413,30 @@ set_internalvar_component (struct internalvar *var, void set_internalvar (struct internalvar *var, struct value *val) { - enum internalvar_kind new_kind; - union internalvar_data new_data = { 0 }; - - if (var->kind == INTERNALVAR_FUNCTION && var->u.fn.canonical) + if (var->kind () == INTERNALVAR_FUNCTION + && nonstd::get (var->v).canonical) error (_("Cannot overwrite convenience function %s"), var->name); /* Prepare new contents. */ switch (check_typedef (value_type (val))->code ()) { case TYPE_CODE_VOID: - new_kind = INTERNALVAR_VOID; + var->v = internalvar_void {}; break; case TYPE_CODE_INTERNAL_FUNCTION: - gdb_assert (VALUE_LVAL (val) == lval_internalvar); - new_kind = INTERNALVAR_FUNCTION; - get_internalvar_function (VALUE_INTERNALVAR (val), - &new_data.fn.function); - /* Copies created here are never canonical. */ - break; + { + gdb_assert (VALUE_LVAL (val) == lval_internalvar); + + internal_function *func; + get_internalvar_function (VALUE_INTERNALVAR (val), &func); + + /* Copies created here are never canonical. */ + var->v = internalvar_function { func, 0 }; + break; + } default: - new_kind = INTERNALVAR_VALUE; struct value *copy = value_copy (val); copy->modifiable = 1; @@ -2382,10 +2447,8 @@ set_internalvar (struct internalvar *var, struct value *val) value_fetch_lazy (copy); /* Release the value from the value chain to prevent it from being - deleted by free_all_values. From here on this function should not - call error () until new_data is installed into the var->u to avoid - leaking memory. */ - new_data.value = release_value (copy).release (); + deleted by free_all_values. */ + var->v = internalvar_value { release_value (copy) }; /* Internal variables which are created from values with a dynamic location don't need the location property of the origin anymore. @@ -2393,73 +2456,35 @@ set_internalvar (struct internalvar *var, struct value *val) when accessing the value. If we keep it, we would still refer to the origin value. Remove the location property in case it exist. */ - value_type (new_data.value)->remove_dyn_prop (DYN_PROP_DATA_LOCATION); - + value_type (copy)->remove_dyn_prop (DYN_PROP_DATA_LOCATION); break; } - - /* Clean up old contents. */ - clear_internalvar (var); - - /* Switch over. */ - var->kind = new_kind; - var->u = new_data; - /* End code which must not call error(). */ } void set_internalvar_integer (struct internalvar *var, LONGEST l) { - /* Clean up old contents. */ - clear_internalvar (var); - - var->kind = INTERNALVAR_INTEGER; - var->u.integer.type = NULL; - var->u.integer.val = l; + var->v = internalvar_integer { nullptr, l }; } void set_internalvar_string (struct internalvar *var, const char *string) { - /* Clean up old contents. */ - clear_internalvar (var); - - var->kind = INTERNALVAR_STRING; - var->u.string = xstrdup (string); + var->v = internalvar_string { string }; } static void set_internalvar_function (struct internalvar *var, struct internal_function *f) { - /* Clean up old contents. */ - clear_internalvar (var); - - var->kind = INTERNALVAR_FUNCTION; - var->u.fn.function = f; - var->u.fn.canonical = 1; /* Variables installed here are always the canonical version. */ + var->v = internalvar_function { f, 1 }; } void clear_internalvar (struct internalvar *var) { - /* Clean up old contents. */ - switch (var->kind) - { - case INTERNALVAR_VALUE: - value_decref (var->u.value); - break; - - case INTERNALVAR_STRING: - xfree (var->u.string); - break; - - default: - break; - } - /* Reset to void kind. */ - var->kind = INTERNALVAR_VOID; + var->v = internalvar_void {}; } const char * @@ -2579,17 +2604,24 @@ static void preserve_one_internalvar (struct internalvar *var, struct objfile *objfile, htab_t copied_types) { - switch (var->kind) + switch (var->kind ()) { case INTERNALVAR_INTEGER: - if (var->u.integer.type - && var->u.integer.type->objfile_owner () == objfile) - var->u.integer.type - = copy_type_recursive (objfile, var->u.integer.type, copied_types); - break; + { + auto &integer = nonstd::get (var->v); + + if (integer.type != nullptr + && integer.type->objfile_owner () == objfile) + integer.type + = copy_type_recursive (objfile, integer.type, copied_types); + + break; + } case INTERNALVAR_VALUE: - preserve_one_value (var->u.value, objfile, copied_types); + preserve_one_value + (nonstd::get (var->v).value.get (), objfile, + copied_types); break; } } -- 2.34.1