-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Panic inside panic when procedural macro is called with proc_macro::bridge::client #60593
Comments
First off, you have both of these in the stack trace:
Did you build the former yourself? If not, how come it doesn't have the hash in it? I'm suspecting you have a One thing you could try is to have a proc macro crate as a dependency of your project, which will ensure it gets compiled with the same compiler and against the same Proc macros presumably work on nightly, and officially your usecase is not supported, so I can't spend a lot of time looking into it, sorry, but I'll try to when I can. |
@eddyb thanks for your time and efforts! If I understand your question correctly, then no, I did not build However, I'm using I don't get the part about making proc_macro a dependency. I wasn't able to add it to the |
Not |
I've tried to do that, unfortunately with no luck I've updated the repo with test that uses path dependency (you should |
@fedochet "no luck" as in "it still causes the panic"? Or does it work as intended in that configuration? |
@eddyb yes, still |
Alright, so a potential reason for |
Just copy from the project issue i wrote: After a little bit of digging, my guess is, it is not related to ABI compatible problem. It seem to be related to the different of The actual panic is this line. rust/src/libproc_macro/bridge/client.rs Line 269 in dc6db14
On the other hand, in the example project:
And after that, when the actual TokenStream start to parse, Bridge::with will be called from shared library crate (which is dynamic link) and it will trigger the panic. It seem to be two thread local are different, such that the panic get raised. But I don't know enough about linux shared library and thread local mechanism to understand this bug. |
@edwin0cheng Static vs dynamic linking does sound scary. rust/src/libproc_macro/bridge/client.rs Lines 341 to 351 in 7da1185
Which is used as a To investigate further, you could transmute the |
@eddyb Thanks for the information, it helps a lot for me to understand how it works. I seem to understand why this bug happens, would you mind to tell me does it make sense ? I can confirmed that the Why the static linkage is introduced ? Surprisingly it is introduced by But in rustc it doesn't use rust/src/libsyntax_ext/proc_macro_impl.rs Line 17 in 9ebf478
use syntax::tokenstream::TokenStream;
// ....
impl base::AttrProcMacro for AttrProcMacro {
fn expand<'cx>(&self,
ecx: &'cx mut ExtCtxt<'_>,
span: Span,
annotation: TokenStream,
annotated: TokenStream)
-> TokenStream {
let server = proc_macro_server::Rustc::new(ecx);
match self.client.run(&EXEC_STRATEGY, server, annotation, annotated) {
// ....
} And that's why in server.rs, it use a Server trait : impl client::Client<fn(crate::TokenStream) -> crate::TokenStream> {
pub fn run<S: Server>( So that it won't introduce the static linkage of thread_local in their code. |
There is a dynamic indirection between the Can you do what I said? Print those transmuted pointers? |
@eddyb Sure :
The code I insert for proc_macro in *proc_macros {
match proc_macro {
ProcMacro::Bang { client, .. } => {
unsafe {
let p1: &[*const (); 3] = std::mem::transmute(client);
let f = &proc_macro::bridge::client::__run_expand1;
println!("{:#?}", p1);
println!("{:#?}", f as *const _);
}
let result = client.run(
&EXEC_STRATEGY,
rustc_server::Rustc::default(),
parse_string("struct S{}").expect("Cannot parse code"),
); It is different. However, the other impl Bridge<'_> {
fn enter<R>(self, f: impl FnOnce() -> R) -> R {
let p_enter = &BRIDGE_STATE as *const _; // <-------------
dbg!(p_enter); // <-------------
// Hide the default panic output within `proc_macro` expansions.
// NB. the server can't do this because it may use a different libstd.
static HIDE_PANICS_DURING_EXPANSION: Once = Once::new();
HIDE_PANICS_DURING_EXPANSION.call_once(|| {
let prev = panic::take_hook();
panic::set_hook(Box::new(move |info| {
let hide = BridgeState::with(|state| match state {
BridgeState::NotConnected => false,
BridgeState::Connected(_) | BridgeState::InUse => true,
});
if !hide {
prev(info)
}
}));
});
BRIDGE_STATE.with(|state| state.set(BridgeState::Connected(self), f))
}
fn with<R>(f: impl FnOnce(&mut Bridge<'_>) -> R) -> R {
let p_with = &BRIDGE_STATE as *const _; // <-------------
dbg!(p_with); // <-------------
BridgeState::with(|state| match state {
BridgeState::NotConnected => {
panic!("procedural macro API is used outside of a procedural macro");
}
BridgeState::InUse => {
panic!("procedural macro API is used while it's already in use");
}
BridgeState::Connected(bridge) => f(bridge),
})
}
} |
|
Here is the output of
impl Bridge<'_> {
fn enter<R>(self, f: impl FnOnce() -> R) -> R {
BRIDGE_STATE.with(|s| {
let p_enter = s as *const _;
dbg!(p_enter);
});
// Hide the default panic output within `proc_macro` expansions.
// NB. the server can't do this because it may use a different libstd.
static HIDE_PANICS_DURING_EXPANSION: Once = Once::new();
HIDE_PANICS_DURING_EXPANSION.call_once(|| {
let prev = panic::take_hook();
panic::set_hook(Box::new(move |info| {
let hide = BridgeState::with(|state| match state {
BridgeState::NotConnected => false,
BridgeState::Connected(_) | BridgeState::InUse => true,
});
if !hide {
prev(info)
}
}));
});
BRIDGE_STATE.with(|state| state.set(BridgeState::Connected(self), f))
}
fn with<R>(f: impl FnOnce(&mut Bridge<'_>) -> R) -> R {
BRIDGE_STATE.with(|s| {
let p_with = s as *const _;
dbg!(p_with);
});
BridgeState::with(|state| match state {
BridgeState::NotConnected => {
panic!("procedural macro API is used outside of a procedural macro");
}
BridgeState::InUse => {
panic!("procedural macro API is used while it's already in use");
}
BridgeState::Connected(bridge) => f(bridge),
})
}
} Anyway, seem like this problem can be solved by adding a flag https://stackoverflow.com/questions/34073051/when-we-are-supposed-to-use-rtld-deepbind |
I can confirm that Maybe the whole problem wasn't noticed in the compiler itself because the compiler and the procedural macros are compiled by different compilers after all? I mean that the final stage of the compiler is compiled with the previous stage compiler, and the macros is compiled with the final stage compiler and then is linked into it In my example, the panic occurs only when the same compiler is used for the procedural macro and for the linking code compilation (and without |
@edwin0cheng Okay that's definitely not supposed to happen, assuming all of those calls are coming from inside the same proc macro |
I think there are some misunderstandings about the usage of @fedochet project here. Let me try to explain more.
And here is the output
Note that the first and second line is coming from step 1(
It is true. But different compilers helps here since the relocation address are different, the dynamic linker will treat as they are different symbol, such that the searches result of symbols will be different. |
Technically |
@fedochet @edwin0cheng I can't reproduce
That last one is I'm curious, what's your |
@eddyb mine is 2.23 according to
|
@fedochet Any chance you could update? I've just asked some other people with Ubuntu installs and they can reproduce with 2.23 but not 2.27. I'm starting to strongly suspect an |
@eddyb I've just tried 2.27 with I guess that to prevent dependency from (possibly) broken version of |
I was finally able to reproduce, by running this: NIX_PATH=nixpkgs=https://github.com/NixOS/nixpkgs-channels/archive/nixos-18.03.tar.gz \
nix-shell --pure -p rustup --run '\
export set RUSTUP_HOME=$(pwd)/rustup-glibc-2.26; \
rustup default nightly && \
cargo clean && \
cargo test -- --nocapture' So that's using |
Uhm, this looks like a drive-by bugfix: bminor/glibc@9d7a374#diff-249add90a52e252f250ab509ffbcf72fL125. |
@eddyb I am not sure it is worth your time - we already have two options for fixing this (using Thank you and @edwin0cheng for digging into this! It would have taken me forever to pinpoint where the problem actually is. Maybe I will try to narrow it to specific line in |
I found another thing that makes this bug go away (on I've also narrowed it down to a very simple test. I can finally see what's happening: That side-effect happens to be "defining" So Presumably this behavior got changed because it introduces unsound inconsistencies into TUs? EDIT: I should also say that this cannot possibly happen within |
I just tried changing EDIT: also, I just noticed that |
This is what i tried to say in my previous post :) |
Why |
Oh dear, it looks like it's #54592 (comment). |
@vlad20012 It should be more like |
@edwin0cheng Yeah, I realize now, and I'm sorry it took me this much, but there were too many levels of confusion, before I arrived at the reduction with one |
Amazingly enough, I, too, am seeing a similar error message! Can you show/tell how you applied the Thanks for all the work that everyone is doing to dig into this problem! |
3920: Implement expand_task and list_macros in proc_macro_srv r=matklad a=edwin0cheng This PR finish up the remain `proc_macro_srv` implementation : 1. Added dylib loading code for proc-macro crate dylib. Note that we have to add some special flags for unix loading because of a bug in old version of glibc, see fedochet/rust-proc-macro-panic-inside-panic-expample#1 and rust-lang/rust#60593 for details. 2. Added tests for proc-macro expansion: We use a trick here by adding `serde_derive` to dev-dependencies and calling `cargo-metadata` for searching its dylib path, and expand it in our tests. [EDIT] Note that this PR **DO NOT** implement the final glue code with rust-analzyer and proc-macro-srv yet. Co-authored-by: Edwin Cheng <edwin0cheng@gmail.com>
Not sure yet if relevant, but: turns out my expectation of proc macros being "most rust/compiler/rustc_codegen_ssa/src/back/linker.rs Lines 651 to 661 in 9067d52
AFAICT that's what's causing proc macro dylibs to have so many dynamic exports (instead of exactly one, the |
@fedochet @edwin0cheng @hawkinsw Checking in now that this landed: Can any of you confirm that:
We can likely close this issue once confirmed. Thanks in advance! |
Thanks @eddyb for your investigation and fixes for this issue ! And since the proc-macro bridge in rust was changed a lot, the original example code from @fedochet is obsoleted. And it confirmed your fixes works ! I think we could close this issue. |
@edwin0cheng Well, you should thank @bjorn3 ;) - I'm happy it's over though! |
I am experiencing a strange panic that happens inside procedural macros when they are compiled as dynamic libraries, dynamically linked and then called explicitly using
proc_macro::bridge::client
.It's quite hard to reproduce in small amount of code, so I've made up an example project where you can launch the tests and see the panic, it is here. Specifically, the stack backtrace can be seen here.
I've experimented with different compiler versions, and it seems that the only thing that triggers such panic is version alignment between compiler that compiled procedural macro itself, and compiler that compiled the code that is calling run method. As long as those compilers have different versions, everything works as expected.
I wasn't able to find the reason of this problem myself, so I thought it may be useful for you to know about this behaviour (maybe it is a bug of some sort).
The text was updated successfully, but these errors were encountered: