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

Dynamic stack frame size support #26

Merged

Conversation

alessandrod
Copy link
Collaborator

@alessandrod alessandrod commented Feb 25, 2022

LLVM side of solana-labs/rbpf#274. Based on initial work from @dmakarov.

This PR does two things:

  • It uses r11 as the stack pointer in function prologues/epilogues.
  • It sets the e_flags field on ELF headers to EF_SBF_V2 (0x20).

rbpf looks at e_flags and if it's EF_SBF_V2 it turns on dynamic frames (if enabled in the vm).

I opted to use e_flags instead of OSABI/ABIVERSION since it's what all other tagets do (except AMDGPU). The idea is that if we need to bump ABI again, we'd use EF_SBF_V2_1 = 0x21 etc.

@alessandrod
Copy link
Collaborator Author

I just realized that this needs to be made opt-in for the foreseeable future, otherwise we can't do a new toolchain release until the rbpf change reaches all the way to MB. Might be time to finally have cargo build-sbf.

@dmakarov
Copy link
Collaborator

dmakarov commented Mar 1, 2022

I just realized that this needs to be made opt-in for the foreseeable future, otherwise we can't do a new toolchain release until the rbpf change reaches all the way to MB. Might be time to finally have cargo build-sbf.

I'm going to release bpf-tools with rust 1.59.0 soon. That probably still can be with the old fixed stack. Later these changes can be applied on top of solana-1.59.0 branch. Yes, we can rename cargo-build-bpf to cargo-build-sbf at that point. @jackcmay What do you think? I'm not sure why we need to wait for rbpf changes to be on MB and not on master branch of solana? Once solana-labs/solana starts using the dynamic stack rbpf, we have to release the updated compiler too.

@alessandrod
Copy link
Collaborator Author

I just realized that this needs to be made opt-in for the foreseeable future, otherwise we can't do a new toolchain release until the rbpf change reaches all the way to MB. Might be time to finally have cargo build-sbf.

I'm going to release bpf-tools with rust 1.59.0 soon. That probably still can be with the old fixed stack.

Yay 🎉

Later these changes can be applied on top of solana-1.59.0 branch. Yes, we can rename cargo-build-bpf to cargo-build-sbf at that point.

I think we'll have to keep both for the foreseeable future. There's plenty of documentation out there that refers to cargo build-bpf and surely lots of scripts that expect cargo build-bpf to exist once the solana toolchain is installed.

I'm not sure why we need to wait for rbpf changes to be on MB and not on master branch of solana? Once solana-labs/solana starts using the dynamic stack rbpf, we have to release the updated compiler too.

Yes but dynamic frames will be feature gated and off to start with in devnet. Then eventually they'll be turned on, make it to testnet and MB. In the meantime, people need to be able to compile and deploy their programs, and we probably want them to be able to do that with the latest toolchain, without having to tell them "if you want to deploy to MB, you need to use toolchain version X".

@dmakarov
Copy link
Collaborator

dmakarov commented Mar 1, 2022

Can the prologue/epilogue etc changes be guarded by a command line option (like a feature maybe?) that is disabled by default. Later we can enable it by default or remove the guards.

@alessandrod
Copy link
Collaborator Author

Can the prologue/epilogue etc changes be guarded by a command line option (like a feature maybe?) that is disabled by default. Later we can enable it by default or remove the guards.

Yep that's what I meant with opt in, putting it behind a feature now

@jackcmay
Copy link
Collaborator

jackcmay commented Mar 2, 2022

Yeah, gotta featurize these changes and continue to support the existing BPF architecture.

Are there other breaking changes we wanted to get into SBF that we might want to put in at the same time, or do you guys think its better to increment the SBF version each time we introduce one?

@alessandrod
Copy link
Collaborator Author

alessandrod commented Mar 2, 2022

Are there other breaking changes we wanted to get into SBF that we might want to put in at the same time, or do you guys think its better to increment the SBF version each time we introduce one?

