`module!` improvements #278

nbdd0121 · 2021-05-19T19:39:15Z

This is a large PR. It's best reviewed by commits.

commit Rename libmodule to libmacros

Renaming to libmacros allows us to potentially have more procedural macros in the future without introducing additional crates.

commit Vendor literal parsing from syn crate
commit accept str instead of bstr in module!

Fixes #252. Doing so requires capability to parse strings so I vendored in the lit mod from syn.

commit Vendor buffer from syn crate
commit module macro cleanup using buffer

Some cleanup with buffer mod vendored from syn. TokenStream itself is very hard to use for parsing, because it does not provide lookahead capability. With the buffer it can be simplified a lot.

commit cleanup modinfo generation in module!
commit separate parse and codegen in module!

These organizes the code so that parsing and codegen are split into two phased. This allows the module_misc_device macro to do codegen directly, so it does not have to re-generate the module! invocation.

commit multiple author/alias support in module!

Fixes #244, fixes #246. Becomes a very simple change with all the cleanups made.

commit allow any constexpr to be used as param default

With syn's buffer's lookahead capability this also becomes a trivial change.

ojeda · 2021-05-19T19:59:58Z

rust/macros/syn/lit.rs

@@ -0,0 +1,401 @@
+use proc_macro::{Literal, Span};


Missing SPDX.

ojeda · 2021-05-19T20:01:49Z

rust/macros/syn/mod.rs

+//! Shared parsing functions for use in procedural macros. These functions are
+//! from [syn](https://github.com/dtolnay/syn).


Suggested change

//! Shared parsing functions for use in procedural macros. These functions are

//! from [syn](https://github.com/dtolnay/syn).

//! Shared parsing functions for use in procedural macros.

//!

//! These functions come from the [`syn`](https://github.com/dtolnay/syn) crate.

Are the files too different from the original ones? If not, perhaps recording in the comment the Git tag from where they were checked out would be nice. If yes, doing it in the commit message is fair.

I does make a few changes to strip away anything that depends on macros, bigint and unicode-xid. But I agree it's better to also record version number in the comment.

That's great, thanks for removing those bits.

Also mentioning that in the comment would be nice too (i.e. so that people doing future updates, if any, are aware of that and do not add bigint etc. back).

ojeda · 2021-05-19T20:22:40Z

Looks great, as usual!

