[PATCH 2/6] RISC-V gas port

Nick Clifton nickc@redhat.com
Fri Oct 14 14:12:00 GMT 2016


Hi Andrew,

  As an aside I would like to point out that it would be helpful if you 
  could include proposed ChangeLog entries for each of these patches.

  Also with a new target like this, an entry in the gas/NEWS file is
  appropriate.  Plus I notice that your patch does not add a new 
  gas/doc/c-riscv.texi file, nor does it update gas/doc/as.texinfo,
  and it does not appear to contain a new sub-directory in 
  gas/testsuite/gas for RISC-V specific tests.  All of these would
  really be appreciated.

  Anyway here are some comments on the gas patch:

> +/* tc-riscv.c -- RISC-V assembler
> +   Copyright 2011-2015 Free Software Foundation, Inc.

  Given that this file is only being contributed in 2016, it is difficult
  to see how the FSF can claim copyright for 2011..2015. :-)  (The same
  thing applies to the other new files too...)
 
> +riscv_set_arch (const char *arg)
> +{
> +  char *uppercase = xstrdup (arg);
> +  char *p = uppercase;
> +  const char *all_subsets = "IMAFDC";
> +  const char *extension = NULL;
> +  int rvc = 0;
> +  int i;
> +
> +  for (i = 0; uppercase[i]; i++)
> +    uppercase[i] = TOUPPER (uppercase[i]);

  You could save code space and memory allocation by using strncasecmp
  instead if strncmp...

> +/* handle of the OPCODE hash table */

  Comment formatting - upper case first letter, period and two spaces at
  the end.
 
> +static int
> +relaxed_branch_length (fragS *fragp, asection *sec, int update)

  Can a relaxed branch ever have a negative length ?  If not then "unsigned int"
  would be a more appropriate return type.

> +static int
> +arg_lookup (char **s, const char *const *array, size_t size, unsigned *regnop)

  Similarly this appears to be a boolean function so bfd_boolean would be a more
  appropriate return type.  (Actually there appear to be quite a few functions that
  should be returning a boolean).

 
> +append_insn (struct riscv_cl_insn *ip, expressionS *address_expr,
> +	     bfd_reloc_code_real_type reloc_type)
> +{
> +#ifdef OBJ_ELF
> +  dwarf2_emit_insn (0);
> +#endif

  Are you supporting file formats other than ELF ?  If not, then there
  is no need for the conditionals around this code.
  
> +  switch (elf_float_mode)
> +    {
> +    case FLOAT_MODE_DEFAULT:
> +      as_bad ("a specific float mode must be specified for an ELF");
> +      break;

  This error message looks a little wrong to me.  I think that
  dropping the "for an ELF" part would make more sense.


> +void
> +md_show_usage (FILE *stream)
> +{
> +  fprintf (stream, _("\
> +RISC-V options:\n\
> +  -m32           assemble RV32 code\n\
> +  -m64           assemble RV64 code (default)\n\
> +  -fpic          generate position-independent code\n\
> +  -fno-pic       don't generate position-independent code (default)\n\
> +"));

  There are more command line options supported than those show here.
  -march, -mrvc, -msoft-float and -mhard-float for example.

Cheers
  Nick



More information about the Binutils mailing list