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

explicit 8-byte alignment #185

Merged
merged 17 commits into from
Mar 30, 2024
Merged

explicit 8-byte alignment #185

merged 17 commits into from
Mar 30, 2024

Conversation

cavemanloverboy
Copy link
Contributor

@cavemanloverboy cavemanloverboy commented Mar 27, 2024

all padding fields are now 8 byte aligned (on bpf, arm, x86), and all program types are now explciitly 8 byte aligned.

improves devex on mac. bpf in-memory (on-chain) representation is unchanged.

Copy link
Contributor

@exdx exdx left a comment

Choose a reason for hiding this comment

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

Does this change make it possible to finally compile marginfi programs on M-series macs? That would be a huge plus and remove the need for having a x86 VM to compile for me.

I'm a big fan of this change but would like @jkbpvsc to make the final approval. This code is very sensitive and if we change it in a way that's not backwards compatible it will be a pain.

@cavemanloverboy
Copy link
Contributor Author

yes it makes it compatible with mac. i am confident it doesn't affect on-chain representation, as it just makes the alignment and size of types equal between bpf and arm without changing bpf, but someone should definitely write a test that fetches all types from mainnet to be doubly sure.

jkbpvsc
jkbpvsc previously approved these changes Mar 29, 2024
@jkbpvsc
Copy link
Member

jkbpvsc commented Mar 29, 2024

@cavemanloverboy can you fix linting, it seems its failing

@cavemanloverboy
Copy link
Contributor Author

@cavemanloverboy can you fix linting, it seems its failing

this failure was on main haha. i'll fix

@cavemanloverboy
Copy link
Contributor Author

the linter failed because the bank and oracles are no longer in the vector holding the lut keys.

    // keys.extend(bank_pks);
    // keys.extend(oracle_pks);

Just wanna confirm this is intentional

@losman0s
Copy link
Member

I added a couple of fixture-based tests on Bank's and MarginfiAccount's WrappedI80F48 fields (most of them at least). I took the values with the current main, so that it reflects what is deployed rn.

You can rebase on main and run those to have that additional confirmation that layout's still ok.

exdx
exdx previously approved these changes Mar 29, 2024
#[macro_export]
macro_rules! assert_struct_align {
($struct: ty, $align: expr) => {
static_assertions::const_assert_eq!(std::mem::align_of::<$struct>(), $align);
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed previously static_assertions hasn't been updated in 4 years, I wonder if we can make the same checks using const generics or some other mechanism in latest rust. Just a thought, not for this PR.

https://github.com/nvzqz/static-assertions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you want to check exactly?

@exdx
Copy link
Contributor

exdx commented Mar 29, 2024

Closes #181

@cavemanloverboy
Copy link
Contributor Author

^ last commit, just wanted to commit latest lock

@losman0s losman0s merged commit 2b750c4 into main Mar 30, 2024
4 checks passed
jkbpvsc pushed a commit that referenced this pull request Oct 15, 2024
* explicit 8-byte alignment

* fix linter

* update lock files

* update locks

* upgrade fuzzer's rustc

* add toolchain for fuzzer

* lower bumpalo version

* fix ahash

* fix ahash

* upgrade rust toolchain for fuzz

* Update nightly toolchain for fuzzer

* Update test.yaml

* latest lock
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.

4 participants