Stream: t-compiler/wg-rls-2.0

Topic: proc-macro


std::Veetaha (Apr 20 2020 at 14:02, on Zulip):

@Edwin Cheng @matklad is there a reason we don't use anyhow in ra_proc_macro* crates like we do in ra_project_model?

Edwin Cheng (Apr 20 2020 at 14:05, on Zulip):

Personally I just prevent to use any external crate as possible when I implement a new feature initially. But I think it is okay to change it to use anyhow.

matklad (Apr 20 2020 at 14:05, on Zulip):

I actually think that anyhow would be a bad fit there

matklad (Apr 20 2020 at 14:06, on Zulip):

The code isn't in cache for me, but I'd expect something like io::Result<Result<tt::TokenTree, ExpansionError>>

matklad (Apr 20 2020 at 14:08, on Zulip):

Ie, we have to failure modes, which we need to distinguish between:

std::Veetaha (Apr 20 2020 at 14:08, on Zulip):

Good point

std::Veetaha (Apr 20 2020 at 14:10, on Zulip):

By the way, I've just tried proc macros support with soa_derive crate and got

Fail to find proc macro. Error: Unknown(
    "Empty result",
)

@Edwin Cheng any assumptions?

std::Veetaha (Apr 20 2020 at 14:11, on Zulip):

The code:

use soa_derive::StructOfArray;

// #[macro_use]
// extern crate soa_derive;

#[derive(StructOfArray)]
pub struct Cheese {
    pub smell: f64,
    pub color: (f64, f64, f64),
    pub with_mushrooms: bool,
    pub name: String,
}

fn main() {
    let vec = CheeseVec::new();
}
Edwin Cheng (Apr 20 2020 at 14:11, on Zulip):

Any usage code example ?

Edwin Cheng (Apr 20 2020 at 14:13, on Zulip):

Um... no, but let me check now

Laurențiu Nicola (Apr 20 2020 at 14:16, on Zulip):

Works for me

Edwin Cheng (Apr 20 2020 at 14:16, on Zulip):

how do we handle incorrect syntax in proc macros?

@std::Veetaha We don't handle it in proc-macro part, we handle it in expanded
macro

Edwin Cheng (Apr 20 2020 at 14:19, on Zulip):

I works for me too.

Laurențiu Nicola (Apr 20 2020 at 14:20, on Zulip):

I mean, "works" as in "doesn't crash", but it doesn't seem to resolve the generated code

std::Veetaha (Apr 20 2020 at 14:20, on Zulip):

Okay, I'll try to debug this bit, unfortunately, there is too little info.
Maybe I forgot something? I added these guys:

    "rust-analyzer.procMacro.enabled": true,
    "rust-analyzer.cargo.loadOutDirsFromCheck": true,

and ran cargo build

Laurențiu Nicola (Apr 20 2020 at 14:21, on Zulip):

Are you on Linux?

std::Veetaha (Apr 20 2020 at 14:21, on Zulip):

@Laurențiu Nicola don't you see any error message in output?

std::Veetaha (Apr 20 2020 at 14:21, on Zulip):

On linux, right

Laurențiu Nicola (Apr 20 2020 at 14:21, on Zulip):

Nope, but I only have the server trace one, not the extension log

Laurențiu Nicola (Apr 20 2020 at 14:22, on Zulip):

Do you have the proc macro process?

Laurențiu Nicola (Apr 20 2020 at 14:22, on Zulip):

It shows up as rust-analyzer proc-macro

std::Veetaha (Apr 20 2020 at 14:24, on Zulip):

By running ps -a I haven't found any of this

std::Veetaha (Apr 20 2020 at 14:24, on Zulip):

I wonder how we ship this server, is this a separate binary?

Laurențiu Nicola (Apr 20 2020 at 14:26, on Zulip):

No, it's the same one, with an extra parameter

Edwin Cheng (Apr 20 2020 at 14:26, on Zulip):

No, we reuse the same binary

Laurențiu Nicola (Apr 20 2020 at 14:26, on Zulip):

Well, put a sleep were it starts, then try to strace it

Laurențiu Nicola (Apr 20 2020 at 14:27, on Zulip):

It's going to open the proc macro library, then look up a symbol inside it

Laurențiu Nicola (Apr 20 2020 at 14:28, on Zulip):

See how it's called (e.g. target/debug/deps/libsoa_derive_internal-227cd67b09461a28.so) and what symbols it exports

std::Veetaha (Apr 20 2020 at 14:28, on Zulip):

Ah, never worked with strace)

Laurențiu Nicola (Apr 20 2020 at 14:28, on Zulip):
nm -S target/debug/deps/libsoa_derive_internal-227cd67b09461a28.so | rg decl                                                                                          ✔
00000000004d73f0 0000000000000010 D __rustc_proc_macro_decls_4b07614003dcdb39fa49471d12911d65__
Laurențiu Nicola (Apr 20 2020 at 14:28, on Zulip):

sudo strace -fp <PID>

Laurențiu Nicola (Apr 20 2020 at 14:29, on Zulip):

or sudo strace -e trace=open,openat -fp <PID> to trace only open() calls

Edwin Cheng (Apr 20 2020 at 14:30, on Zulip):

Or you could add some dbg! in https://github.com/rust-analyzer/rust-analyzer/blob/90f837829d4f2c1054751de2de695ba1c0b8ae5c/crates/ra_proc_macro/src/process.rs#L48

Jeremy Kolb (Apr 20 2020 at 14:30, on Zulip):

I just flipped this on and everything slowed to a crawl

Laurențiu Nicola (Apr 20 2020 at 14:32, on Zulip):

Edwin Cheng said:

Or you could add some dbg! in https://github.com/rust-analyzer/rust-analyzer/blob/90f837829d4f2c1054751de2de695ba1c0b8ae5c/crates/ra_proc_macro/src/process.rs#L48

Yeah, well, strace is my golden hammer.

Laurențiu Nicola (Apr 20 2020 at 14:32, on Zulip):

Jeremy Kolb said:

I just flipped this on and everything slowed to a crawl

Do you have cargo check disabled?

Edwin Cheng (Apr 20 2020 at 14:33, on Zulip):

I wish i could use strace or any trace in Windows :(

std::Veetaha (Apr 20 2020 at 14:33, on Zulip):

Why not using linux?

Edwin Cheng (Apr 20 2020 at 14:33, on Zulip):

Jeremy Kolb said:

I just flipped this on and everything slowed to a crawl

which project?

Laurențiu Nicola (Apr 20 2020 at 14:33, on Zulip):

Edwin Cheng said:

I wish i could use strace or any trace in Windows :(

http://www.rohitab.com/apimonitor (or Procmon for simpler stuff)

Edwin Cheng (Apr 20 2020 at 14:34, on Zulip):

If I am using linux, who tests RA in Windows ? :)

Edwin Cheng (Apr 20 2020 at 14:34, on Zulip):

I have to use Windows for my company works

std::Veetaha (Apr 20 2020 at 14:35, on Zulip):

Bruh, just install Ubuntu alongside Widows)

Edwin Cheng (Apr 20 2020 at 14:35, on Zulip):

Edwin Cheng said:

If I am using linux, who tests RA in Windows ? :)

@Jeremy Kolb use Windows too btw

matklad (Apr 20 2020 at 14:39, on Zulip):

If I am using linux, who tests RA in Windows ? :)

This actually is hugely important. I am so glad that you are using windows and Florian Diebold parenthesis Emacs -- testing these integrations would be very hard otherwise.

std::Veetaha (Apr 20 2020 at 14:59, on Zulip):

@Edwin Cheng is this expected that we will have more ProcMacroClientKinds in future?

Edwin Cheng (Apr 20 2020 at 15:00, on Zulip):

std::Veetaha said:

Edwin Cheng is this expected that we will have more ProcMacroClientKinds in future?

wasm , hopefully

Jeremy Kolb (Apr 20 2020 at 15:11, on Zulip):

Yes I am on Windows and have check enabled. After restarting vscode I did notice a rogue rust-analyzer process lying around that I had to kill. Might be related.

Edwin Cheng (Apr 20 2020 at 15:25, on Zulip):

matklad said:

If I am using linux, who tests RA in Windows ? :)

This actually is hugely important. I am so glad that you are using windows and Florian Diebold parenthesis Emacs -- testing these integrations would be very hard otherwise.

yeah , Emacs is another important OS we have to test .

Jeremy Kolb (Apr 20 2020 at 15:27, on Zulip):

I also like how gnome-builder was recently tried as an LSP client

std::Veetaha (Apr 20 2020 at 15:46, on Zulip):

@Edwin Cheng is there a reason why list_macros returns a Result? I don't see any fallable operation in it

Edwin Cheng (Apr 20 2020 at 15:48, on Zulip):

Yes, it should return the list directly.

std::Veetaha (Apr 20 2020 at 18:11, on Zulip):

@Edwin Cheng , what about merging the ProcMacroProcessThread with ProcMacroProcessSrv? This way we also get rid of the weak pointer. As I can tell for now this struct has no logic apart from holding the jod_thread

Edwin Cheng (Apr 20 2020 at 18:17, on Zulip):

Um.. sound reasonable. I really forget why I design the structure like this.

std::Veetaha (Apr 20 2020 at 18:19, on Zulip):

Yeah, there is probably something caveat-y underneath...

Edwin Cheng (Apr 20 2020 at 18:22, on Zulip):

Ah

Edwin Cheng (Apr 20 2020 at 18:25, on Zulip):

I remember now, it is because we hold an arc pointer for each proc-macro and we might destroyed the thread but proc-macro itself still held by crates. We use the weak pointer to separate that thread and proc-macros

std::Veetaha (Apr 20 2020 at 18:27, on Zulip):

This ownership story is quite complicated. I wonder if we can avoid it.

std::Veetaha (Apr 20 2020 at 20:14, on Zulip):

@Edwin Cheng shouldn't we pass the same arguments to the proc macro process in fn restart() as we do in fn run()?

Edwin Cheng (Apr 20 2020 at 20:15, on Zulip):

Yeah, it is a bug , nice catch !

Edwin Cheng (Apr 20 2020 at 20:29, on Zulip):

I will submit a PR after https://github.com/rust-analyzer/rust-analyzer/pull/4061 is merged.

std::Veetaha (Apr 20 2020 at 20:42, on Zulip):

I also wonder if there is any data race possible. Like can't RA send several requests to proc marco srv in parallel? LSP (or JSON RPC in general, not sure) uses request ids for example...

Edwin Cheng (Apr 20 2020 at 20:43, on Zulip):

The send request is blocking from client to server.

std::Veetaha (Apr 20 2020 at 20:44, on Zulip):

Yeah, it's blocking, but can't RA send_task() in parallel threads such that both threads are blocked, but then some of them randomly can get the response it was not awaiting for.

std::Veetaha (Apr 20 2020 at 20:46, on Zulip):

Here is my imaginary workflow:

  1. RA thead foo sends request 1 and waits for the response.
  2. Proc macro srv is blocked processing this request.
  3. RA thread bar send request 2 and waits for the response.
  4. Proc macro srv writes response for request 1 to stdout.
  5. RA thread bar reads the stdout with the response for request 1 (BUG)
Edwin Cheng (Apr 20 2020 at 20:49, on Zulip):

Note that we send the task_sender into the thread channel, the thread use that channel to send back to request.

Edwin Cheng (Apr 20 2020 at 20:51, on Zulip):

And we only have a single worker thread in client side.

std::Veetaha (Apr 20 2020 at 20:52, on Zulip):

Oh, right, now I see...

std::Veetaha (Apr 20 2020 at 21:47, on Zulip):

Hmh, I think we (me) broke something, now one core is constantly 100% and no hints in the edittor appear....

std::Veetaha (Apr 20 2020 at 21:47, on Zulip):

image.png

std::Veetaha (Apr 20 2020 at 21:49, on Zulip):

Okay it just took some time and everything stabilized..

std::Veetaha (Apr 20 2020 at 21:50, on Zulip):

Don't know how, but now I don't get that "Empty response error"

Laurențiu Nicola (Apr 21 2020 at 05:37, on Zulip):

Edwin Cheng said:

And we only have a single worker thread in client side.

Wouldn't it be worth being able to support multiple concurrent expansions? I know it came from https://github.com/rust-analyzer/rust-analyzer/pull/3738#discussion_r399637997.

Edwin Cheng (Apr 21 2020 at 05:46, on Zulip):

iirc, currently RA are concurrent but not parallel, which means if there is multiple requests from lsp client, AND its changed salsa db, it will cancel former request. I think single worker thread in proc-macro client fit this design. But of course I only have vague understanding in this part.

Edwin Cheng (Apr 21 2020 at 05:48, on Zulip):

And note that if everything implemented correctly, a request to proc-macro expansion must be triggered by a new salsa db invocation, which must change the db itself.

Laurențiu Nicola (Apr 21 2020 at 05:48, on Zulip):

But a single LSP request (e.g. type hints) might expand multiple proc macros anyway

Edwin Cheng (Apr 21 2020 at 05:50, on Zulip):

Yes, but these requests must be synchronized in current design.

Laurențiu Nicola (Apr 21 2020 at 05:58, on Zulip):

Got it, thanks

Edwin Cheng (Apr 21 2020 at 06:05, on Zulip):

But there is a rare case here: two concurrent request is asking for expansion at the same time (the salsa db is not modified yet), that is the potential bug mentioned. But as I answered previously, we pass a sender channel to the worker thread for replying to prevent this bug.

Laurențiu Nicola (Apr 21 2020 at 06:46, on Zulip):

Well, I was looking at it as a potential optimization, not a bug :laughter_tears:

Laurențiu Nicola (Apr 22 2020 at 16:04, on Zulip):

std::Veetaha said:

The code:

use soa_derive::StructOfArray;

// #[macro_use]
// extern crate soa_derive;

#[derive(StructOfArray)]
pub struct Cheese {
    pub smell: f64,
    pub color: (f64, f64, f64),
    pub with_mushrooms: bool,
    pub name: String,
}

fn main() {
    let vec = CheeseVec::new();
}

Huh. I'm pretty sure it wasn't working at the time, but now I even get completions in spite of https://github.com/rust-analyzer/rust-analyzer/pull/4029:

image.png

std::Veetaha (Apr 22 2020 at 22:53, on Zulip):

Really the performance of proc_macro_srv is not very impressive, I wish we could paralellize it...

Edwin Cheng (Apr 23 2020 at 18:42, on Zulip):

@matklad I just found out why sometimes the proc-macro srv will be deadlock. It is because we load and unload dlls for each request. However, currently rustc TLS is leaky , such that if we do it a lot of time, all index will be consumed and it will be deadlocked inside panic (it is because panic itself is using thread local too).

Edwin Cheng (Apr 23 2020 at 18:44, on Zulip):

The maxium number of TLS in Windows are small (1,088), so basically it will be hit for sure..

Edwin Cheng (Apr 23 2020 at 18:47, on Zulip):

A simplest solution is we spawn a thread for each request and join it. Another solution will be we cached all dlls ..

matklad (Apr 23 2020 at 19:02, on Zulip):

I !:heart: TLS

matklad (Apr 23 2020 at 19:03, on Zulip):

@Edwin Cheng why do we need to unload dlls? I'd expect to keep all proc macros in memory all the time

matklad (Apr 23 2020 at 19:03, on Zulip):

(but it probably make sense to lazy-load them)

Edwin Cheng (Apr 23 2020 at 19:05, on Zulip):

My initial thought are saving memory for the dll, but it seems to be not a good idea now :)

bjorn3 (Apr 23 2020 at 20:10, on Zulip):

On macOS you are not supposed to unload dylibs. In fact I believe you have to call dlclose twice to actually unload it. Otherwise it will be kept open and reused the next time you use dlopen. Even if the dylib has changed since.

pksunkara (Apr 23 2020 at 21:52, on Zulip):

Can someone tell me why proc_macro support needs loadOutDirsFromCheck option enabled? I might be a noob for asking this but why are out dirs even checked for? Is it not always target?

Edwin Cheng (Apr 24 2020 at 01:20, on Zulip):

@pksunkara Although it is always target, but we dont't know the full path. Additionally, e.g. if using sccache, it might be not in target...

std::Veetaha (Apr 25 2020 at 14:07, on Zulip):

@Edwin Cheng, smol question. Am I right that we use a separate process for proc macro expanding to prevent crashing the rust-analyzer process if something really bad happens inside of a proc macro, and are there any other raesons?

Edwin Cheng (Apr 25 2020 at 14:08, on Zulip):

That's the main reason. I think

std::Veetaha (Apr 25 2020 at 14:39, on Zulip):

@Edwin Cheng I am getting this error with today's nightly, did you see it before?

thread 'main' panicked at 'Cannot create expander for /home/veetaha/dev/rust-analyzer/target/debug/deps/libserde_repr-33c31e7306fd2bdf.so: "Dynamic loading not supported"', crates/ra_proc_macro_srv/src/lib.rs:45:31
Edwin Cheng (Apr 25 2020 at 14:43, on Zulip):

No.. oh ... um...

Edwin Cheng (Apr 25 2020 at 14:43, on Zulip):

Do we compile our nightly binary with musl?

Edwin Cheng (Apr 25 2020 at 14:45, on Zulip):

https://git.musl-libc.org/cgit/musl/tree/src/ldso/dlopen.c

Edwin Cheng (Apr 25 2020 at 14:46, on Zulip):

FXXXXK

std::Veetaha (Apr 25 2020 at 14:49, on Zulip):

FXXXXK

Not sure what this means )
Hmm, this is very strange that musl doesn't support dlopen loading...

std::Veetaha (Apr 25 2020 at 14:51, on Zulip):

I guess we do compile it with musl
I also wonder why this was not appearing before, something has changed...

Edwin Cheng (Apr 25 2020 at 14:52, on Zulip):

Just some dirty words to express my angry of the mess to handle platform depends dlls and tls related stuffs.

Edwin Cheng (Apr 25 2020 at 14:53, on Zulip):

std::Veetaha said:

I guess we do compile it with musl
I also wonder why this was not appearing before, something has changed...

We don't run test with the nightly build, I think .

Laurențiu Nicola (Apr 25 2020 at 15:33, on Zulip):

Wait, so are proc macros working for anyone using the marketplace release?

Edwin Cheng (Apr 26 2020 at 09:48, on Zulip):

@Laurențiu Nicola I don't think it is working except Windows users

Laurențiu Nicola (Apr 26 2020 at 10:38, on Zulip):

Should we build the binaries on glibc?

Laurențiu Nicola (Apr 26 2020 at 10:51, on Zulip):

I wonder how can the toolchain work on an alpine host. But it does.

bjorn3 (Apr 26 2020 at 10:53, on Zulip):

If building for glibc, please build using an old system, otherwise I can't run them on my debian system, as you can't run an executable compiled for newer version of glibc on an older system.

Laurențiu Nicola (Apr 26 2020 at 10:59, on Zulip):

GitHub Actions seems to support Ubuntu 16.04. Is that old enough? But yeah, binary compatibility across distros is a mess.

matklad (Apr 26 2020 at 11:11, on Zulip):

Does it matter at all against which glibc we are building? We don’t use C
code, so I think only the version of glibc used to build rust standard
library matters?

Hm, actually, I think I am missing some large piece of knowledge here...
Why build-time version of glibc matters at all? Like, what matters is the
header files(because we link glibc dynamically), and I assume Rust just has
some extern C declarations which are fixed?

Laurențiu Nicola (Apr 26 2020 at 11:42, on Zulip):

You might have a point there. My rust-analyzer binary (built on Arch with glibc 2.31 appears to work on Ubuntu 14.04 (with glibc 2.19).

Laurențiu Nicola (Apr 26 2020 at 11:47, on Zulip):

Since Sundays is usually "merge day", let me file a PR to switch to x86_64-unknown-linux-gnu to have something exciting in the next release :sweat_smile:. Maybe we could even include https://github.com/rust-analyzer/rust-analyzer/pull/4153

matklad (Apr 26 2020 at 11:58, on Zulip):

I am prepping a big "relase" post on monday, so I'll fold this into the next release, and not this one :)

Laurențiu Nicola (Apr 28 2020 at 17:32, on Zulip):

@matklad I realize you're busy, but what do you say about merging https://github.com/rust-analyzer/rust-analyzer/pull/4157 soon-ish to have it bake before Monday? It also resolves https://github.com/rust-analyzer/rust-analyzer/pull/4143, but on the other hand it breaks bors on every open PR :sad:.

matklad (Apr 28 2020 at 19:41, on Zulip):

that's a fun thread: https://users.rust-lang.org/t/loading-dynamic-libraries-from-memory/41697

Edwin Cheng (Apr 29 2020 at 06:00, on Zulip):

Nice, I hope someone will publish a crate for that <3.

And here are some other funny and (non practical) solutions (projects):

Laurențiu Nicola (Apr 29 2020 at 06:08, on Zulip):

Is anyone using nightly instead of source builds on Linux? I wonder if they still work.

Laurențiu Nicola (Apr 29 2020 at 06:08, on Zulip):

Edwin Cheng said:

Well, there's miri...

matklad (Apr 29 2020 at 06:21, on Zulip):

x86towasm sounds like a name of a project which should exist :D

Last update: May 29 2020 at 17:50UTC