[PATCH 2/2] RISC-V: Merge ELF attribute for ld

[Kito Cheng]

This patch implement the RISC-V ELF attribute merge, only implement
exact arch version match now, we'll implement different merge policy
later.

ChangeLog:
bfd/
* elfnn-riscv.c (in_subsets): New variable.
(out_subsets): Likewise.
(merged_subsets): Likewise.
(riscv_std_ext_p): Likewise.
(riscv_non_std_ext_p): Likewise.
(riscv_std_sv_ext_p): Likewise.
(riscv_non_std_sv_ext_p): Likewise.
(riscv_version_mismatch): Likewise.
(riscv_i_or_e_p): Likewise.
(riscv_merge_std_ext): Likewise.
(riscv_merge_non_std_and_sv_ext): Likewise.
(riscv_merge_arch_attr_info): Likewise.
(riscv_merge_attributes): Likewise.
(riscv_elf_modify_segment_map): Likewise.
(riscv_elf_additional_program_headers): Likewise.
(_bfd_riscv_elf_merge_private_bfd_data): Merge attribute.
(elf_backend_additional_program_headers): Define as
riscv_elf_additional_program_headers.
(elf_backend_modify_segment_map): Define as
riscv_elf_modify_segment_map.

binutils/
* readelf.c (get_riscv_segment_type) New function.
(get_segment_type): Add handler for RISC-V.

include/
* elf/riscv.h (PT_RISCV_ATTRIBUTES): Define.

ld/
* testsuite/ld-elf/orphan-region.d: XFAIL for RISC-V, because add new
section.
* testsuite/ld-scripts/size.exp: Unsupported size-2 for RISC-V,
because RISC-V has new segment in PHDR.
* testsuite/ld-riscv-elf/ld-riscv-elf.exp: Add new tests.
* testsuite/ld-riscv-elf/attr-merge-arch-01.d: New test.
* testsuite/ld-riscv-elf/attr-merge-arch-01a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-01b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-02.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-02a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-02b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-03.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-03a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-03b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-failed-01.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-failed-01a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-arch-failed-01b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-priv-spec-a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-priv-spec-b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-priv-spec.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-stack-align-a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-stack-align-b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-stack-align-failed-a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-stack-align-failed-b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-stack-align-failed.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-stack-align.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-01.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-01a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-01b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-02.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-02a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-02b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-03.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-03a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-03b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-04.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-04a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-04b.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-05.d: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-05a.s: Likewise.
* testsuite/ld-riscv-elf/attr-merge-strict-align-05b.s: Likewise.

[Jim Wilson]

On Fri, Dec 7, 2018 at 4:40 AM Kito Cheng <kito....@gmail.com> wrote:
> This patch implement the RISC-V ELF attribute merge, only implement
> exact arch version match now, we'll implement different merge policy
> later.

riscv_version_mismatch, the function parameters aren't declared in the right
coding style, and likewise for a lot of following functions, parameters should
start on the same line as the open parenthesis, and be indented to match if
carry over to multiple lines

riscv_i_or_e_p uses strcmp, the previous patch uses strcasecmp, if we aren't
canonicalizing names to lowercase somewhere, this should probably use
strcasecmp too

I think a ? in the middle of an error message looks funny, we know that there
is a problem here, we should state the error, not ask a question, I'd change
it to a comma

a complex function like riscv_merge_std_ext should have a comment explaining
what it does, why pin and pout are pointers for instance, it looks like it
parses what it can parse and then stores the unparsed stuff back into pin and
pout for the next function, that should be documented

the first error message has a typo, "tring" -> "string"

should presumably return false after calling _bfd_error_handler

for the base architecture, it is putting IN name, major_version, and
minor_version into merged_subsets, but seems to be ignoring OUT name,
major_version, and minor_version, the code only verifies that the first letter
between IN and OUT are the same, it isn't comparing the version numbers

a few more code style issues, when calling a function, arguments go after the
open parenthesis, not on the next line

in the comnment at the end, "Skip all standard extension." should be
"Skip all standard extensions."

in riscv_merge_arch_attr_info, "is unmatched with" sounds awkward, I'd suggest
"doesn't match"