We've already removed support fot ldabs* and ldind* in LLVM and that change made it to the last bpf-tools release I think. The corresponding rbpf change is feature gated. Since our API crates never supported/never led to the emission of ldabs* and ldind* anyway, practically speaking I don't think that was a breaking change in the compiler.

Dynamic stacks are the only big breaking change I think. When we do the call change to fail on unresolved symbols, that will only impact programs that are currently failing at runtime, so technically I don't think that qualifies as a breaking change either - we'll just catch the error earlier.

@Lichtso is there anything else you think should go in with this?

@jackcmay
Copy link
Collaborator

jackcmay commented Mar 2, 2022

We also had adding support for signed division on the list for SBFv2, do we want to roll that into this change from BPF to SBF as well?
solana-labs/solana#20323

@alessandrod
Copy link
Collaborator Author

We also had adding support for signed division on the list for SBFv2, do we want to roll that into this change from BPF to SBF as well? solana-labs/solana#20323

It looks like signed division was already enabled a while ago via intrinsics. I'm assuming you mean undo that, and add a a new instruction so we can jit it?

@alessandrod
Copy link
Collaborator Author

alessandrod commented Mar 3, 2022

Ok I've pushed an extra commit that makes this opt-in (I'll squash before merging).

The way to opt-in is:

RUSTFLAGS="-C target-cpu=sbfv2" cargo build -Zbuild-std=std,panic_abort --target=sbf-solana-solana

-Zbuild-std is needed since now the compiled std we'll be distributing will still use old style fixed frames, so std needs to be recompiled along with the root crate and its deps to enable dynamic frames.

Next, I'm going to make cargo build-bpf --arch=sbfv2 invoke cargo this way.

alessandrod added a commit to alessandrod/solana that referenced this pull request Mar 3, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
@dmakarov
Copy link
Collaborator

dmakarov commented Mar 3, 2022

solana-labs/solana#20323

@@ -30,7 +30,9 @@ class BPF final : public TargetInfo {
const uint8_t *loc) const override;
RelType getDynRel(RelType type) const override;
int64_t getImplicitAddend(const uint8_t *buf, RelType type) const override;
void relocate(uint8_t *loc, const Relocation &rel, uint64_t val) const override;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there is a bunch of pure formatting changes it's nice to pull those into a separate pr

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

dbgs() << " ";
DL.print(dbgs());
}
dbgs() << " Function " << MF.getFunction().getName() << " Stack offset of " << -Offset
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we want to keep displaying this message for fixed solana stack frames?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup nice catch, fixed

MachineBasicBlock::iterator &MBBI,
unsigned int Opcode) {
MachineFrameInfo &MFI = MF.getFrameInfo();
int NumBytes = (int)MFI.getStackSize();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the total stack size known to llvm or does it allow overflow expecting the runtime to catch it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can't be known statically and the runtime needs to handle overflows anyway for hand rolled code

Copy link
Collaborator

@jackcmay jackcmay left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of nits/questions

@alessandrod alessandrod merged commit 7b107cd into anza-xyz:solana-rustc/13.0-2021-08-08 Mar 8, 2022
@alessandrod alessandrod deleted the dynamic-frames branch March 8, 2022 00:22
alessandrod added a commit to alessandrod/solana that referenced this pull request Jun 4, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
dmakarov pushed a commit to alessandrod/solana that referenced this pull request Jul 11, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
dmakarov pushed a commit to alessandrod/solana that referenced this pull request Jul 11, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
dmakarov pushed a commit to alessandrod/solana that referenced this pull request Jul 11, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
dmakarov pushed a commit to alessandrod/solana that referenced this pull request Jul 12, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
dmakarov pushed a commit to alessandrod/solana that referenced this pull request Jul 13, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
dmakarov pushed a commit to alessandrod/solana that referenced this pull request Jul 13, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
dmakarov pushed a commit to solana-labs/solana that referenced this pull request Jul 13, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
Lcchy pushed a commit to Bonfida/solana that referenced this pull request Jul 22, 2022
--arch allows selecting the target SBF version. See
anza-xyz/llvm-project#26.
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.

3 participants