From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 56737 invoked by alias); 27 Apr 2016 11:43:51 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 56634 invoked by uid 89); 27 Apr 2016 11:43:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=lex, discriminant, bison, Lex 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 27 Apr 2016 11:43:38 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1AD5F81F03; Wed, 27 Apr 2016 11:43:37 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u3RBhWmh017139; Wed, 27 Apr 2016 07:43:33 -0400 Subject: Re: [PATCH 6/8] Add support for the Rust language To: Tom Tromey , gdb-patches@sourceware.org References: <1461725371-17620-1-git-send-email-tom@tromey.com> <1461725371-17620-7-git-send-email-tom@tromey.com> From: Pedro Alves Message-ID: <5720A5E4.2020603@redhat.com> Date: Wed, 27 Apr 2016 11:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 MIME-Version: 1.0 In-Reply-To: <1461725371-17620-7-git-send-email-tom@tromey.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-04/txt/msg00583.txt.bz2 On 04/27/2016 03:49 AM, Tom Tromey wrote: > This patch adds support for the Rust language. This all looks very well done to me. Though it's huge and I doubt anyone else would be able to comment on Rust-specific design issues. So from the bike-shed-vs-nuclear-power-plant department, I'll point out some minor nits below. This is good to go once the below are resolved somehow. > @@ -9225,6 +9226,11 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu) > if (cu->producer && strstr (cu->producer, "GNU Go ") != NULL) > set_cu_language (DW_LANG_Go, cu); > > + /* Rust does not currently emit DW_LANG_Rust or DW_LANG_Rust_old. "Currently" is a moving target. Could you replace it with some version number or some such? > + See https://github.com/rust-lang/rust/pull/33097. */ > + if (cu->producer && strstr (cu->producer, "rustc ") != NULL) > + set_cu_language (DW_LANG_Rust, cu); > + > @@ -14966,6 +14975,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu) > case language_d: > case language_java: > case language_objc: > + case language_rust: > low.data.const_val = 0; > low_default_is_valid = (cu->header.version >= 4); > break; > @@ -17061,6 +17071,10 @@ set_cu_language (unsigned int lang, struct dwarf2_cu *cu) > default: > cu->language = language_minimal; > break; > + case DW_LANG_Rust: > + case DW_LANG_Rust_old: > + cu->language = language_rust; > + break; I think it'd read better to move this to before the "default:" case. > } > cu->language_defn = language_def (cu->language); > } > + > +#define RUSTSTYPE YYSTYPE > + > +#ifndef RUSTDEBUG > +#define RUSTDEBUG 1 /* Default to debug support */ > +#endif Is this referenced anywhere, or a leftoever from when this used the bison prefix support? I think that all you have is YYDEBUG. > + > +#define YYFPRINTF parser_fprintf This one's already done by yy-remap.h. > + { > + /* This is a gdb extension to make it possible to > + refer to items in other crates. It just bypasses > + adding the current crate to the front of the > + name. */ > + $$ = ast_path (rust_concat3 ("::", $2->left.sval.ptr, NULL), > + $2->right.params); > + } I haven't cross-checked, but, it this extension documented? > +/* A helper to look up a Rust type, or fail. This only works for > + types defined by rust_language_arch_info. */ > + > +static struct type * > +rust_type (const char *name) > +{ > + struct type *type; > + > + if (unit_testing) > + return NULL; I think it'd be good to have a comment on use of unit_testing here. > + > + type = language_lookup_primitive_type (parse_language (pstate), > + parse_gdbarch (pstate), > + name); > + if (type == NULL) > + error (_("Could not find Rust type %s"), name); > + return type; > +} > + > +/* Return the offset of the double quote if STR looks like the start > + of a raw string, or 0 if STR does not start a raw string.. */ Spurious double period. > + > +static int > +starts_raw_string (const char *str) > +{ > + > +/* Lex a number. */ > + > +static int > +lex_number (void) > +{ > + regmatch_t subexps[8]; Magical number 8 is magical. > + int match; > + int is_integer = 0; > + int could_be_decimal = 1; > + char *type_name = NULL; > + struct type *type; > + int end_index; > + int type_index = -1; > + int i, out; > + char *number; > + struct cleanup *cleanup = make_cleanup (null_cleanup, NULL); > + > + match = regexec (&number_regex, lexptr, ARRAY_SIZE (subexps), subexps, 0); > + /* Failure means the regexp is broken. */ > + gdb_assert (!match); gdb_assert (match == 0); > +/* Make a structure creation operation. */ > + > +static const struct rust_op * > +ast_struct (const struct rust_op *name, VEC (set_field) **fields) > +{ > + struct rust_op *result = OBSTACK_ZALLOC (&work_obstack, struct rust_op); > + > + /* We treat this differently than Ada. */ What does "differently" mean? Similar to the Fortran opcode, did you think about promoting OP_AGGREGATE out of ada-operator.def ? > + result->opcode = OP_AGGREGATE; > + result->left.op = name; > + result->right.field_inits = fields; > + > + return result; > +} > + > +/* Create an AST node describing a pointer type. */ > + > +static const struct rust_op * > +ast_pointer_type (const struct rust_op *type, int is_mut) > +{ > + struct rust_op *result = ast_basic_type (TYPE_CODE_PTR); > + > + result->left.op = type; > + /* DO we care about is_mut? */ Who would know? Not me. :-) > + > + case UNOP_CAST: > + { > + struct type *type = convert_ast_to_type (state, operation->right.op); > + > + convert_ast_to_expression (state, operation->left.op, top); > + write_exp_elt_opcode (state, UNOP_CAST); > + write_exp_elt_type (state, type); > + write_exp_elt_opcode (state, UNOP_CAST); > + } > + break; > + > + case OP_FUNCALL: > + { > + if (operation->left.op->opcode == OP_VAR_VALUE) > + { > + struct type *type; > + const char *varname = convert_name (state, operation->left.op); > + > + type = rust_lookup_type (varname, expression_context_block); > + if (type != NULL) > + { > + /* This is actually a tuple struct expression, not a > + call expression. */ > + rust_op_ptr elem; > + int i; > + VEC (rust_op_ptr) *params = *operation->right.params; > + > + if (TYPE_CODE (type) == TYPE_CODE_NAMESPACE) > + goto got_ns; This goto will probably need to go away with C++-ification. Maybe the got_ns label label could be a helper function, called here, and where it is currently defined. However, other .y files use gotos as well, so guess it shouldn't be a requirement. > + > +/* The parser error handler. */ > + > +void > +rustyyerror (char *msg) > +{ > + const char *where = prev_lexptr ? prev_lexptr : lexptr; > + error (_("A %s in expression, near `%s'."), (msg ? msg : "error"), where); _("error"), I suppose. I note this expands to the non-grammatical "A error". Maybe reword to avoid it? Maybe drop the "A" ? > + > +/* The prefix of a specially-encoded enum. */ > + > +#define RUST_ENUM_PREFIX "RUST$ENCODED$ENUM$" > + > +/* The number of the real field. */ > + > +#define RUST_ENCODED_ENUM_REAL 0 > + > +/* The number of the hidden field. */ > + > +#define RUST_ENCODED_ENUM_HIDDEN 1 > + > +/* Utility function to get discriminant info for a given value. */ > + > +static struct disr_info > +rust_get_disr_info (struct type *type, const gdb_byte *valaddr, Did you mean to name these discr_info and rust_get_discr_info ? I mean, the missing 'c'. > + int embedded_offset, CORE_ADDR address, > + const struct value *val) > +{ > + int i; > + struct disr_info ret; > + struct type *disr_type; > + struct ui_file *temp_file; > + struct value_print_options opts; > + struct cleanup *cleanup; > + const char *name_segment; > + > + get_no_prettyformat_print_options (&opts); > + > + ret.field_no = -1; > + ret.is_encoded = 0; > + > + if (TYPE_NFIELDS (type) == 0) > + error (_("Encountered void enum value")); > + > + /* If an enum has two values wher one is empty and the other holds a "where". > + pointer that cannot be zero; then the rust compiler optimizes "Rust", uppercase, I guess. > + away the discriminant and instead uses a zero value in the > + pointer field to indicate the empty variant. */ > + if (strncmp (TYPE_FIELD_NAME (type, 0), RUST_ENUM_PREFIX, > + strlen (RUST_ENUM_PREFIX)) == 0) > + { > + char *tail; > + unsigned long fieldno; > + struct type *member_type; > + LONGEST value; > + > + ret.is_encoded = 1; > + > + if (TYPE_NFIELDS (type) != 1) > + error (_("Only expected one field in " RUST_ENUM_PREFIX " type")); >From i18n perspective, should probably be: error (_("Only expected one field in %s type"), RUST_ENUM_PREFIX); > + > + fieldno = strtoul (TYPE_FIELD_NAME (type, 0) + strlen (RUST_ENUM_PREFIX), > + &tail, 10); > + if (*tail != '$') > + error (_("Invalid form for " RUST_ENUM_PREFIX)); Ditto. > + > + member_type = TYPE_FIELD_TYPE (type, 0); > + if (fieldno >= TYPE_NFIELDS (member_type)) > + error (_(RUST_ENUM_PREFIX " refers to field after end of member type")); Etc. > + ret.name = ui_file_xstrdup (temp_file, NULL); > + name_segment = rust_last_path_segment (ret.name); > + if (name_segment != NULL) > + { > + for (i = 0; i < TYPE_NFIELDS (type); ++i) > + { > + /* Sadly, the discriminant value paths do not match the type > + field name paths ('core::option::Option::Some' vs > + 'core::option::Some') However, enum variant names are Missing period and double space after parens. > + unique in the last path segment and the generics are not > + part of this path, so we can just compare those. This is > + hackish and would be better fixed by improving rustc's > + metadata for enums. */ > +/* Return true if all non-static fields of a structlike type are in a > + sequence like __0, __1, __2. OFFSET lets us skip fields. */ > + > +static int > +rust_underscore_fields (struct type *type, int offset) > +{ > + int i, field_number; > + > + field_number = 0; > + > + if (TYPE_CODE (type) != TYPE_CODE_STRUCT) > + return 0; > + for (i = 0; i < TYPE_NFIELDS (type); ++i) > + { > + if (!field_is_static (&TYPE_FIELD (type, i))) > + { > + if (offset > 0) > + offset--; > + else > + { > + char buf[20]; > + > + xsnprintf (buf, 20, "__%d", field_number); sizeof buf. > + if (strcmp (buf, TYPE_FIELD_NAME (type, i)) != 0) > + return 0; > + field_number++; > + Thanks, Pedro Alves