Re: [PATCH 6/8] Add support for the Rust language

[Pedro Alves <palves at redhat dot com>]

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