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

[Nick Clifton <nickc at redhat dot com>]

Hi Palmer,

>>   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?

In the email alongside the patches please.  Just as plain text, not a context
diff.  As you say the changelogs update so frequently that a changelog in the
patch almost never applies cleanly.


>>   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?

Very nice.

>   https://github.com/riscv/riscv-binutils-gdb/commit/84fbe7dc2a007898e8b9cbfa865313f979db09e3
>   https://github.com/riscv/riscv-binutils-gdb/commit/75564972be7916567432daf33b91775e581b646d
>   https://github.com/riscv/riscv-binutils-gdb/commit/e028448560e2c77d36da572e5ef206e6d9b44873
> 
> I have no idea how to format the NEWS entry, sorry.

Just as you did is fine.

> I know the test case is pretty anemic, but I'm not sure how to run the GAS test
> suite.

GAS tests are run in pretty much the same way as GCC tests.  You build a toolchain,
go into the gas directory and run "make check".

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

Do the GCC tests just check the assembler output from the compiler, or do they
also include checking the output of the assembler and disassembler ?  If they do
all three things then I agree that the GAS tests would be mostly redundant.  But
they do still serve a purpose.  Or several really - they can be run without needing
a compiler - they can test the generation of instructions that the compiler will
not generate - they can test assembler features that the compiler does not use.

I will not insist on an assembler testsuite being included as part of the 
contribution, but I do think that it is in your interest to have one.  Perhaps as a 
later update.

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

Running "make info" in the gas build directory should do the trick.  Then running
info ./doc/as.info should allow you to read the documentation.  Or running 
"nroff -man ./doc/as.1" to check the manual page.

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

Fair enough - but please could you extend the dates to include 2016, since that
is the year in which the sources will be added to the repository.

 
> https://github.com/riscv/riscv-binutils-gdb/commit/31454d5265bec2733075daf668b6ba70fd6e99ff
> 
>>> +  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.

OK, but "ELF" is just a binary file format - ie an abstract thing - and not a
physical object like an actual file.  Most users would, at least in my opinion,
refer to binary files as "object files" or "executables" or "libraries", not
"ELFs".  So how about this alternative wording:

  a specific float mode must be specified for the output file

or even:

  a specific float mode must be specified in order for the output file to be valid

or just:

  please specify the float mode for the output

Cheers
  Nick

PS. I am going to wait for the v2 patches to be posted and then try reviewing 
en masse again.