GitHub does not allow to comment on the commit messages themselves :( So I put those here:

It would be best if you move the explanations you have written in the PR body above into the commit messages, i.e. please explain the "why" in the commit messages themselves.
For the separation of codegen and parsing, it would be nice to say why avoiding a macro invocation is better, e.g. is it faster? or is it "only" for cleanliness of the code? (do not get me wrong: doing cleanups is great nevertheless!).
A super trivial nit: in one of the titles you used backticks for e.g. module! but not for str or bstr. I would do it for all of them (there are other titles like that -- I noticed because that was in the same one! :)

(I also really miss the "suggestion" feature in the commit-by-commit view -- if someone knows how to easily do it without having to move to the "changes" view, please let me know!).

Renaming to libmacros allows us to potentially have more procedural macros in the future without introducing additional crates. Signed-off-by: Gary Guo <[email protected]>

`module!` macro currently have some ad-hoc literal parsing. `syn` crate provides some proper parsing for us to use. Signed-off-by: Gary Guo <[email protected]>

Signed-off-by: Gary Guo <[email protected]>

`TokenStream` itself is very hard to use for parsing, as it does not provide lookahead capability. `syn`'s buffer will provide cleanup opportunity. Signed-off-by: Gary Guo <[email protected]>

Make use of `syn` buffer's lookahead capability to remove `try_*` parsing functions. Signed-off-by: Gary Guo <[email protected]>

Signed-off-by: Gary Guo <[email protected]>

Doing so allows `module_misc_device!` to do codegen directly instead of generating a `module!` invocation. Generating `module!` invocation is error-prone and would require significant effort when features are added to `module!` macro or when syntax is changed. Signed-off-by: Gary Guo <[email protected]>

Signed-off-by: Gary Guo <[email protected]>

ksquirrel · 2021-05-26T02:31:44Z

Review of 6f7f4b324c0f:

⚠️ This PR changes more than 20 files.
⚠️ This PR adds/deletes more than 500 lines.
✔️ Commit 0748450: Looks fine!
✔️ Commit fd4ef65: Looks fine!
✔️ Commit ab4c335: Looks fine!
✔️ Commit fc7c60f: Looks fine!
✔️ Commit c677a79: Looks fine!
✔️ Commit c10abf5: Looks fine!
✔️ Commit 5598904: Looks fine!
✔️ Commit 38e0f28: Looks fine!
✔️ Commit 6f7f4b3: Looks fine!

ojeda · 2021-05-26T19:09:21Z

Perhaps let's take the chance of the conflict to split this into smaller PRs.

The first commit can go in its own PR if others agree on having all procedural macros in a single crate. It would be best to put the pros/cons of that in the commit message.

nbdd0121 · 2021-05-26T19:18:47Z

I couldn't think of any pros for having each procedural macro in their own crate.

ojeda · 2021-05-26T19:22:08Z

Hmm... faster re-compilation on changes? Perhaps compilation speed overall since it can be parallelized? (because otherwise we use codegen-units 1)

What about the pros of a single crate? Easier Makefile, I guess.

bjorn3 · 2021-05-26T19:26:20Z

Using -Ccodegen-units=1 for proc macros doesn't have any benefit. They are not performance critical. At most compiling them with optimizations enabled could improve compilation speed for users of the proc macro, but the couple of percent speedup that -Ccodegen-unit=1 gives is negligible compared to the rest of the time spent in rustc and the default local thin lto (thin lto with only cgus from the current crate participating rather than all object files) used when optimizations are enabled already gives most of the wins that using a single codegen unit would give. By the way is incremental compilation supported as option for development?

ojeda · 2021-05-26T19:29:10Z

Using -Ccodegen-units=1 for proc macros doesn't have any benefit

Of course, but that is how the flags are setup at the moment; i.e. I was explaining why having split crates would increase compilation speed in our current setup (because if a reader does not know about -Ccodegen-units=1 being used, they would wonder otherwise).

ojeda · 2021-05-26T19:35:05Z

To be clear: I am happy if we decide to split the flags for proc macros if we end up with a big macros crate.

When both bpf_spin_lock and bpf_timer are present in a BPF map value, copy_map_value needs to skirt both objects when copying a value into and out of the map. However, the current code does not set both s_off and t_off in copy_map_value, which leads to a crash when e.g. bpf_spin_lock is placed in map value with bpf_timer, as bpf_map_update_elem call will be able to overwrite the other timer object. When the issue is not fixed, an overwriting can produce the following splat: [root@(none) bpf]# ./test_progs -t timer_crash [ 15.930339] bpf_testmod: loading out-of-tree module taints kernel. [ 16.037849] ================================================================== [ 16.038458] BUG: KASAN: user-memory-access in __pv_queued_spin_lock_slowpath+0x32b/0x520 [ 16.038944] Write of size 8 at addr 0000000000043ec0 by task test_progs/325 [ 16.039399] [ 16.039514] CPU: 0 PID: 325 Comm: test_progs Tainted: G OE 5.16.0+ #278 [ 16.039983] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ArchLinux 1.15.0-1 04/01/2014 [ 16.040485] Call Trace: [ 16.040645] <TASK> [ 16.040805] dump_stack_lvl+0x59/0x73 [ 16.041069] ? __pv_queued_spin_lock_slowpath+0x32b/0x520 [ 16.041427] kasan_report.cold+0x116/0x11b [ 16.041673] ? __pv_queued_spin_lock_slowpath+0x32b/0x520 [ 16.042040] __pv_queued_spin_lock_slowpath+0x32b/0x520 [ 16.042328] ? memcpy+0x39/0x60 [ 16.042552] ? pv_hash+0xd0/0xd0 [ 16.042785] ? lockdep_hardirqs_off+0x95/0xd0 [ 16.043079] __bpf_spin_lock_irqsave+0xdf/0xf0 [ 16.043366] ? bpf_get_current_comm+0x50/0x50 [ 16.043608] ? jhash+0x11a/0x270 [ 16.043848] bpf_timer_cancel+0x34/0xe0 [ 16.044119] bpf_prog_c4ea1c0f7449940d_sys_enter+0x7c/0x81 [ 16.044500] bpf_trampoline_6442477838_0+0x36/0x1000 [ 16.044836] __x64_sys_nanosleep+0x5/0x140 [ 16.045119] do_syscall_64+0x59/0x80 [ 16.045377] ? lock_is_held_type+0xe4/0x140 [ 16.045670] ? irqentry_exit_to_user_mode+0xa/0x40 [ 16.046001] ? mark_held_locks+0x24/0x90 [ 16.046287] ? asm_exc_page_fault+0x1e/0x30 [ 16.046569] ? asm_exc_page_fault+0x8/0x30 [ 16.046851] ? lockdep_hardirqs_on+0x7e/0x100 [ 16.047137] entry_SYSCALL_64_after_hwframe+0x44/0xae [ 16.047405] RIP: 0033:0x7f9e4831718d [ 16.047602] Code: b4 0c 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b3 6c 0c 00 f7 d8 64 89 01 48 [ 16.048764] RSP: 002b:00007fff488086b8 EFLAGS: 00000206 ORIG_RAX: 0000000000000023 [ 16.049275] RAX: ffffffffffffffda RBX: 00007f9e48683740 RCX: 00007f9e4831718d [ 16.049747] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00007fff488086d0 [ 16.050225] RBP: 00007fff488086f0 R08: 00007fff488085d7 R09: 00007f9e4cb594a0 [ 16.050648] R10: 0000000000000000 R11: 0000000000000206 R12: 00007f9e484cde30 [ 16.051124] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000 [ 16.051608] </TASK> [ 16.051762] ================================================================== Fixes: 6813466 ("bpf: Add map side support for bpf timers.") Signed-off-by: Kumar Kartikeya Dwivedi <[email protected]> Signed-off-by: Alexei Starovoitov <[email protected]> Link: https://lore.kernel.org/bpf/[email protected]

This comment has been minimized.

Sign in to view

ojeda reviewed May 19, 2021

View reviewed changes

nbdd0121 force-pushed the module branch from acceed7 to 6e6a5a1 Compare May 20, 2021 23:53

This comment has been minimized.

Sign in to view

nbdd0121 added 9 commits May 26, 2021 03:31

rust: Rename libmodule to libmacros

0748450

Renaming to libmacros allows us to potentially have more procedural macros in the future without introducing additional crates. Signed-off-by: Gary Guo <[email protected]>

rust: Vendor literal parsing from syn crate

fd4ef65

`module!` macro currently have some ad-hoc literal parsing. `syn` crate provides some proper parsing for us to use. Signed-off-by: Gary Guo <[email protected]>

rust: accept str instead of bstr in module!

ab4c335

Signed-off-by: Gary Guo <[email protected]>

rust: Vendor buffer from syn crate

fc7c60f

`TokenStream` itself is very hard to use for parsing, as it does not provide lookahead capability. `syn`'s buffer will provide cleanup opportunity. Signed-off-by: Gary Guo <[email protected]>

rust: module! macro cleanup using buffer

c677a79

Make use of `syn` buffer's lookahead capability to remove `try_*` parsing functions. Signed-off-by: Gary Guo <[email protected]>

rust: cleanup modinfo generation in module!

c10abf5

Signed-off-by: Gary Guo <[email protected]>

rust: multiple author/alias support in module!

38e0f28

Signed-off-by: Gary Guo <[email protected]>

rust: allow any constexpr to be used as param default

6f7f4b3

Signed-off-by: Gary Guo <[email protected]>

nbdd0121 force-pushed the module branch from 6e6a5a1 to 6f7f4b3 Compare May 26, 2021 02:31

This was referenced May 26, 2021

Depend on syn #302

Closed

Rename libmodule to libmacros #306

Merged

nbdd0121 mentioned this pull request Jun 10, 2021

cleanup modinfo generation in module! #373

Merged

nbdd0121 mentioned this pull request Jul 2, 2021

rust: separate parse and codegen in module! #423

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`module!` improvements #278

`module!` improvements #278

nbdd0121 commented May 19, 2021 •

edited

Loading

This comment has been minimized.

ojeda May 19, 2021

ojeda May 19, 2021

nbdd0121 May 19, 2021

ojeda May 19, 2021

ojeda May 19, 2021

ojeda commented May 19, 2021 •

edited

Loading

This comment has been minimized.

ksquirrel commented May 26, 2021

ojeda commented May 26, 2021

nbdd0121 commented May 26, 2021

ojeda commented May 26, 2021

bjorn3 commented May 26, 2021 •

edited

Loading

ojeda commented May 26, 2021 •

edited

Loading

ojeda commented May 26, 2021

		//! Shared parsing functions for use in procedural macros. These functions are
		//! from [syn](https://github.com/dtolnay/syn).

module! improvements #278

Are you sure you want to change the base?

module! improvements #278

Conversation

nbdd0121 commented May 19, 2021 • edited Loading

This comment has been minimized.

ojeda May 19, 2021

Choose a reason for hiding this comment

ojeda May 19, 2021

Choose a reason for hiding this comment

nbdd0121 May 19, 2021

Choose a reason for hiding this comment

ojeda May 19, 2021

Choose a reason for hiding this comment

ojeda May 19, 2021

Choose a reason for hiding this comment

ojeda commented May 19, 2021 • edited Loading

This comment has been minimized.

ksquirrel commented May 26, 2021

ojeda commented May 26, 2021

nbdd0121 commented May 26, 2021

ojeda commented May 26, 2021

bjorn3 commented May 26, 2021 • edited Loading

ojeda commented May 26, 2021 • edited Loading

ojeda commented May 26, 2021

`module!` improvements #278

`module!` improvements #278

nbdd0121 commented May 19, 2021 •

edited

Loading

ojeda commented May 19, 2021 •

edited

Loading

bjorn3 commented May 26, 2021 •

edited

Loading

ojeda commented May 26, 2021 •

edited

Loading