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

Using thread-local state inside proc-macro causes ICE #66003

Open
olegnn opened this issue Oct 31, 2019 · 9 comments
Open

Using thread-local state inside proc-macro causes ICE #66003

olegnn opened this issue Oct 31, 2019 · 9 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-proc-macros Area: Procedural macros A-thread-locals Area: Thread local storage (TLS) E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@olegnn
Copy link
Contributor

olegnn commented Oct 31, 2019

ICE will be produced on any channel.

Error:

thread 'rustc' panicked at 'use-after-free in `proc_macro` handle', src/libcore/option.rs:1190:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.39.0-beta.1 (968967007 2019-09-24) running on x86_64-apple-darwin

note: compiler flags: -C debuginfo=2 -C incremental --crate-type bin

note: some of the compiler flags provided by cargo are hidden

error: custom attribute panicked
 --> proc_macro_bug/src/main.rs:7:1
  |
7 | #[some_macro(0)]
  | ^^^^^^^^^^^^^^^^
  |
  = help: message: use-after-free in `proc_macro` handle

error: aborting due to previous error

error: could not compile `proc_macro_bug`.

LocalKey used in macro generation:

extern crate proc_macro;
extern crate quote;
extern crate syn;

use proc_macro::TokenStream;
use quote::quote;
use syn::{parse_quote, Path};

thread_local! {
    static DEFAULT_CRATE_PATH: Path = parse_quote! { ::std };
}

#[proc_macro_attribute]
pub fn some_macro(_: TokenStream, _: TokenStream) -> TokenStream {
    TokenStream::from(DEFAULT_CRATE_PATH.with(|default_crate_path| {
        quote! {
            use #default_crate_path::boxed::Box;
        }
    }))
}

Macro calls (should be 2 at least) which produce error:

#[some_macro(0)]
struct Abc {}

#[some_macro(0)]
struct Cde {}

fn main() {}

Repo with minimal code sample: https://github.com/olegnn/proc-macro-ICE.

@csmoe csmoe added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) labels Nov 1, 2019
@Mark-Simulacrum
Copy link
Member

Seems to have been fixed, at least, I cannot reproduce. Closing. I don't think a test is needed here.

@olegnn
Copy link
Contributor Author

olegnn commented Feb 15, 2020

@Mark-Simulacrum latest nightly:

thread 'rustc' panicked at 'use-after-free in `proc_macro` handle', /rustc/433aae93e4ef866a1fdfefad136b32ed89acd3e7/src/libproc_macro/bridge/handle.rs:42:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.43.0-nightly (433aae93e 2020-02-14) running on x86_64-apple-darwin

note: compiler flags: -C debuginfo=2 -C incremental --crate-type bin

note: some of the compiler flags provided by cargo are hidden

Check the repo https://github.com/olegnn/proc-macro-ICE if you can't reproduce on playground.

@Mark-Simulacrum
Copy link
Member

I feel like this is intentional behavior, though I guess we probably want to produce an error rather than ICEing. Reopening.

@JohnTitor JohnTitor added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 9, 2020
@Centril Centril added the A-diagnostics Area: Messages for errors, warnings, and lints label Mar 10, 2020
@Centril
Copy link
Contributor

Centril commented Mar 10, 2020

cc @eddyb @petrochenkov

@eddyb
Copy link
Member

eddyb commented Mar 10, 2020

It's generally hard to handle this gracefully due to how deep it is in the whole system - usually a panic inside a proc macro would turn into an error but here it doesn't seem to.
Maybe we just need to include more adjacent code in some catch_panic scope?


@olegnn This kind of code can't work, every time a proc macro is ran, the proc_macro types (e.g. TokenStream) have incompatible values with other runs.
Ideally we'd use multiple threads, or clear TLS from the proc macro somehow, but we'd need more optimizations to make the former viable, and I don't even know if the latter is possible.

It's probably better to replace DEFAULT_CRATE_PATH with a function, or even a struct DefaultCratePath; that implements ToTokens (if you want to be able to use #DefaultCratePath).

@Aaron1011 Aaron1011 added the A-proc-macros Area: Procedural macros label May 21, 2020
csnover added a commit to csnover/binread that referenced this issue Feb 27, 2021
The original purpose of most of this refactoring was actually to
cache the generated `TokenStream`s, since constantly regenerating
these takes a long time (an extra ~20000ns per call) and these
calls happen thousands of times.

However, it turns out that:

1. It not valid to retain any TokenStream between different
   invocations of a proc-macro. Trying will cause the compiler to
   crash (rust-lang/rust#66003);
2. `syn::Ident` has some problem where it will also cause the
   compiler to crash if it ends up in thread-local storage
   (rust-lang/rust#80473) and upstream seems to want to stop
   any thread-local storage in proc-macros eventually
   (rust-lang/rust#56058).

Being able to cache these tokens would provide a small but decent
speedup (~3%), so it is unfortunate that this is not possible.
Still, the formatting changes made while trying to implement a
cache seem reasonable enough to keep in the tree.
csnover added a commit to csnover/binread that referenced this issue Mar 6, 2021
The original purpose of most of this refactoring was actually to
cache the generated `TokenStream`s, since constantly regenerating
these takes a long time (an extra ~20000ns per call) and these
calls happen thousands of times.

However, it turns out that:

1. It not valid to retain any TokenStream between different
   invocations of a proc-macro. Trying will cause the compiler to
   crash (rust-lang/rust#66003);
2. `syn::Ident` has some problem where it will also cause the
   compiler to crash if it ends up in thread-local storage
   (rust-lang/rust#80473) and upstream seems to want to stop
   any thread-local storage in proc-macros eventually
   (rust-lang/rust#56058).

Being able to cache these tokens would provide a small but decent
speedup (~3%), so it is unfortunate that this is not possible.
Still, the formatting changes made while trying to implement a
cache seem reasonable enough to keep in the tree.
jam1garner pushed a commit to jam1garner/binread that referenced this issue Mar 9, 2021
The original purpose of most of this refactoring was actually to
cache the generated `TokenStream`s, since constantly regenerating
these takes a long time (an extra ~20000ns per call) and these
calls happen thousands of times.

However, it turns out that:

1. It not valid to retain any TokenStream between different
   invocations of a proc-macro. Trying will cause the compiler to
   crash (rust-lang/rust#66003);
2. `syn::Ident` has some problem where it will also cause the
   compiler to crash if it ends up in thread-local storage
   (rust-lang/rust#80473) and upstream seems to want to stop
   any thread-local storage in proc-macros eventually
   (rust-lang/rust#56058).

Being able to cache these tokens would provide a small but decent
speedup (~3%), so it is unfortunate that this is not possible.
Still, the formatting changes made while trying to implement a
cache seem reasonable enough to keep in the tree.
@JohnTitor JohnTitor added the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Mar 3, 2023
@estebank
Copy link
Contributor

Current output:

error: custom attribute panicked
 --> proc-macro-bug/src/main.rs:7:1
  |
7 | #[some_macro(0)]
  | ^^^^^^^^^^^^^^^^
  |
  = help: message: use-after-free of `proc_macro` symbol

error: could not compile `proc-macro-bug` (bin "proc-macro-bug") due to previous error

@estebank estebank reopened this Mar 15, 2023
@estebank estebank added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. and removed I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ C-bug Category: This is a bug. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Mar 15, 2023
@peter-lyons-kehl
Copy link
Contributor

peter-lyons-kehl commented Mar 27, 2023

Hi Thank you for your investigation so far.

  1. proc_macro stream parts are perfectly suited for thread local re-use, because they
  • take long to parse, and
  • are immutable, !Sync and !Send, and
  • they are designed for re-use: their clone() is lightweight.

If (subsequent) proc_macro invocations are invoked in different threads, or shift-rota over a thread pool, that's fine with the above. It should also be fine with any well implemented proc macros (as user space-defined macros shouldn't have any side effects).

May I guess: The current restriction/ICE is in place to prevent/alert macro developers so as not to make a mess. Could we introduce a configuration option to suppress that check?

Yes, if a macro or its output depends on the thread (or the thread ID...), then the macro or its output/tests will break. But that's because such a macro is dirty.

  1. The actual error message gets to be much worse than (reports) above. With #[proc_macro_attribute] this problem doesn't show up as an (obvious/well named) ICE, but much more confusing. It does mentions neither thread_local!, nor proc macro at all!
  • cargo test
  • Compiling allows v0.1.0 (/share/pkehl/GIT/allows)
  • thread panicked while panicking. aborting.
  • error: could not compile allows (test "simple")
  • Caused by:
  • process didn't exit successfully: rustc --crate-name simple --edition=2021 tests/simple.rs --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat --diagnostic-width=41 --emit=dep-info,link -C embed-bitcode=no -C debuginfo=2 --test -C metadata=825a0056779a2e30 -C extra-filename=-825a0056779a2e30 --out-dir /share/pkehl/GIT/allows/target/debug/deps -C incremental=/share/pkehl/GIT/allows/target/debug/incremental -L dependency=/share/pkehl/GIT/allows/target/debug/deps --extern allows=/share/pkehl/GIT/allows/target/debug/deps/liballows-cdc70b39f5946a9c.so -Ctarget-cpu=native (signal: 6, SIGABRT: process abort signal)

See minimized:

The above unhelpful error message is the same on both nightly and stable. My minimized example needs nightly, but only for the actual test that uses the proc macro - to keep it all simple. The proc macro itself compiles (but fails to run) on stable, too.

@peter-lyons-kehl
Copy link
Contributor

peter-lyons-kehl commented Mar 27, 2023

(Updated): Producing ICE for me, too.

@tmandry tmandry added the F-thread_local `#![feature(thread_local)]` label Apr 27, 2023
@RalfJung RalfJung changed the title Use of std::thread::LocalKey inside proc-macro produces ICE Use of thread-local state inside proc-macro produces ICE Sep 7, 2023
@RalfJung RalfJung changed the title Use of thread-local state inside proc-macro produces ICE Cannot use thread-local state inside proc-macro Sep 7, 2023
@RalfJung RalfJung added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Sep 7, 2023
@RalfJung RalfJung changed the title Cannot use thread-local state inside proc-macro Using thread-local state inside proc-macro causes ICE Sep 7, 2023
@RalfJung RalfJung added A-thread-locals Area: Thread local storage (TLS) and removed F-thread_local `#![feature(thread_local)]` labels Sep 7, 2023
@ple1n
Copy link

ple1n commented Sep 26, 2023

It just happened for me

fn types() -> HashSet<Ident> {
    let idents: Punctuated<Ident, Comma> = parse_quote!(Select, End, Receive, Branch);
    idents.into_iter().collect()
}

thread_local! {
    static  TYPES: HashSet<Ident> = types();
}
error: custom attribute panicked
   --> netns-proxy-rs/src/tasks.rs:208:5
    |
208 |     #[session('k Self::Selector)]
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: message: use-after-free of `proc_macro` symbol

rustc 1.74.0-nightly (0288f2e 2023-09-25)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-proc-macros Area: Procedural macros A-thread-locals Area: Thread local storage (TLS) E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests