-
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
use of proc_macro[2]::TokenStream in a vector in TLS causes rustc panic #63804
Comments
Note that procedural macros do not support retaining state between executions or sharing state between macros. That said, |
cc @eddyb -- We get this question every now and then... I think we need to clarify somewhere that this is explicitly unsupported. |
No, that would behave much more differently. There is a server attached, it just didn't recognize that handle you held onto (we use atomic counters to guarantee we can detect this). @Centril Something more drastic we could attempt is leak-checking (note that this only works for non-interned handles, so The problem is that leaks could be legitimate (e.g. |
@eddyb That does sound drastic but perhaps worth a try -- could we perhaps look for |
Kinda tricky to mess around with the layers of indirection around |
@eddyb Let's give it a try as an error (at first) and crater that? |
Thank you for responding! Can you explain to me why I am keeping something
too long? Both of these proc macro implementations are invoked from the
same thread, I believe. If they weren't, then they would access different
different vectors through the refcell. I'm not keeping references to the
destination items across calls.
…On Thu, Aug 22, 2019, 9:09 AM Mazdak Farrokhzad ***@***.***> wrote:
@eddyb <https://github.com/eddyb> Let's give it a try as an error (at
first) and crater that?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#63804?email_source=notifications&email_token=ACCP2CQWQAVOC4LCXGCPFYTQF2FYRA5CNFSM4IOS6LTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45A3FA#issuecomment-523898260>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACCP2CQETZZD3AN7HWM2QXDQF2FYRANCNFSM4IOS6LTA>
.
|
The only thing I could imagine that is being retained incorrectly is
something internal to the implementation of a tokenstream that is specific
to the proc macro implementation in which it is constructed. Otherwise, my
use case seems entirely legitimate.
In other words, it is not that sharing generally is unsupported in this
case it is, specifically, the sharing of tokenstream across invocations.
If I retained state across implementations with a refcell of a vector of
"thing" where the implementation of "thing" does not make use of any global
state, then all should be well and good.
Please correct me where I am wrong. Thanks again!
…On Thu, Aug 22, 2019, 10:08 AM Will Hawkins ***@***.***> wrote:
Thank you for responding! Can you explain to me why I am keeping something
too long? Both of these proc macro implementations are invoked from the
same thread, I believe. If they weren't, then they would access different
different vectors through the refcell. I'm not keeping references to the
destination items across calls.
On Thu, Aug 22, 2019, 9:09 AM Mazdak Farrokhzad ***@***.***>
wrote:
> @eddyb <https://github.com/eddyb> Let's give it a try as an error (at
> first) and crater that?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#63804?email_source=notifications&email_token=ACCP2CQWQAVOC4LCXGCPFYTQF2FYRA5CNFSM4IOS6LTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45A3FA#issuecomment-523898260>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACCP2CQETZZD3AN7HWM2QXDQF2FYRANCNFSM4IOS6LTA>
> .
>
|
In the original issue report, you wrote:
Which is definitely unsupported (generally you shouldn't assume you can communicate between invocations in any way, we don't really guarantee the order your functions will be called in). What might be happening (and which I overlooked above, my bad) is whenever the If you want to keep it in a thread-local for the duration of one invocation, you could look at |
I completely understand that this is completely unsupported. That said, if possible, I'd like to continue this discussion based on the current implementation and assume that I am willing to accept the attendant risks should the implementation change. I know that you are busy, so please feel free to say that you have better things to do than discuss this, obviously stupid choice ...
If I understand correctly, there is some global state shared among all instantiated
Does this accurately summarize the issue? If it does, then I am completely satisfied with the state of this very productive discussion and feel like I completely understand. Obviously I would be very happy knowing that I understand and will offer to make suggestions (through PRs) to changes to the documentation to make it more obvious for others why what I am attempting to do is "not good". Going back to the hypothetical discussion (where I am okay relying on implementation-specific behavior that is completely unsupported and may change at any time), this gives me a path forward. Again, thank you for the discussion. It's been very helpful.
|
I think that message is a bit too confusing (but I didn't know what to call it). I should explain a bit more about how the system works. Whenever you get a So if you had two uses of // server connects and is told counter starts at 1
server calls arm(TokenStream(1), TokenStream(2))
TokenStream::new() returns TokenStream(3)
TokenStream(3) ends up in TLS (leaking it)
TokenStream(2) is returned
// server still knows about TokenStream(3)
// server disconnects (taking TokenStream(3)'s actual data with it) // server connects and is told counter starts at 4
server calls arm(TokenStream(4), TokenStream(5))
TokenStream::new() returns TokenStream(6)
TokenStream(6) ends up in TLS (leaking it)
TokenStream(5) is returned
// server still knows about TokenStream(6)
// server disconnects (taking TokenStream(6)'s actual data with it) So at the end you have If you tried to access the saved
No implementation-specific behavior here. Keeping
I assume the reason for "occasionally" here has to do with whether your code accesses the handles stored in TLS from other invocations, or there is something non-deterministic about TLS drop order.
All the associated data is server-side and it's gone by the time an invocation is finished, before the thread exits - all of that is harmless (e.g. if you were leaking handles with The only way a |
Wait, I'm glad I'm wrong about the TLS panics, they're just... always broken: struct Bomb;
impl Drop for Bomb {
fn drop(&mut self) {
panic!("bomb dropped");
}
}
thread_local!(static BOMB: Bomb = Bomb);
fn main() {
BOMB.with(|_| {}); // arm the bomb
} try on playpen - results in this output:
cc @RalfJung @alexcrichton @gnzlbg Has this been reported before? Since C calls into the TLS destructor hooks, I don't think we can unwind out into C, so TLS destructor panics should abort? |
Another clue: if you see As you can see above, panicking should always print a message, before trying to unwind ("failed to initiate panic" should probably say "failed to (start) unwind(ing)" instead). |
After reading your explanation, I am convinced that we are on the same page. I was just using different words since I didn't understand the behind-the-scenes details. Thank you for taking the time to explain. I get it so much more. Please see below for an additional question/clarification! Thanks again for this description.
I think that we are saying exactly the same thing but with different words because I didn't know exactly the implementation of proc_macro. Let me play this back and see if I have it correct: For every invocation of a function f that implements a proc macro, there is a server connection that may be used by the implementation in proc_macro features (I am assuming this is to facilitate a single point of contact with a thread that knows how to parse/tokenize/etc). One such feature would be int fd = open(...);
close(fd);
write(fd, ...); will not work, using a proc_macro resource with handle h created during the lifetime of connection c outside c's lifetime won't work either.
will work fine. This is why I was seeing these errors only when the RefCell in TLS owned a vector of Please correct me where I am wrong, but I feel pretty confident that I get it. That's only because you spent the time explaining -- thank you!
|
I should've just compared proc_macro handles to file descriptors, your write-after-close example is spot on! Another way to see the TLS situation is something like running the same program multiple times, and every time it appends a fd into a file. You could then look at all of those fd's but they are meaningless, because those processes no longer exist. You can use strings in TLS, yes, but keep in mind we might not run all of your invocations, or always in the same order, so it's not usable for what you want, I'm afraid. @Centril hmm we should also do a crater run in which we error if there are non-frozen statics in a proc-macro crate (would catch thread_local! too). You should instead have another attribute on the enclosing function (for example) in which you use cc @petrochenkov @dtolnay (who might be able to help you come up with a working design) |
I'm glad that I grok!
Yes, this is a great example, too! In fact, I think this is a better example!
Completely understand. See below for additional context on this design/implementation.
Yes, this is exactly the design that I initially considered. I'll tell you why I didn't go for it immediately: I wanted a POC for whether or not this type of crate API is useful in my own project. The correct design would require more in-depth implementation and I just wanted to get something work in the short term. I will go back and rewrite now that I've proven that the API is something that works! Thank you so much for digging in to this issue with me. I really appreciate your patience and your willingness to read and comprehend each my responses before sending your responses (that's really hard to do in a communication medium like this). I understand it so much better. Is there something that I can do to help make the documentation around this better? If so, please let me know. I am a compiler geek and eager to contribute to rustc if/where I can. Thanks again! |
I have not heard about this issue before, but what you are saying makes sense. TLS destructor hooks are basically "extern" fn. We probably need a panic-aborting wrapper around them. Can you report an issue specifically about that? |
[WIP] rustc_mir: disallow global mutable state in proc macros. Along the lines of #63809, this PR attempts to get rid of the main (or only?) place `proc_macro` handles could be leaked to, *and* further disallow/discourage sharing (other) state between invocations. The approach of banning (interior-)mutable `static`s was most recently mentioned in #63804 (comment), but it's likely been brought up several times, we just never tried it. (Note that this is not foolproof: one would have to scan all dependencies for such `static`s, modulo `proc_macro`/`std`, and even then it's possible there would be a lot of false positives) So this is mostly for a check-only crater run, to see what (if anything) breaks.
I am relatively new to rust, but I am a huge, huge fan. I hate to report an issue and hope that it's something worthwhile.
I am attempting to use
proc_macro[2]::TokenStream
in the implementation of a proc macro. I share a vector ofTokenStream
s across multiple invocations of those proc macros using storage allocated withthread_local!
. That sounds complicated, but I promise it's not:Here's how I could use these proc macros:
Here is my Cargo.toml
If it's easier, you can grab these files from this git repository: http://git.obs.cr/hawkinsw/rust_macro_error
When I run
cargo build
, I get arustc
failure. I have attempted to compile this with stable, nightly and "head" (stage2 compiler of HEAD). I get the same error no matter which compiler:fatal runtime error: failed to initiate panic, error 5
I can run the compiler under
gdb
but see nothing worthwhile:There are a few interesting things that I have noticed while attempting to debug this:
String
, in the TLS vector, everything works fine.use-after-free in 'proc_macro' handle
and can narrow that down to line 315 in libproc_macro/bridge/client.rs. I understand that to mean that the "server" for the proc_macro implementation is not alive, for some reason. I have seen Panic inside panic when procedural macro is called with proc_macro::bridge::client #60593 and think that they are related but cannot seem to find the common thread.
I have debugged as much as I can and I am reaching out for your help! I would love to help solve the problem if you can point me in the right direction.
Again, thank you for all the work that you are doing for rust and the community around the language.
The text was updated successfully, but these errors were encountered: