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

Do not build anchor syn for the Solana target #3062

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

Conversation

LucasSte
Copy link
Contributor

Function inside anchor/lang/syn only serve for code generation, but are also built for Solana programs, leading to spurious stack error warnings.

Copy link

vercel bot commented Jun 28, 2024

@LucasSte is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

@LucasSte LucasSte marked this pull request as ready for review June 28, 2024 18:41
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but could you mention how you got these spurious stack error warnings? You'd get it if you included anchor-syn as a direct dependency, but we don't have a direct dependency to it. You shouldn't run into this just from using anchor-lang.

@LucasSte
Copy link
Contributor Author

Thanks for the PR, but could you mention how you got these spurious stack error warnings? You'd get it if you included anchor-syn as a direct dependency, but we don't have a direct dependency to it. You shouldn't run into this just from using anchor-lang.

The contract I was testing had anchor-syn as dependency, bringing up errors about functions that are not in the final binary. Adding the CFG flag ensures syn is not built for Solana, removing the errors. As far as I could dig the code, syn only deals with code generation, right? It does not contain code to be executed on chain.

@acheroncrypto
Copy link
Collaborator

As far as I could dig the code, syn only deals with code generation, right? It does not contain code to be executed on chain.

Yeah it's used for code generation, though only exception currently is the hash feature:

anchor/lang/syn/src/hash.rs

Lines 102 to 116 in 0a1d458

#[cfg(target_arch = "bpf")]
{
extern "C" {
fn sol_sha256(vals: *const u8, val_len: u64, hash_result: *mut u8) -> u64;
}
let mut hash_result = [0; HASH_BYTES];
unsafe {
sol_sha256(
vals as *const _ as *const u8,
vals.len() as u64,
&mut hash_result as *mut _ as *mut u8,
);
}
Hash::new_from_array(hash_result)
}

This feature is not being used much at all, but someone ran into an issue with it last year and submitted a fix (#2682).

This is also what I meant when I said unreachable code in #3060 (comment). You get unnecessary stack errors even though that code never gets called. You don't even need a dependency for it, as you also get those errors if you just create a function that overflows the stack even if it never gets called. For example:

pub fn _random_function_that_never_gets_called() {
    let x = [u8::MAX; 8192];
    format!("{x:?}");
}

This should be fixed before fully blocking compilation imo.

@LucasSte
Copy link
Contributor Author

Now the issue with hash in the BPF target is interesting. The BPF target is deprecated for almost two years: solana-labs/solana#28125. We also have been replacing the bpf parameters to sbf since then.

https://github.com/solana-labs/solana/blob/27eff8408b7223bb3c4ab70523f8a8dca3ca6645/sdk/cargo-build-bpf/src/main.rs#L13-L18

@acheroncrypto
Copy link
Collaborator

I don't see why that code should exist in anchor-syn, probably was forgotten after copying solana_program::hash. Removing it in #3078.

@LucasSte
Copy link
Contributor Author

You get unnecessary stack errors even though that code never gets called. You don't even need a dependency for it, as you also get those errors if you just create a function that overflows the stack even if it never gets called. For example:

I can think of unused code in two ways:

  1. You add a dependency in Cargo.toml that is unneeded. In this case, cargo will invoke rust for it, even though it is not necessary. Cargo does not yet warn about unused dependencies, although there are tools to detect so. My suggestion for this case would be to put code that shouldn't go to contracts (like anchor syn, responsible for code generation) behind the #![cfg(not(target_os = "solana"))] flag to guarantee no one will see spurious stack errors (I can see another one still, but I haven't pinpointed the location on Anchor yet).

  2. You have an unused function in your code. In this case, I tested it here rustc warns about an unused function without emitting the stack error. This means it left it out of SBF code generation. If I use it, I see the stack error.

@acheroncrypto
Copy link
Collaborator

  1. You add a dependency in Cargo.toml that is unneeded. In this case, cargo will invoke rust for it, even though it is not necessary. Cargo does not yet warn about unused dependencies, although there are tools to detect so.

It's not exactly about unused dependencies, it's about unused functions. You can add a dependency and use many functions from that library that doesn't overflow the stack, but you still get the stack error logs if there is a function that overflows the stack even it's not getting called from your program (or from your dependencies).

  1. My suggestion for this case would be to put code that shouldn't go to contracts (like anchor syn, responsible for code generation) behind the #![cfg(not(target_os = "solana"))] flag to guarantee no one will see spurious stack errors

This kind of assumes people only use Solana crates in their program, we can't expect other crates in the Rust ecosystem to add that check to their crates.

  1. You have an unused function in your code. In this case, I tested it here rustc warns about an unused function without emitting the stack error. This means it left it out of SBF code generation. If I use it, I see the stack error.

Did you make the function you're testing public like the example I shared above? Putting the above function in lib.rs and running cargo-build-sbf is enough to trigger stack error logs for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants