This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/6] RISC-V gas port

On Fri, 14 Oct 2016 07:12:00 PDT (-0700), wrote:
> 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.

I've been avoiding any ChangeLog stuff because they always cause merge
conflicts.  Do you want them included in the patch or in the email that goes
along with the 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.

How do these look?

I have no idea how to format the NEWS entry, sorry.

I know the test case is pretty anemic, but I'm not sure how to run the GAS test
suite.  We've just been running GCC tests.  There is a RISC-V ISA test suite
that I can copy into the GAS port if you want, but I generally don't like
having copies of stuff floating around.

I'm also not sure how to build or view the texinfo documentation.

>   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...)

Our port was originally based on the MIPS port, but also contains a lot of code
from the Tilera and ARMv8 ports.  Since we started by copying a lot of their
files we left the copyright headers intact.  If that's not how it's supposed to
work then I can go change all the copyright dates, but my understand is that
these files are actually copyright FSF as of whatever their dates say.

>> +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.

I think this is actually correct.  The idea here is that ELF files in RISC-V
can be tagged with the floating-point ABI.  This lets us avoid linking ELFs
with incompatible ABIs, much like 32-bit vs 64-bit ELFs.  So in this case I
believe the "for an ELF" is necessary -- without it users might thing there's
something wrong with their GAS command-line arguments.

>> +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.

I think I got them all:

struct option md_longopts[] =
  {"m32", no_argument, NULL, OPTION_M32},
  {"m64", no_argument, NULL, OPTION_M64},
  {"march", required_argument, NULL, OPTION_MARCH},
  {"fPIC", no_argument, NULL, OPTION_PIC},
  {"fpic", no_argument, NULL, OPTION_PIC},
  {"fno-pic", no_argument, NULL, OPTION_NO_PIC},
  {"mrvc", no_argument, NULL, OPTION_MRVC},
  {"mno-rvc", no_argument, NULL, OPTION_MNO_RVC},
  {"msoft-float", no_argument, NULL, OPTION_MSOFT_FLOAT},
  {"mhard-float", no_argument, NULL, OPTION_MHARD_FLOAT},

  {NULL, no_argument, NULL, 0}

> Cheers
>   Nick

Thanks for the comments!

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]