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

Clarify Tag_RISCV_arch is the minimum execution environment requireme… #292

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kito-cheng
Copy link
Collaborator

…nt of objects

And also add NOTE to mention we could use more than Tag_RISCV_arch
specify, but must make sure that should work on the execution
environment with what Tag_RISCV_arch specify only.

@kito-cheng kito-cheng requested a review from jrtc27 June 14, 2022 17:12
riscv-elf.adoc Outdated
when object files are merged.
Tag_RISCV_arch contains a string for the target architecture, this information
used as the minimum execution environment requirement. Different architectures
will be integrated into a superset when object files are merged.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's now how current binaries work, and it's a pretty strong meaning associated with that tag. IMO it's best to add a new one, as trying to re-purpose something with a bunch of old binaries lying around is going to lead to confusion.

Copy link
Collaborator Author

@kito-cheng kito-cheng Jun 23, 2022

Choose a reason for hiding this comment

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

Okay, I think that's my bad, isn't define that clearly that in spec before even I preaching this concept for several years :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see how it poses a problem? If you add a new tag, you still have old binaries lying around that have Tag_RISCV_arch specifying some arch string without some Tag_RISCV_min_req_arch specifying the minimum, so you'd have to fall back on treating Tag_RISCV_arch as the requirement for those, at which point binaries without Tag_RISCV_min_req_arch behave identically to how Tag_RISCV_min_req_arch is specified. Redefining Tag_RISCV_arch to be the minimum required architecture to run the binary (as always intended, just not adequately captured due to a lack of IFUNC etc specification) wouldn't change behaviour at all, no? What am I missing?

Copy link
Collaborator Author

@kito-cheng kito-cheng Jul 22, 2022

Choose a reason for hiding this comment

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

Has some off list discussion with Palmer, his concern is maybe here is already some binary has set that beyond the minimal requirement, but that currently work fine for now since no simulator or execution environment, however that could be problem those kind of old binary run on newer execution environment which will check Tag_RISCV_arch as minimum execution environment.

I think currently there are 3 options:

  1. Update the definition as this PR.
  2. Add a new tag called Tag_RISCV_min_req_arch for minimum execution environment requirement, but that will identical to Tag_RISCV_arch in most case.
  3. Add a new tag to identify Tag_RISCV_arch is presenting minimum execution environment requirement, e.g. Tag_RISCV_arch_is_min_req or a more generic attribute to recording the psABI version to disctionsu that

However option 2 seems just waste bunch of space to saving the duplicated string, so only option 1 or option 3 should be consider I think.

@asb
Copy link
Collaborator

asb commented Aug 1, 2022

Regardless of whether a specific solution can be agreed ahead of 1.0 or not, I think it would be helpful for the psABI to comment on the case of one ELF file with functions that target different architecture baselines. If it's something that isn't handled well right now but is planned for future work, it would be helpful to say so.

@asb
Copy link
Collaborator

asb commented Aug 1, 2022

Further reflecting on this - it's not immediately obvious to me if/how this PR alters the status quo (vs my understanding of the current text at least).

The current text talks about taking Tag_RISCV_arch from -march. This effectively means that Tag_RISCV_arch will reflect the set of ISA extensions that a compiler may have selected without runtime feature checks. This provides enough to e.g. produce a meaningful diagnostic from an ELF loader if trying to load an rv64gc_zba_zbb_zbc_zbs binary on a system with B support.

I think part of the confusion / challenge with these ELF attributes is that we've looked to use them to solve multiple problems, but they're not well suited to solving all of them as defined and the problems aren't the same priority:

  • Basic link-time diagnostics when mixing and matching ELF files targeting different ISA baselines. Can be particularly useful on embedded targets IMHO.
  • Load-time diagnostics on larger systems. e.g. suppose a new 'G' baseline adds bitmanip extensions in the future, a sensible diagnostic can be emitted on systems trying to load these binaries that don't support those extensions.
  • Helping to guide a disassembler in how to decode instructions (i.e. indicating which instructions may be found in the object file). Given the attribute doesn't describe all extensions that could possibly be found in an ELF (except in the case those ISA variants were enabled via -march, when they could be generated in other ways e.g. function-specific attributes), it won't work for that purpose in all cases. It also couldn't work in the case of extensions that overlap in encoding space but which are included in the same file.

