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

[Jim Wilson]

On Fri, Dec 7, 2018 at 4:34 AM Kito Cheng <kito.cheng@gmail.com> wrote:
> This patch implement RISC-V ELF attribute[1] support for assembler and readelf.

Tags should have RISCV in the name, to avoid potential conflicts with Tags
defined for other architectures.  This requires a change to the
riscv-elf-psabi-doc pull request.  I'd suggest Tag_RISCV_priv_spec for
instance.  We aren't using .gnu_attribute, but rather adding this info to
the psABI, and adding a .attribute directive, so we don't need GNU in the name.

riscv_elf_obj_attrs_arg_type has a default case that handles tags that match
the documented rules, but there are no such tags that can match.

In the gas manual, in the .gnu_attribute section, it mentions that if an
attribute is odd it takes a string value, and if it is even if takes an integer
value.  It also says that the second bit (0x2) marks whether the tag is
arch independent or arch dependent.  It looks like a few targets are trying
to follow this rule, e.g. include/elf/ppc.h numbers their tags 4, 8, 12 to
comply.  However, tic6x only follows the even/odd rule.  ARM, the first target
to have Tags, only follows the even/odd rule, and only for "new" Tags numbered
32 and up.  I think the rule for arch independent Tag is pointless as we only
have one and no one seems to be using it.  We probably should try to follow the
even/odd rule though.  Things get a bit simpler if you do that.  You can
simplify the riscv_elf_attrs_arg_type function, and then the default case is no
longer useless.  It can be used for Tag_RISCV_arch for instance.  Note that
if you follow the even/odd rule, and someone uses an old readelf binary with
a new RISC-V binary using new Tags, then readelf will still be able to parse
it, it just won't be able to pretty print the new Tag names.  Otherwise,
readelf has to exit with an error without printing anything, because there is
no way to find the next attribute if you don't know if new unrecognized
attributes tag have a string or int argument.  So the even/odd rule gives us
better forward compatibility.

It isn't clear why priv_spec, priv_spec_minor, priv_spec_revision,
unsigned_access, and stack_align are marked as NO_DEFAULT.  It looks like
only ARM uses NO_DEFAULT, and it only uses it for one Tag, Tag_nodefaults.
The ARM ABI docs say that if Tag_nodefaults appears, then all following tags
in the same scope are undefined if the arg is missing.  Though the
implementation doesn't seem to do that, so maybe I'm reading it wrong, or it
wasn't implemented right for ARM.  Anyways, if this works the way ARM
documented it, then you are using it wrong.  I think it should be left out.
This simplifies riscv_elf_obj_attrs_arg_type even more, and maybe lets you
eliminate it with some tag renumbering.

elf_backend_obj_attr_arg_type is missing from ChangeLog entry.

The new section type, SHT_RISCV_ATTRIBUTES, should be documented in
riscv-elf-psabi-doc.  This can be added to the existing pull request.

In riscv_estimate_arch_strlen1, "for estimate" should be "to estimate" or
"for estimating".

Similarly with riscv_arch_str1.

In riscv_arch_str comment, "explicity" -> "explicit".

display_riscv_attribute has a default case that handles tags that match the
documented rules, but there are no such tags that can match.

See the comments above for tag numbers.  This can be simplified if we follow
the even/odd rule for tags, and then the default case is no longer useless.

In display_riscv_attribute, in the Tag_arch case, there are two spaces after
the equal sign in the line setting p.

display_riscv_attribute assumes that the tag will always be found in the table.
This isn't true if an old readelf is used with a new binary.  The code will
do a out-of-bounds array access in that case.  Should check for that case,
and print something sensible like "Unknown".  If you use the even/odd rule,
then you can go ahead and parse the argument even if you don't know the tag
name.

Documentation for the new gas .attribute directive is missing.  Curiously,
you have a comment saying that doc/c-riscv.texi must be kept in sync with
the code, but no doc/c-riscv.texi patch.

s_riscv_attribute calls riscv_set_arch if a Tag_arch is seen.  That lets
someone change the architecture in the middle of a .s file which won't work.
Maybe there should be a consistency check against a command line argument
or the builtin defaults.  Or maybe we should verify that no actual instructions
have been seen yet.  If no code yet, then changing the arch is OK.

riscv_set_public_attributes has a comment typo, "for normalize" should be
"to normalize".

Running the binutils, gas, and ld testsuites with the patch applied for
riscv{32,64}-{elf,linux} targets, I get a binutils failure for the two linux
targets.  The testcase that fails is binutils/testsuite/binutils-all/strip-3.d.
You can fix this by adding a -R option for the new .riscv.attributes section.

Jim