This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 2/6] RISC-V gas port
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