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

Codify Save/Restore Frame Layout #70

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lenary
Copy link

@lenary lenary commented Jan 2, 2025

These layouts need to be compatible between what the compiler expects when emitting CFI information, and what the library actually implements, so we write a bit more detail about how these should work to ensure that compatibility.

Fixes #35

@lenary
Copy link
Author

lenary commented Jan 2, 2025

@kito-cheng I know you did the GCC implementation, hopefully you feel this wording is good enough to allow some flexibility in implementation, while still ensuring compilers can emit the right CFI information?

address register `ra` is always included in the registers saved and restored.
`<N>` is a value between 0 and 12 and corresponds to the number of registers
between `s0` and `s11` that are saved/restored. When using the `ILP32E` ABI,
`<N>` can be at most 2, as `s3` to `s12` are not CSRs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you expand CSRs? I know it's the term used in LLVM for callee saved regs, but it might be ambiguous in the RISC-V land since Control and Status Register also called CSR.

(And caller-saved reg can also abbreviated as CSR as well in theory...)

Copy link
Author

Choose a reason for hiding this comment

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

Done.


In all the save restore routines, across all ABIs, `ra` is stored adjacent to
the incoming stack pointer (highest address), then the CSRs in register order
from `s0` to `s11`. This follows the Frame Pointer Convention, whether or not a
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
from `s0` to `s11`. This follows the Frame Pointer Convention, whether or not a
from `s0` to `s11`. This follows the [Frame Pointer Convention](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#frame-pointer-convention), whether or not a

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines +224 to +227
While the implementation of the save restore routines are in the library, it is
the compiler's responsibility to emit the unwind information (CFI) for the
registers that are saved and restored by these routines, so the compilers and
the libraries must agree on the stack layouts used by these routines.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some random thing come to my mind:

GCC always use libgcc since it's not provide a command line option to let user use compiler-rt, but LLVM offer option to let user using either libgcc or compiler-rt?

However I guess it's may not the problem on the spec side, it's implementation side issue: either 1) align libgcc and compiler-rt or 2) LLVM will gen the CFI according the runtime library preference if libgcc and compiler-rt has different layout.

Copy link
Author

Choose a reason for hiding this comment

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

For most of the ABIs, we are already at 1 as far as I understand, and I think that is the way to go, on the basis that libgcc can either be a shared or a static link (I'm not sure if compiler-rt is the same).

I think the only place where anyone differs from the description is the ilp32e ABI, which is still unstable everywhere, so this change can be made to the libraries as long as it is noted (hence my PR to compiler-rt).

I would really prefer not to have to teach LLVM to emit two different CFI directive sets depending on the library chosen.

I recall in the investigations that ilp32e CFI information from GCC is not correct anyway, so needs re-checking? Maybe I'm wrong about that or it's been fixed since I noticed.

These layouts need to be compatible between what the compiler expects
when emitting CFI information, and what the library actually implements,
so we write a bit more detail about how these should work to ensure that
compatibility.

Fixes riscv-non-isa#35

Signed-off-by: Sam Elliott <[email protected]>
@lenary lenary force-pushed the pr/save-restore-layout branch from 35aff36 to 07462b9 Compare January 7, 2025 15:50
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.

Stack frame layout for __riscv_{save,restore}_N
2 participants