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 Mon, 17 Oct 2016 05:09:06 PDT (-0700), wrote:
>> 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.

As far as I know, the GCC test suite won't do a good job testing disassembly.
I think I'm going to hold off on committing a larger test suite until later.
Doing a good job here is going to be a lot of work, and I think doing a bad job
won't get us anything.

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

If there were any copyrights that don't include 2016 then I just missed them.
That's likely, as I miss these a lot.

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

Good news: this error handling code has been removed and is replaced with a
more robust way to pass around the floating point ABIs.

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

Thanks!  I'll try and get out a v2 today.

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