Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document ELF attribute section #71

Merged
merged 10 commits into from
Jun 30, 2021

Conversation

kito-cheng
Copy link
Collaborator

Hi all:

It's the draft of the ELF attribute.

Copy link
Contributor

@palmer-dabbelt palmer-dabbelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Kito! I have a few small comments...

riscv-elf.md Outdated
```

For example, the value for version 2.1.3 of the privileged specification will be
20103.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a problem to encode these as separate fields, so there isn't a problem with running out of space? I don't want to end up in a "100 is enough for anyone" situation in 20 years :).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you are right, maximal number of attribute is almost unlimited, we should separate that.

riscv-elf.md Outdated
`-march`. Different architectures will be integrated into a superset when object
files are merged.

#### Tag_priv_spec, 5, uleb128=version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the priv ISA version specified in the ELF but not the user ISA? The user ISA seems like the more important one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought version info of user ISA should separated in different Tag_*_ext entry? Does here any info not included in a standard extension or base integer instruction?

riscv-elf.md Outdated
Each attribute is described in the following structure:
```<Tag name>, <Value>, <Parameter type 1>=<Parameter name 1>[, <Parameter type 2>=<Parameter name 2>]```

#### Tag_arch, 4, NTBS=subarch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're including "-march" then we should include "-mabi" as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-mabi are all included in e_flags, so I guess we don't need repeat again in attribute?

riscv-elf.md Outdated
The arch string denotes the name of a non-standard extension with a prefix of
x. If there are multiple non-standard extensions in the string, each extension
is separated from another with an underline. For example, when there are two
non-standard extensions foo and bar, the string is encoded as `xfoo_xbar`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aswaterman IIRC there was some ambiguous wording in the ISA spec about this, did it ever get fixed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and this description is correct.

riscv-elf.md Outdated
- Version information
The value of version information is computed by the following formula:

```Major version * 10000 + Minor version * 100 + Revision version```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same versioning question, though it's probably less important for these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version of instruction extension should less change than priv version? It should be fine for there, and keep one entry for every instruction extension has one more advantage is the tag value can just use same ASCII code of the extension name :P

Copy link
Collaborator Author

@kito-cheng kito-cheng Mar 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I am not mean every extension should use only one entry, I expect there would be more than one entry for V extension to recording type and vector length, but the version info using same rule for V extension just like all other extension.

@asb
Copy link
Collaborator

asb commented Mar 27, 2018

For anyone who is only following the discussion here, note that there is additional discussion on the sw-dev mailing list:
https://groups.google.com/a/groups.riscv.org/d/msgid/sw-dev/CA%2ByXCZCr%3DGPJWYHcc-NMrfYpy4CfQNu8WCMjUwiRVHwLsGEnnQ%40mail.gmail.com

I think it would be worth considering just emitting a ISA string of the form specified in Chapter 22 of the v2.2 user spec. That section only anticipates specs having major and minor version numbers (i.e. no 'revision version').

@palmer-dabbelt
Copy link
Contributor

I agree with @asb here: there's already a standard for naming RISC-V extensions so let's just use that.

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Update the description of Tag_arch: Need to expand abbreviation and version explicitly.
  • Update the define of Tag_priv_spec: it's record major version of priv spec only now.
  • Add new attributes: Tag_priv_spec_minor and Tag_priv_spec_revision.
  • Remove Tag_*_ext entries.

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Rebase to master
  • Change Tag_strict_align to Tag_unaligned_access
    • It also getting opposite meaning, because value 0 also mean not present in implementation.

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Add new section: Section Types and Special Sections.
  • Update Tag number to fit even/odd rule.
  • Change prefix to Tag_RISCV_

@jim-wilson
Copy link
Collaborator

FYI ELF Attribute support has been added to FSF Binutils now, using Kito's patches, and a new release is expected by end of January.

@aswaterman
Copy link
Contributor

aswaterman commented Jan 18, 2019

@jim-wilson is the version in FSF Binutils consistent with this pull request?

@jim-wilson
Copy link
Collaborator

I haven't double checked that. I can do that tomorrow. I do see some changes related to my binutils patch review, so it is likely to be up to date.

@asb
Copy link
Collaborator

asb commented Jan 18, 2019

I think RISC-V needs a standard like this for specifying the target ISA using ELF attributes and I'm incredibly grateful for Kito's work on it. However, I'm unhappy at the idea that this be blessed as a standard based on a binutils decision to merge patches. That's not the way standards development should work in an ecosystem with so many interested parties.

I think thought needs to be given to attributes that have scope narrower than an object (as supported by ARM BuildAttributes). This is likely to be a common requirements if you consider e.g. a library with multiple versions of the same function compiled for different extensions. With appropriately scoped attributes, objdump can correctly disassemble these functions even if there was an overlap in encodings.

As commented in March, official specs don't seem to have a "revision" just major+minor (so maybe Tag_RISCV_priv_spec_revision should be removed). If it's meant to indicate draft/non-draft, some description would be useful. It's a weakness of the ISA naming string description that there's not a clear way to specify the version of the privileged spec and no names are defined to specify e.g. a system that supports U+M privilege levels. I think it should be fixed there, and we should use the ISA naming string throughout.

Copy link
Collaborator

@jim-wilson jim-wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks consistent with the binutils patch, and only needs some minor fixes, mostly trying to improve the English a little.

riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
@jim-wilson
Copy link
Collaborator

priv spec 1.9.1 is not compatible with priv spec 1.9, so we do need 3 numbers to precisely describe a priv spec version. Though hopefully this mistake won't be made again, and 1.9.1 will be the only priv spec version with 3 numbers.

@jim-wilson
Copy link
Collaborator

On the plus side here, the binutils patches have a lot of flexibility. You can specify at configure time whether attribute support is on by default or off by default. If you don't use the configure option, then it is on by default for embedded elf targets and off by default for linux which presumably doesn't need them. There is also an assembler option that you can use to force attribute support on or off at assembler run time.

The even/odd numbering scheme for tags gives us some forward/backward compatibility, and we have the option of adding new attributes and deprecating old ones.

On the flip side, Kito opened this pull request almost 10 months ago, and attributes were discussed in email messages even before that. There was no comment on the proposal until I accepted a binutils patch. As a gnu toolchain developer, when I have a problem that needs to be solved, I can't wait years for a committee to make a decision before I fix a problem. I'm happy to work with the committee and adjust the support as necessary, but if there isn't going to be any committee decision at all then I will make my own decisions.

Another consideration here is that the RISC-V Foundation doesn't have any programming staff. It is nice to ask for interesting features to be added to the standard, but as a practical matter, we can only add stuff to the tools that a volunteer is willing to implement.

@asb
Copy link
Collaborator

asb commented Jan 18, 2019

I'm not suggesting waiting years for a committee, just ensuring that there's at least some degree of support from stakeholders in the community. Without being subscribed to the binutils list, it was not at all clear that the latest version of this proposal was being considered as "ready to go".

Thanks for pointing out the privspec 1.9.1 issue, that is indeed unfortunate.

@kito-cheng
Copy link
Collaborator Author

I think thought needs to be given to attributes that have scope narrower than an object (as supported by > ARM BuildAttributes). This is likely to be a common requirements if you consider e.g. a library with
multiple versions of the same function compiled for different extensions. With appropriately scoped
attributes, objdump can correctly disassemble these functions even if there was an overlap in encodings.

ARM was deprecated that since ABI revision r2.09 (at November 2012)[1], and binutils seems not well supported now, so I think we need more discussion and investigation about that .

[1] ARM ABI. Page 29 of 45, http://infocenter.arm.com/help/topic/com.arm.doc.ihi0045e/IHI0045E_ABI_addenda.pdf

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Revise the words.
  • Add even/odd rule.
  • Add reserved tag value range for non-standard attribute.

@kito-cheng
Copy link
Collaborator Author

Current status:

  • Implemented on GCC 9 and binutils 2.32.
  • Merge policy between different arch version didn't specified on spec, my thought is leave it implementation-defined, binutils only allow exact version match, but we should implement different merge policy.
  • Lacking of section or symbol scoped attributes which is nice to have for ifunc, so that we can dis-assemble binary correctly.
    • Upstream binutils DIDN'T implement section or symbol scoped attributes even for ARM port, so here is lots work need to do.

@kito-cheng
Copy link
Collaborator Author

LLVM support has been merged into trunk:

@kito-cheng
Copy link
Collaborator Author

Rebased.

riscv-elf.md Outdated Show resolved Hide resolved
riscv-elf.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@jim-wilson jim-wilson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the stack align attribute error (or warning) on mismatch

riscv-elf.md Outdated Show resolved Hide resolved
@kito-cheng
Copy link
Collaborator Author

Changes:

  • Address @jrtc27's comment
  • Change the merge policy for Tag_RISCV_stack_align.

@kito-cheng kito-cheng requested a review from jrtc27 May 19, 2021 10:20
@jrtc27
Copy link
Collaborator

jrtc27 commented May 31, 2021

Something that occurred to me the other day regarding this is that we need a program header too, not just a section header, since ELF loaders only look at program headers rather than the more complicated section headers (and require them to do otherwise will result in pushback). PT_LOPROC and PT_HIPROC are the same value as SHT_LOPROC and SHT_HIPROC, so I suggest we add a PT_RISCV_ATTRIBUTES with the same value as SHT_RISCV_ATTRIBUTES. This unfortunately means binaries produced today by GNU ld and LLD will not have information that loaders can usefully use, but that's true of binaries produced by toolchains before they added initial RISC-V attributes support, and so just have to be assumed compatible (which they should generally be, since IMAFDC are all that you can use without enabling experimental extensions in either toolchain).

@kito-cheng
Copy link
Collaborator Author

@jrtc27 Good point, add PT_RISCV_ATTRIBUTES would make kernel or dynamic linker easier to get the location of ELF attribute.

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Add PT_RISCV_ATTRIBUTES

@kito-cheng
Copy link
Collaborator Author

binutils patch for PT_RISCV_ATTRIBUTES.
https://sourceware.org/pipermail/binutils/2021-June/116990.html

@MaskRay
Copy link
Collaborator

MaskRay commented Jun 17, 2021

Is there a justification for PT_RISCV_ATTRIBUTES? There are reasons why other SHT_*_ATTRIBUTES arches don't introduce PT_*_ATTRIBUTES.

I think it may be because these SHT_*_ATTRIBUTES are only for static link time checks which don't really need runtime detection checks. At runtime GNU_PROPERTY is prevailing, though I don't necessarily with the existing GNU_PROPERTY usage.

@kito-cheng
Copy link
Collaborator Author

Is there a justification for PT_RISCV_ATTRIBUTES? There are reasons why other SHT_ATTRIBUTES arches don't introduce PT_ATTRIBUTES.

I think it may be because these SHT_*_ATTRIBUTES are only for static link time checks which don't really need runtime detection checks. At runtime GNU_PROPERTY is prevailing, though I don't necessarily with the existing GNU_PROPERTY usage.

Yes, currently is used that for static link time only, and we are try to bring it into loader and dynamic linker, for checking the minimum ISAs, it's same propose as GNU_PROPERTY, but I think we already have build attribute for a while, so leverage that is kind of straightforward.

[1] https://sourceware.org/legacy-ml/gnu-gabi/2016-q4/msg00000.html

@kito-cheng
Copy link
Collaborator Author

@MaskRay ping.

@MaskRay
Copy link
Collaborator

MaskRay commented Jun 28, 2021

I hope every program header type can find a use case. If you find PT_RISCV_ATTRIBUTES, it's fine.
The caveat is that the loader may need to decode a lot of values.

The binutils patch stills needs a fix as it searches for the attribute section by name instead of by type.

@kito-cheng
Copy link
Collaborator Author

Updated patch for PT_RISCV_ATTRIBUTES, accepted by RISC-V binutils maintainer, and will merge after binutils 2.37 branch created.
https://sourceware.org/pipermail/binutils/2021-June/117182.html

@kito-cheng
Copy link
Collaborator Author

Gonna merge this PR now, welcome send PR or create issue if any further comment!

@kito-cheng kito-cheng merged commit 9001839 into riscv-non-isa:master Jun 30, 2021
@kito-cheng kito-cheng deleted the attr-sections branch June 30, 2021 13:09
saagarjha pushed a commit to ahjragaas/binutils-gdb that referenced this pull request Jul 6, 2021
We added PT_RISCV_ATTRIBUTES to program header to make
.riscv.attribute easier to find in dynamic loader or kernel.

Ref:
riscv-non-isa/riscv-elf-psabi-doc#71

ChangeLog:

bfd/

	* elfnn-riscv.c(RISCV_ATTRIBUTES_SECTION_NAME): New.
	(riscv_elf_additional_program_headers): Ditto.
	(riscv_elf_modify_segment_map): Ditto.
	(elf_backend_additional_program_headers): Ditto.
	(elf_backend_modify_segment_map): Ditto.
	(elf_backend_obj_attrs_section): Use RISCV_ATTRIBUTES_SECTION_NAME
	rather than string literal.

binutils/

	* readelf.c(get_riscv_segment_type): New.
	(get_segment_type): Handle EM_RISCV.

include/

	* elf/riscv.h (PT_RISCV_ATTRIBUTES): New.
	* testsuite/ld-elf/orphan-region.ld: Discard .riscv.attributes
	section for simplify testcase.
	* testsuite/ld-riscv-elf/attr-phdr.d: New.
	* testsuite/ld-riscv-elf/attr-phdr.s: Ditto.
	* testsuite/ld-riscv-elf/ld-riscv-elf.exp: Add attr-phdr to
	testcase.
MaskRay added a commit to llvm/llvm-project that referenced this pull request Jun 6, 2023
Close #63084

Unlike AArch32, RISC-V defines PT_RISCV_ATTRIBUTES to include the
SHT_RISCV_ATTRIBUTES section. There is no real-world use case yet.

We place PT_RISCV_ATTRIBUTES after PT_GNU_STACK, similar to PT_ARM_EXIDX. GNU ld
places PT_RISCV_ATTRIBUTES earlier, but the placement should not matter.

Link: riscv-non-isa/riscv-elf-psabi-doc#71

Reviewed By: asb

Differential Revision: https://reviews.llvm.org/D152065
@MaskRay
Copy link
Collaborator

MaskRay commented Jun 7, 2023

Actually, I find PT_RISCV_ATTRIBUTES strange. The SHT_RISCV_ATTRIBUTES section is not contained by a PT_LOAD program header, so the SHT_RISCV_ATTRIBUTES section may not be mapped, and dynamic loaders cannot use PT_RISCV_ATTRIBUTES.

I think SN systems seems to use program headers that include a non-allocable section, but I don't know an open-source precedent.

MaskRay added a commit to MaskRay/riscv-elf-psabi-doc that referenced this pull request Jun 7, 2023
riscv-non-isa#71 added a PT_RISCV_ATTRIBUTES segment to contain the SHT_RISCV_ATTRIBUTES section.
The SHT_RISCV_ATTRIBUTES section does not have the SHF_ALLOC flag and is therefore not in a PT_LOAD segment.
Dynamic loaders cannot assume the PT_RISCV_ATTRIBUTES part is present.

I think the existence of PT_RISCV_ATTRIBUTES just complicates binary
manipulation tools like strip.
MaskRay added a commit to MaskRay/riscv-elf-psabi-doc that referenced this pull request Jun 7, 2023
riscv-non-isa#71 added a PT_RISCV_ATTRIBUTES segment to contain the SHT_RISCV_ATTRIBUTES section. The
SHT_RISCV_ATTRIBUTES section does not have the SHF_ALLOC flag and is therefore not in a PT_LOAD
segment. Dynamic loaders cannot assume the PT_RISCV_ATTRIBUTES part is present.

I think the existence of PT_RISCV_ATTRIBUTES just complicates analysis tools that falsely assume
p_filesz<=p_memsz and binary manipulation tools like strip. E.g. if the SHT_RISCV_ATTRIBUTES section
is removed, what should we do with PT_RISCV_ATTRIBUTES?

I think we should remove PT_RISCV_ATTRIBUTES, but ensure that the program header type is not reused
by a future program header type.
MaskRay added a commit to MaskRay/riscv-elf-psabi-doc that referenced this pull request Jun 7, 2023
riscv-non-isa#71 added a PT_RISCV_ATTRIBUTES segment to contain the SHT_RISCV_ATTRIBUTES section. The
SHT_RISCV_ATTRIBUTES section does not have the SHF_ALLOC flag and is therefore not in a PT_LOAD
segment. Dynamic loaders cannot assume the PT_RISCV_ATTRIBUTES part is present (it may happen
to be mapped due to page alignment, but there is no guarantee).

I think the existence of PT_RISCV_ATTRIBUTES just complicates analysis tools and binary manipulation
tools like strip. E.g. if the SHT_RISCV_ATTRIBUTES section is removed, what should we do with
PT_RISCV_ATTRIBUTES?

I think we should remove PT_RISCV_ATTRIBUTES, but ensure that the program header type is not reused
by a future program header type.
MaskRay added a commit to MaskRay/riscv-elf-psabi-doc that referenced this pull request Jun 7, 2023
riscv-non-isa#71 added a PT_RISCV_ATTRIBUTES segment to contain the SHT_RISCV_ATTRIBUTES section. The
SHT_RISCV_ATTRIBUTES section does not have the SHF_ALLOC flag and is therefore not in a PT_LOAD
segment. Dynamic loaders cannot assume the PT_RISCV_ATTRIBUTES part is present (it may happen
to be mapped due to page alignment, but there is no guarantee).

To the best of my knowledge, PT_RISCV_ATTRIBUTES is the only open-source usage of a segment that
contains a non-SHF_ALLOC section.

I think the existence of PT_RISCV_ATTRIBUTES just complicates analysis tools and binary manipulation
tools like strip. E.g. if the SHT_RISCV_ATTRIBUTES section is removed, what should we do with
PT_RISCV_ATTRIBUTES?

I think we should remove PT_RISCV_ATTRIBUTES, but ensure that the program header type is not reused
by a future program header type.
MaskRay added a commit to MaskRay/riscv-elf-psabi-doc that referenced this pull request Apr 12, 2024
riscv-non-isa#71 added a PT_RISCV_ATTRIBUTES segment to contain the SHT_RISCV_ATTRIBUTES section. The
SHT_RISCV_ATTRIBUTES section does not have the SHF_ALLOC flag and is therefore not in a PT_LOAD
segment. Dynamic loaders cannot assume the PT_RISCV_ATTRIBUTES part is present (it may happen
to be mapped due to page alignment, but there is no guarantee).

To the best of my knowledge, PT_RISCV_ATTRIBUTES is the only open-source usage of a segment that
contains a non-SHF_ALLOC section.

I think the existence of PT_RISCV_ATTRIBUTES just complicates analysis tools and binary manipulation
tools like strip. E.g. if the SHT_RISCV_ATTRIBUTES section is removed, what should we do with
PT_RISCV_ATTRIBUTES?

I think we should remove PT_RISCV_ATTRIBUTES, but ensure that the program header type is not reused
by a future program header type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants