Re: [PATCH 1/2 v2] RISC-V: Support ELF attribute for gas and readelf

[Jim Wilson <jimw at sifive dot com>]

On Wed, Jan 9, 2019 at 5:47 AM Kito Cheng <kito.cheng@gmail.com> wrote:
&gt; This patch implement RISC-V ELF attribute[1] support for
assembler and readelf.

It looks nice.  Adding the gas command line option and configure
option makes it safer to add the feature this close to a release.  If
there is a problem, people can disable it.

In the old code, there is one place that has "p =  " which has one too
many spaces after the equal sign, but that is a minor ignorable
problem.

In the new code, I see uses of "RISC-V used arch attribute" that I
think should just be "RISC-V arch attribute".  This appears in gas
configure and config/tc-riscv.c patches, including strings printed in
messages.

In the new docs, there are lines over 80 characters in length.

In the .attribute docs, there is a comma missing after
Tag_RISCV_unaligned_access, and a missing period at the end of the
line.

In the Tag_RISCV_arch docs, there are references to "-march" that
should be @option{-march) instead.  And I think stuff like "RV32I"
should be @code{RV32I} instead.  Basically, anyplace in the docs where
you use double quotes should be @option or @code instead.

In the Tag_RISCV_unaligned_access docs, it uses bullets instead of a
paragraph which is an odd style change in the middle of the
documentation.  I think it should be a paragraph like the rest.

There is some stuff in the docs that doesn't look quite right to a
native English speaker, but it is probably easier if I fix that
myself.  I could perhaps just fix the other stuff too.

Without the other patch to elf-attrs.c, I get some unresolved
testcases because of readelf errors.  WIth that elf-attrs.c patch, I
get some linker failures, but maybe there is a fix for that in the
second part of this patch set.  I'll look at the second part next, and
deal with the elf-attrs.c patch separately.

Jim