Page MenuHomePhabricator

[ELF] Merge SHT_RISCV_ATTRIBUTES sections
ClosedPublic

Authored by MaskRay on Nov 23 2022, 1:06 AM.

Details

Summary

Currently we take the first SHT_RISCV_ATTRIBUTES (.riscv.attributes) as the
output. If we link an object without an extension with an object with the
extension, the output Tag_RISCV_arch may not contain the extension and some
tools like objdump -d will not decode the related instructions.

This patch implements
Tag_RISCV_stack_align/Tag_RISCV_arch/Tag_RISCV_unaligned_access merge as
specified by
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#attributes

For the deprecated Tag_RISCV_priv_spec{,_minor,_revision}, dump the attribute to
the output iff all input agree on the value. This is different from GNU ld but
our simple approach should be ok for deprecated tags.

RISCVAttributeParser::handler currently warns about unknown tags. This
behavior is retained. In GNU ld arm, tags >= 64 (mod 128) are ignored with a
warning. If RISC-V ever wants to do something similar
(https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/352), consider
documenting it in the psABI and changing RISCVAttributeParser.

Like GNU ld, zero value integer attributes and empty string attributes are not
dumped to the output.

Diff Detail

Event Timeline

MaskRay created this revision.Nov 23 2022, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2022, 1:06 AM
MaskRay requested review of this revision.Nov 23 2022, 1:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2022, 1:06 AM
MaskRay edited the summary of this revision. (Show Details)Nov 23 2022, 10:43 AM

Unknown tag handling can be improved. But as ld.bfd null pointer calls when merging unknown tags in .riscv.attributes (https://sourceware.org/bugzilla/show_bug.cgi?id=29823), and changing the behavior requires RISCVAttributeParser modification, I think polishing the behavior is not worth doing in this patch.

asb added a comment.Nov 30 2022, 3:30 AM

This seems a sensible approach to me. One quick question about the commenting approach - I think the patch summary provides a really nice clear description of the desired behaviour for merging attributes and where it comes from. Putting some of that information / justification into RISCV.cpp would I think be an improvement - at least pointing to the specification for this behaviour and noting that deprecated tags are included in the output only if all inputs have the same value.

MaskRay updated this revision to Diff 479701.Dec 2 2022, 11:45 AM
MaskRay edited the summary of this revision. (Show Details)

add comment

kito-cheng accepted this revision.Dec 4 2022, 7:30 PM
kito-cheng added inline comments.
lld/ELF/Arch/RISCV.cpp
865–872

IIUC that's handle different version like I 2.0 + I 2.1, but RISCVISAInfo::parseArchString don't have capability to parse/accept unsupported version yet, we might need to extend that to made it able to accept any extension version even not supported.

But that should be a separated patch I think, so I am OK with this for now, but maybe we should add TODO or FIXME in mergeArch to mention this.

This revision is now accepted and ready to land.Dec 4 2022, 7:30 PM
asb accepted this revision.Dec 5 2022, 7:12 AM

LGTM.

MaskRay updated this revision to Diff 481198.Dec 8 2022, 1:45 AM
MaskRay retitled this revision from [ELF] Combine .riscv.attributes to [ELF] Merge .riscv.attributes.

Rename combine to Merge: I think "merge" implies a size shrinking intention and is used by Arm/RISC-V ABIs.
Add more tests.

MaskRay updated this revision to Diff 481200.Dec 8 2022, 1:53 AM
MaskRay retitled this revision from [ELF] Merge .riscv.attributes to [ELF] Merge SHT_RISCV_ATTRIBUTES sections.
MaskRay edited the summary of this revision. (Show Details)

use canonical name

This revision was landed with ongoing or failed builds.Dec 8 2022, 1:53 AM
This revision was automatically updated to reflect the committed changes.