@kito-cheng
Copy link
Collaborator Author

Changes:

  • Rebase
  • Adding more non-normative to describe the usage and recommend execution environment could use this info to checking executable

@kito-cheng
Copy link
Collaborator Author

kito-cheng commented Aug 10, 2022

After few rounds of off-list chat I realized I put the design story in different places like .option arch[1] and mapping symbol [2] and obviously not easy to understand if only read the description of Tag_RISCV_arch, so I've added a bunch of words in this update to try and tell the full story here.

@asb do you mind take a look to see it's much clear to you now? I intentionally NOT document about disassembler part since mapping symbol ins't included in this release yet.

[1] riscv-non-isa/riscv-asm-manual#67
[2] #196

@asb
Copy link
Collaborator

asb commented Aug 16, 2022

What do you think about adding a sentence to clarify what is meant by "minimum execution environment". e.g. "this information
used as the minimum execution environment requirement." => "this information reflects the minimum execution environment requirement. i.e. the set of instruction set extensions that the compiler may have used in code generation without any runtime feature detection or gating."

…nt of objects

And also add NOTE to mention we could use more than Tag_RISCV_arch
specify, but must make sure that should work on the execution
environment with what Tag_RISCV_arch specify only.
@kito-cheng
Copy link
Collaborator Author

@asb , thanks for the suggestion, applied!

@palmer-dabbelt
Copy link
Contributor

Existing binaries don't behave as described in this: the Tag_RISCV_arch is just the total set of extensions (along with some base ISA twiddling) that the object files were built with, it doesn't take into account any detection mechanisms those binaries may have. Unless I'm missing something there's no way to differentiate those old binaries from the new ones, so we're not really going to be able to do much with the tag's information.

@kito-cheng
Copy link
Collaborator Author

Defer this change, and adding another tag to mark this tag is minimum execution environment requirement.

@asb
Copy link
Collaborator

asb commented Sep 1, 2022

I think "this information reflects the minimum execution environment requirement. i.e. the set of instruction set extensions that the compiler may have used in code generation without any runtime feature detection or gating." is a logical consequence of the references to this tag being set based on -march and the current semantics of -march for bother clang and GCC. But I agree this is all a bit fuzzy and underspecified, so adding a new tag is probably the right approach.

@vineetgarc
Copy link

The "minimal" set has its merits, but I'd vote to keep the exact user specified -march as well (and a superset when combining object files with disparate such entries in an executable/dso). This could be used to make runtime decisions. See [1]

[1] https://sourceware.org/pipermail/libc-alpha/2023-January/144578.html

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 10, 2023

-march is the minimal set, without further application-specific knowledge about the conditions under which functions get called (there's no way for a compiler or linker to know that functions compiled in an -march=rv32if file are only ever called when F's existence has already been dynamically checked). For anything else you should be using __attribute__((__target__(...))) or .option arch to selectively and locally change the ISA in use, which does not end up in the ELF attributes (as for those you're expected to have checked you can use the extensions enabled).

@vineetgarc
Copy link

Indeed I'm only referring to build time info available to compiler/linker and nothing about execution env itself.
And I would expect RV_ATTR_TAG_arch to capture that as is.

Kito's msg in above thread seemed to indicate that glibc with V based ifuncs won't have 'V' in attributes to convey a "minimal" execution env which is a problem for what I'm envisioning trying to the attr for.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jan 10, 2023

V not showing up for V IFUNCs is a feature/deliberate design decision. In fact, this lets you have conflicting extensions in use by different IFUNCs, which is a feature that needs to be supported (for example, it lets you have F and Zfinx versions of floating-point library routines).

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.

None yet

5 participants