From 05445c718e6dda2f166a0ce1d2c7e1f85eddd83b Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Sat, 20 Jun 2020 01:42:11 -0400 Subject: [PATCH] Fix hygiene issues in `declare_program!` and `declare_loader!` The `declare_program!` and `declare_loader!` macros both expand to new macro definitions (based on the `$name` argument). These 'inner' macros make use of the special `$crate` metavariable to access items in the crate where the 'inner' macros is defined. However, this only works due to a bug in rustc. When a macro is expanded, all `$crate` tokens in its output are 'marked' as being resolved in the defining crate of that macro. An inner macro (including the body of its arms) is 'just' another set of tokens that appears in the body of the outer macro, so any `$crate` identifiers used there are resolved relative to the 'outer' macro. For example, consider the following code: ```rust macro_rules! outer { () => { macro_rules! inner { () => { $crate::Foo } } } } ``` The path `$crate::Foo` will be resolved relative to the crate that defines `outer`, **not** the crate which defines `inner`. However, rustc currently loses this extra resolution information (referred to as 'hygiene' information) when a crate is serialized. In the above example, this means that the macro `inner` (which gets defined in whatever crate invokes `outer!`) will behave differently depending on which crate it is invoked from: When `inner` is invoked from the same crate in which it is defined, the hygiene information will still be available, which will cause `$crate::Foo` to be resolved in the crate which defines 'outer'. When `inner` is invoked from a different crate, it will be loaded from the metadata of the crate which defines 'inner'. Since the hygiene information is currently lost, rust will 'forget' that `$crate::Foo` is supposed to be resolved in the context of 'outer'. Instead, it will be resolved relative to the crate which defines 'inner', which can cause incorrect code to compile. This bug will soon be fixed in rust (see https://github.com/rust-lang/rust/pull/72121), which will break `declare_program!` and `declare_loader!`. Fortunately, it's possible to obtain the desired behavior (`$crate` resolving in the context of the 'inner' macro) by use of a procedural macro. This commit adds a `respan!` proc-macro to the `sdk/macro` crate. Using the newly-stabilized (on Nightly) `Span::resolved_at` method, the `$crate` identifier can be made to be resolved in the context of the proper crate. Since `Span::resolved_at` is only stable on the latest nightly, referencing it on an earlier version of Rust will cause a compilation error. This requires the `rustversion` crate to be used, which allows conditionally compiling code epending on the Rust compiler version in use. Since this method is already stabilized in the latest nightly, there will never be a situation where the hygiene bug is fixed (e.g. https://github.com/rust-lang/rust/pull/72121) is merged but we are unable to call `Span::resolved_at`. --- Cargo.lock | 14 +++++++ programs/bpf_loader/Cargo.toml | 1 + programs/bpf_loader/src/lib.rs | 8 ++++ sdk/Cargo.toml | 1 + sdk/macro/Cargo.toml | 1 + sdk/macro/src/lib.rs | 73 +++++++++++++++++++++++++++++++++- sdk/src/entrypoint_native.rs | 70 ++++++++++++++++++++++++++------ sdk/src/lib.rs | 2 + 8 files changed, 155 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bd199667dc..4bb92dcab3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3253,6 +3253,17 @@ dependencies = [ "webpki", ] +[[package]] +name = "rustversion" +version = "1.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9bdc5e856e51e685846fb6c13a1f5e5432946c2c90501bdc76a1319f19e29da" +dependencies = [ + "proc-macro2 1.0.17", + "quote 1.0.6", + "syn 1.0.27", +] + [[package]] name = "rusty-fork" version = "0.2.2" @@ -3775,6 +3786,7 @@ dependencies = [ "num-derive 0.3.0", "num-traits", "rand 0.7.3", + "rustversion", "solana-runtime", "solana-sdk 1.3.0", "solana_rbpf", @@ -4642,6 +4654,7 @@ dependencies = [ "rand 0.7.3", "rand_chacha 0.2.2", "rustc_version", + "rustversion", "serde", "serde_bytes", "serde_derive", @@ -4674,6 +4687,7 @@ dependencies = [ "bs58 0.3.1", "proc-macro2 1.0.17", "quote 1.0.6", + "rustversion", "syn 1.0.27", ] diff --git a/programs/bpf_loader/Cargo.toml b/programs/bpf_loader/Cargo.toml index b31039a914..74eb549e99 100644 --- a/programs/bpf_loader/Cargo.toml +++ b/programs/bpf_loader/Cargo.toml @@ -21,6 +21,7 @@ thiserror = "1.0" [dev-dependencies] rand = "0.7.3" +rustversion = "1.0.3" [lib] crate-type = ["lib", "cdylib"] diff --git a/programs/bpf_loader/src/lib.rs b/programs/bpf_loader/src/lib.rs index 456f516db0..41d48d0c02 100644 --- a/programs/bpf_loader/src/lib.rs +++ b/programs/bpf_loader/src/lib.rs @@ -340,6 +340,14 @@ mod tests { } } + #[rustversion::since(1.46.0)] + #[test] + fn test_bpf_loader_same_crate() { + // Ensure that we can invoke this macro from the same crate + // where it is defined. + solana_bpf_loader_program!(); + } + #[test] #[should_panic(expected = "ExceededMaxInstructions(10)")] fn test_bpf_loader_non_terminating_program() { diff --git a/sdk/Cargo.toml b/sdk/Cargo.toml index 385b9efa89..6716fa615f 100644 --- a/sdk/Cargo.toml +++ b/sdk/Cargo.toml @@ -55,6 +55,7 @@ solana-crate-features = { path = "../crate-features", version = "1.3.0", optiona solana-logger = { path = "../logger", version = "1.3.0", optional = true } solana-sdk-macro = { path = "macro", version = "1.3.0" } solana-sdk-macro-frozen-abi = { path = "macro-frozen-abi", version = "1.3.0" } +rustversion = "1.0.3" [dev-dependencies] tiny-bip39 = "0.7.0" diff --git a/sdk/macro/Cargo.toml b/sdk/macro/Cargo.toml index 50c8234974..839e88f842 100644 --- a/sdk/macro/Cargo.toml +++ b/sdk/macro/Cargo.toml @@ -16,6 +16,7 @@ bs58 = "0.3.0" proc-macro2 = "1.0" quote = "1.0" syn = { version = "1.0", features = ["full", "extra-traits"] } +rustversion = "1.0.3" [package.metadata.docs.rs] targets = ["x86_64-unknown-linux-gnu"] diff --git a/sdk/macro/src/lib.rs b/sdk/macro/src/lib.rs index e58d9b4319..f130a53055 100644 --- a/sdk/macro/src/lib.rs +++ b/sdk/macro/src/lib.rs @@ -5,7 +5,7 @@ extern crate proc_macro; use proc_macro::TokenStream; -use proc_macro2::Span; +use proc_macro2::{Delimiter, Span, TokenTree}; use quote::{quote, ToTokens}; use std::convert::TryFrom; use syn::{ @@ -14,7 +14,7 @@ use syn::{ parse_macro_input, punctuated::Punctuated, token::Bracket, - Expr, Ident, LitByte, LitStr, Token, + Expr, Ident, LitByte, LitStr, Path, Token, }; struct Id(proc_macro2::TokenStream); @@ -63,6 +63,75 @@ impl ToTokens for Id { } } +#[allow(dead_code)] // `respan` may be compiled out +struct RespanInput { + to_respan: Path, + respan_using: Span, +} + +impl Parse for RespanInput { + fn parse(input: ParseStream) -> Result { + let to_respan: Path = input.parse()?; + let _comma: Token![,] = input.parse()?; + let respan_tree: TokenTree = input.parse()?; + match respan_tree { + TokenTree::Group(g) if g.delimiter() == Delimiter::None => { + let ident: Ident = syn::parse2(g.stream())?; + Ok(RespanInput { + to_respan, + respan_using: ident.span(), + }) + } + val @ _ => Err(syn::Error::new_spanned( + val, + "expected None-delimited group", + )), + } + } +} + +/// A proc-macro which respans the tokens in its first argument (a `Path`) +/// to be resolved at the tokens of its second argument. +/// For internal use only. +/// +/// There must be exactly one comma in the input, +/// which is used to separate the two arguments. +/// The second argument should be exactly one token. +/// +/// For example, `respan!($crate::foo, with_span)` +/// produces the tokens `$crate::foo`, but resolved +/// at the span of `with_span`. +/// +/// The input to this function should be very short - +/// its only purpose is to override the span of a token +/// sequence containing `$crate`. For all other purposes, +/// a more general proc-macro should be used. +#[rustversion::since(1.46.0)] // `Span::resolved_at` is stable in 1.46.0 and above +#[proc_macro] +pub fn respan(input: TokenStream) -> TokenStream { + // Obtain the `Path` we are going to respan, and the ident + // whose span we will be using. + let RespanInput { + to_respan, + respan_using, + } = parse_macro_input!(input as RespanInput); + // Respan all of the tokens in the `Path` + let to_respan: proc_macro2::TokenStream = to_respan + .into_token_stream() + .into_iter() + .map(|mut t| { + // Combine the location of the token with the resolution behavior of `respan_using` + // Note: `proc_macro2::Span::resolved_at` is currently gated with cfg(procmacro2_semver_exempt) + // Once this gate is removed, we will no longer need to use 'unwrap()' to call + // the underling `proc_macro::Span::resolved_at` method. + let new_span: Span = t.span().unwrap().resolved_at(respan_using.unwrap()).into(); + t.set_span(new_span); + t + }) + .collect(); + return TokenStream::from(to_respan); +} + #[proc_macro] pub fn declare_id(input: TokenStream) -> TokenStream { let id = parse_macro_input!(input as Id); diff --git a/sdk/src/entrypoint_native.rs b/sdk/src/entrypoint_native.rs index 04ecd3b0d6..24822d7986 100644 --- a/sdk/src/entrypoint_native.rs +++ b/sdk/src/entrypoint_native.rs @@ -30,6 +30,60 @@ pub type LoaderEntrypoint = unsafe extern "C" fn( invoke_context: &dyn InvokeContext, ) -> Result<(), InstructionError>; +#[rustversion::since(1.46.0)] +#[macro_export] +macro_rules! declare_name { + ($name:ident) => { + #[macro_export] + macro_rules! $name { + () => { + // Subtle: + // The outer `declare_name!` macro may be expanded in another + // crate, causing the macro `$name!` to be defined in that + // crate. We want to emit a call to `$crate::id()`, and have + // `$crate` be resolved in the crate where `$name!` gets defined, + // *not* in this crate (where `declare_name! is defined). + // + // When a macro_rules! macro gets expanded, any $crate tokens + // in its output will be 'marked' with the crate they were expanded + // from. This includes nested macros like our macro `$name` - even + // though it looks like a separate macro, Rust considers it to be + // just another part of the output of `declare_program!`. + // + // We pass `$name` as the second argument to tell `respan!` to + // apply use the `Span` of `$name` when resolving `$crate::id`. + // This causes `$crate` to behave as though it was written + // at the same location as the `$name` value passed + // to `declare_name!` (e.g. the 'foo' in + // `declare_name(foo)` + // + // See the `respan!` macro for more details. + // FIXME: Use `crate::respan!` once https://github.com/rust-lang/rust/pull/72121 + // is merged. + // `respan!` respans the path `$crate::id`, which we then call (hence the extra + // parens) + ( + stringify!($name).to_string(), + ::solana_sdk::respan!($crate::id, $name)(), + ) + }; + } + }; +} + +#[rustversion::not(since(1.46.0))] +#[macro_export] +macro_rules! declare_name { + ($name:ident) => { + #[macro_export] + macro_rules! $name { + () => { + (stringify!($name).to_string(), $crate::id()) + }; + } + }; +} + /// Convenience macro to declare a native program /// /// bs58_string: bs58 string representation the program's id @@ -102,13 +156,7 @@ pub type LoaderEntrypoint = unsafe extern "C" fn( macro_rules! declare_program( ($bs58_string:expr, $name:ident, $entrypoint:expr) => ( $crate::declare_id!($bs58_string); - - #[macro_export] - macro_rules! $name { - () => { - (stringify!($name).to_string(), $crate::id()) - }; - } + $crate::declare_name!($name); #[no_mangle] pub extern "C" fn $name( @@ -126,13 +174,9 @@ macro_rules! declare_program( macro_rules! declare_loader( ($bs58_string:expr, $name:ident, $entrypoint:expr) => ( $crate::declare_id!($bs58_string); + $crate::declare_name!($name); + - #[macro_export] - macro_rules! $name { - () => { - (stringify!($name).to_string(), $crate::id()) - }; - } #[no_mangle] pub extern "C" fn $name( diff --git a/sdk/src/lib.rs b/sdk/src/lib.rs index 99c3b7098f..0268e705ce 100644 --- a/sdk/src/lib.rs +++ b/sdk/src/lib.rs @@ -67,6 +67,8 @@ pub mod timing; /// ``` pub use solana_sdk_macro::declare_id; pub use solana_sdk_macro::pubkeys; +#[rustversion::since(1.46.0)] +pub use solana_sdk_macro::respan; // On-chain program specific modules pub mod account_info;