at the end, why are you passing ARCH_SIZE into riscv_arch_str? Shouldn't that
be the xlen_in or xlen_out value that we already parsed from the attributes?,
the code doesn't require that xlen_in or xlen_out match ARCH_SIZE, maybe it
should?

riscv_merge_attributes has a comment mentioning IBFD and OBFD, but there is
no OBFD parameter, usually capital letters here indicate a parameter name,
should mention the output bfd in INFO instead, I see that the elf32-arm.c
file has the same bug, maybe a previous version took an OBFD argument and
they forgot to fix the comment, we should get our version right

"that hasn't attribute section" is awkward, I'd suggest "that doesn't have
an attribute section"

there is a comment that refers to "ARM" at the end, suggest RISC-V instead

PT_RISCV_ATTRIBUTES should be in the riscv-elf-psabi-doc pull request

Do we really need PT_RISCV_ATTRIBUTES? The ARM port doesn't have one. It
doesn't appear that any other target has one either. This seems unnecessary.

In the bfd ChangeLog entry, I'd suggest "New." instead of "New variable."
because only the first three are variables, the rest are functions.

I get 10 extra ld testsuite failures for riscv64-{elf,linux} targets. It
looks like the problem is the one I referred to earlier, where ARCH_SIZE is
used instead of the xlen mentioned in the attributes.

A more general question, do we want the attribute support enabled for linux
targets? It may be more of a distraction than a help there, as we don't
expect that linux systems will support more than one type of processor. At
the moment the attribute support is enabled for linux targets.

Jim

[Kito Cheng]

Hi Jim:

> at the end, why are you passing ARCH_SIZE into riscv_arch_str? Shouldn't that
> be the xlen_in or xlen_out value that we already parsed from the attributes?,
> the code doesn't require that xlen_in or xlen_out match ARCH_SIZE, maybe it
> should?

Yeah, I should add check xlen_in == xlen_out == ARCH_SIZE.

> PT_RISCV_ATTRIBUTES should be in the riscv-elf-psabi-doc pull request
>
> Do we really need PT_RISCV_ATTRIBUTES? The ARM port doesn't have one. It
> doesn't appear that any other target has one either. This seems unnecessary.

PT_RISCV_ATTRIBUTES was used in linux kernel for easier find attribute
section, it can just parse program header instead of section headers.

That's a mechanism to check ELF executable is safe to execute, however it was
not accepted by most other guys at Linux Plumbers Conference 2018[1],
so I'll remove that until community have consensus on that.

[1] Note for RISC-V micro conf: https://etherpad.openstack.org/p/RISC-V

>
> In the bfd ChangeLog entry, I'd suggest "New." instead of "New variable."
> because only the first three are variables, the rest are functions.
>
> I get 10 extra ld testsuite failures for riscv64-{elf,linux} targets. It
> looks like the problem is the one I referred to earlier, where ARCH_SIZE is
> used instead of the xlen mentioned in the attributes.
>
> A more general question, do we want the attribute support enabled for linux
> targets? It may be more of a distraction than a help there, as we don't
> expect that linux systems will support more than one type of processor. At
> the moment the attribute support is enabled for linux targets.

Yeah, it sound a good suggestion, ELF attribute also don't consider ifunc yet,
so here is my plan:
1. Add a configure option --with-default-riscv-attribute=[yes|no|default]
to enable/disable default attribute (e.g. Tag_RISCV_arch).
2. Default enable for riscv*-*-elf target
3. Default disable for riscv*-*-linux* target

[Maciej W. Rozycki]

On Fri, 21 Dec 2018, Kito Cheng wrote:

> > PT_RISCV_ATTRIBUTES should be in the riscv-elf-psabi-doc pull request
> >
> > Do we really need PT_RISCV_ATTRIBUTES? The ARM port doesn't have one. It
> > doesn't appear that any other target has one either. This seems unnecessary.
>
> PT_RISCV_ATTRIBUTES was used in linux kernel for easier find attribute
> section, it can just parse program header instead of section headers.
>
> That's a mechanism to check ELF executable is safe to execute, however it was
> not accepted by most other guys at Linux Plumbers Conference 2018[1],
> so I'll remove that until community have consensus on that.

Hmm, an ELF exexutable or DSO is not required to have section headers, as
these structures are not meant to be used for program loading, so what was
the alternative proposed by the opposing party?

Maciej