`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many #122840

GuillaumeGomez · 2024-03-21T20:14:31Z

Thanks to this I discovered that rust was using @ to add arguments from a file, quite convenient.

If there are too many cfg arguments given to rustdoc --test, it'll now put them into a temporary file and passing it as argument to the rustc command.

I added a test with 100_000 cfg arguments to ensure it'll not break again.

r? @Notrid

rustbot · 2024-03-21T20:14:38Z

Failed to set assignee to notrid: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

rustbot · 2024-03-21T20:14:40Z

Some changes occurred in run-make tests.

cc @jieyouxu

GuillaumeGomez · 2024-03-21T20:14:50Z

Github auto-complete being broken is very annoying...

r? @notriddle

Urgau · 2024-03-21T20:36:00Z

src/librustdoc/doctest.rs

+
+    // If there are too many `cfg` arguments, instead of risking reaching `Command`'s limit on
+    // the number of arguments, we put them into a file which we then pass as `@` argument.
+    if rustdoc_options.cfgs.len() + rustdoc_options.check_cfgs.len() > 1_000


Thing can become unreadable and have a perf impact way lower, I suggest switching at 50. It should be enough to handle most crates without the argfile while switch sooner rather than latter for bigger crate.

Suggested change

if rustdoc_options.cfgs.len() + rustdoc_options.check_cfgs.len() > 1_000

if rustdoc_options.cfgs.len() + rustdoc_options.check_cfgs.len() > 50

Also, note that check-cfg options can be very long, in particular Cargo put all the features in the same argument. So it would probably be better to compute the length of all the strings instead of just the number of strings.

Yeah, some operating systems may have low limits on the number of arguments and/or total length, so it is best to be conservative.

At this point I think I'll simply put all arguments into a file directly and pass it to all doctests. :)

tests/run-make/rustdoc-test-args/Makefile

Urgau · 2024-03-21T20:45:26Z

While this is certainly a solution for --cfg and --check-cfg, it isn't a general solution to the issue, which would be to retry the command when it fails by putting all the args into a argfile.

Cargo utilize that mechanism internaly, ProcessBuilder::retry_with_argfile

notriddle · 2024-03-22T00:48:54Z

I think this fix is too specific to a single problem. --cfg is not the only repeatable option, so it's not the only thing that would benefit. extern_strs and lib_strs, just from a cursory glance through the file, also also produced by loops, so they could also be too big.

I would prefer if the file was restructured to create a Vec of every¹ CLI option, then check if it's too big, and generate an arg file if it is. That way, no matter which CLI option causes the problem, they'll all be passed by file and avoid the problem.

If there's an option that you specifically want to pass on the CLI and know won't cause a problem, you can exempt it and add a comment explaining why, of course. The point is that there are a few options that look like they could cause this problem that aren't going through an arg file. ↩

GuillaumeGomez · 2024-03-22T09:54:12Z

I was planning to generalize the creation of a file containing all arguments instead of checking if there are too many of them. Will make things simpler.

One thing I wonder though is how I will be able to test it...

GuillaumeGomez · 2024-03-22T13:49:33Z

I moved more arguments into the arg file and even better: it allows to prevent recomputing some arguments for each doctest by passing the same arg file to all doctests (since it's arguments they all share), which will likely improve doctest performance nicely.

I took the opportunity to also stop cloning RustdocOptions completely and instead only clone what I need and move it into a new RustdocTestOptions type.

Urgau

Looks pretty good. I would still like to see a rmake.rs test¹ with at least 50000 different cfgs, it won't be perfect but it should at least validate RfL usage.

if our still encountering issue with tmp dir not being writtable, I suggest using the $(TMPDIR) env var. ↩

src/librustdoc/doctest.rs

…y using files for arguments if there are too many

rustbot · 2024-03-23T14:48:52Z

The run-make-support library was changed

cc @jieyouxu

GuillaumeGomez · 2024-03-23T14:49:27Z

Implemented all suggestions and added back the run-make test using rmake.rs this time. Hopefully it'll work in the CI this time. :)

notriddle · 2024-03-23T16:21:28Z

@bors r+

bors · 2024-03-23T16:21:30Z

📌 Commit 08af772 has been approved by notriddle

It is now in the queue for this repository.

Urgau

Looks good to me.

raced with @notriddle 😄

tests/run-make/rustdoc-test-args/rmake.rs

src/tools/run-make-support/src/lib.rs

ojeda · 2024-03-23T18:20:25Z

with at least 50000 different cfgs, it won't be perfect but it should at least validate RfL usage.

I am not sure it does, e.g. with very small --cfg flags like the one in the test now (A), 50000 or even 75000 do not hit the limit for me with my reproducer. I would suggest something substantially bigger.

Rollup merge of rust-lang#122840 - GuillaumeGomez:rustdoc-test-too-many-args, r=notriddle,Urgau,jieyouxu `rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many Fixes rust-lang#122722. Thanks to this I discovered that rust was using ``@`` to add arguments from a file, quite convenient. If there are too many `cfg` arguments given to `rustdoc --test`, it'll now put them into a temporary file and passing it as argument to the rustc command. I added a test with 100_000 `cfg` arguments to ensure it'll not break again. r? `@notrid`

RalfJung · 2024-03-24T09:45:10Z

I think this is causing issues in Miri... rust-lang/miri#3404

GuillaumeGomez · 2024-03-24T10:21:18Z

Please open an issue and tag me on it. I'll try to take a look in the week (rust nation conf so maybe not very fast...).

…ny-args, r=notriddle,Urgau,jieyouxu `rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many Fixes rust-lang#122722. Thanks to this I discovered that rust was using ``@`` to add arguments from a file, quite convenient. If there are too many `cfg` arguments given to `rustdoc --test`, it'll now put them into a temporary file and passing it as argument to the rustc command. I added a test with 100_000 `cfg` arguments to ensure it'll not break again. r? `@notrid`

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#121281 (regression test for rust-lang#103626) - rust-lang#122168 (Fix validation on substituted callee bodies in MIR inliner) - rust-lang#122217 (Handle str literals written with `'` lexed as lifetime) - rust-lang#122379 (transmute: caution against int2ptr transmutation) - rust-lang#122840 (`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many) - rust-lang#122907 (Uniquify `ReError` on input mode in canonicalizer) - rust-lang#122942 (Add test in higher ranked subtype) - rust-lang#122943 (add a couple more ice tests) - rust-lang#122963 (core/panicking: fix outdated comment) r? `@ghost` `@rustbot` modify labels: rollup

When KUnit tests are enabled, under very big kernel configurations (e.g. `allyesconfig`), we may trigger an ICE in `rustdoc` [1]: RUSTDOC TK rust/kernel/lib.rs error: the compiler unexpectedly panicked. this is a bug. The reason is that this build step has a duplicated `@rustc_cfg` argument, which contains the kernel configuration, and thus a lot of arguments. The factor 2 happens to be enough to reach the ICE. Thus remove the unneeded `@rustc_cfg`, workarounding the ICE and cleaning up the command. The ICE has been fixed for the upcoming Rust 1.79 [2]. Cc: [email protected] Fixes: a66d733 ("rust: support running Rust documentation tests as KUnit ones") Link: rust-lang/rust#122722 [1] Link: rust-lang/rust#122840 [2] Signed-off-by: Miguel Ojeda <[email protected]>

When KUnit tests are enabled, under very big kernel configurations (e.g. `allyesconfig`), we can trigger a `rustdoc` ICE [1]: RUSTDOC TK rust/kernel/lib.rs error: the compiler unexpectedly panicked. this is a bug. The reason is that this build step has a duplicated `@rustc_cfg` argument, which contains the kernel configuration, and thus a lot of arguments. The factor 2 happens to be enough to reach the ICE. Thus remove the unneeded `@rustc_cfg`. By doing so, we clean up the command and workaround the ICE. The ICE has been fixed in the upcoming Rust 1.79 [2]. Cc: [email protected] Fixes: a66d733 ("rust: support running Rust documentation tests as KUnit ones") Link: rust-lang/rust#122722 [1] Link: rust-lang/rust#122840 [2] Reviewed-by: Alice Ryhl <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]>

commit 50cfe93 upstream. When KUnit tests are enabled, under very big kernel configurations (e.g. `allyesconfig`), we can trigger a `rustdoc` ICE [1]: RUSTDOC TK rust/kernel/lib.rs error: the compiler unexpectedly panicked. this is a bug. The reason is that this build step has a duplicated `@rustc_cfg` argument, which contains the kernel configuration, and thus a lot of arguments. The factor 2 happens to be enough to reach the ICE. Thus remove the unneeded `@rustc_cfg`. By doing so, we clean up the command and workaround the ICE. The ICE has been fixed in the upcoming Rust 1.79 [2]. Cc: [email protected] Fixes: a66d733 ("rust: support running Rust documentation tests as KUnit ones") Link: rust-lang/rust#122722 [1] Link: rust-lang/rust#122840 [2] Reviewed-by: Alice Ryhl <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>

stable inclusion from stable-v6.6.30 commit 2eed4381ee41759f29adc7e235b91e8978f8e948 bugzilla: https://gitee.com/openeuler/kernel/issues/I9MPZ8 Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=2eed4381ee41759f29adc7e235b91e8978f8e948 -------------------------------- commit 50cfe93b01475ba36878b65d35d812e1bb48ac71 upstream. When KUnit tests are enabled, under very big kernel configurations (e.g. `allyesconfig`), we can trigger a `rustdoc` ICE [1]: RUSTDOC TK rust/kernel/lib.rs error: the compiler unexpectedly panicked. this is a bug. The reason is that this build step has a duplicated `@rustc_cfg` argument, which contains the kernel configuration, and thus a lot of arguments. The factor 2 happens to be enough to reach the ICE. Thus remove the unneeded `@rustc_cfg`. By doing so, we clean up the command and workaround the ICE. The ICE has been fixed in the upcoming Rust 1.79 [2]. Cc: [email protected] Fixes: a66d733 ("rust: support running Rust documentation tests as KUnit ones") Link: rust-lang/rust#122722 [1] Link: rust-lang/rust#122840 [2] Reviewed-by: Alice Ryhl <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]> Signed-off-by: ZhangPeng <[email protected]>

BugLink: https://bugs.launchpad.net/bugs/2070337 commit 50cfe93b01475ba36878b65d35d812e1bb48ac71 upstream. When KUnit tests are enabled, under very big kernel configurations (e.g. `allyesconfig`), we can trigger a `rustdoc` ICE [1]: RUSTDOC TK rust/kernel/lib.rs error: the compiler unexpectedly panicked. this is a bug. The reason is that this build step has a duplicated `@rustc_cfg` argument, which contains the kernel configuration, and thus a lot of arguments. The factor 2 happens to be enough to reach the ICE. Thus remove the unneeded `@rustc_cfg`. By doing so, we clean up the command and workaround the ICE. The ICE has been fixed in the upcoming Rust 1.79 [2]. Cc: [email protected] Fixes: a66d733 ("rust: support running Rust documentation tests as KUnit ones") Link: rust-lang/rust#122722 [1] Link: rust-lang/rust#122840 [2] Reviewed-by: Alice Ryhl <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]> Signed-off-by: Manuel Diewald <[email protected]>

BugLink: https://bugs.launchpad.net/bugs/2070337 commit 50cfe93b01475ba36878b65d35d812e1bb48ac71 upstream. When KUnit tests are enabled, under very big kernel configurations (e.g. `allyesconfig`), we can trigger a `rustdoc` ICE [1]: RUSTDOC TK rust/kernel/lib.rs error: the compiler unexpectedly panicked. this is a bug. The reason is that this build step has a duplicated `@rustc_cfg` argument, which contains the kernel configuration, and thus a lot of arguments. The factor 2 happens to be enough to reach the ICE. Thus remove the unneeded `@rustc_cfg`. By doing so, we clean up the command and workaround the ICE. The ICE has been fixed in the upcoming Rust 1.79 [2]. Cc: [email protected] Fixes: a66d733 ("rust: support running Rust documentation tests as KUnit ones") Link: rust-lang/rust#122722 [1] Link: rust-lang/rust#122840 [2] Reviewed-by: Alice Ryhl <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Miguel Ojeda <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]> Signed-off-by: Manuel Diewald <[email protected]> Signed-off-by: Stefan Bader <[email protected]>

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 21, 2024

rustbot assigned notriddle Mar 21, 2024

ojeda mentioned this pull request Mar 21, 2024

rustdoc wanted features & bugfixes Rust-for-Linux/linux#350

Open

17 tasks

Urgau reviewed Mar 21, 2024

View reviewed changes

ojeda reviewed Mar 21, 2024

View reviewed changes

tests/run-make/rustdoc-test-args/Makefile Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the rustdoc-test-too-many-args branch from c5dc4c7 to 7776678 Compare March 22, 2024 13:48

Urgau reviewed Mar 23, 2024

View reviewed changes

src/librustdoc/doctest.rs Outdated Show resolved Hide resolved

src/librustdoc/doctest.rs Outdated Show resolved Hide resolved

GuillaumeGomez added 4 commits March 23, 2024 14:42

rustdoc --test: Prevent reaching the maximum size of command-line b…

0d3ef80

…y using files for arguments if there are too many

Prevent cloning more than necessary RustdocOptions fields

33735d5

Put temporary directory into one common function

86fe400

Rename RustdocTestOptions into IndividualTestOptions

773084f

GuillaumeGomez force-pushed the rustdoc-test-too-many-args branch from 7776678 to 08af772 Compare March 23, 2024 14:48

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2024

Urgau approved these changes Mar 23, 2024

View reviewed changes

tests/run-make/rustdoc-test-args/rmake.rs Outdated Show resolved Hide resolved

tests/run-make/rustdoc-test-args/rmake.rs Outdated Show resolved Hide resolved

jieyouxu reviewed Mar 23, 2024

View reviewed changes

src/tools/run-make-support/src/lib.rs Outdated Show resolved Hide resolved

This was referenced Mar 24, 2024

Automatic Rustup rust-lang/miri#3401

Closed

cargo miri test is broken for doctests (Option 'sysroot' given more than once) rust-lang/miri#3404

Closed

GuillaumeGomez deleted the rustdoc-test-too-many-args branch March 24, 2024 10:20

jieyouxu mentioned this pull request Mar 24, 2024

Add tidy check to error on new Makefiles in tests/run-make #122898

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many #122840

`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many #122840

GuillaumeGomez commented Mar 21, 2024

rustbot commented Mar 21, 2024

rustbot commented Mar 21, 2024

GuillaumeGomez commented Mar 21, 2024

Urgau Mar 21, 2024

Urgau Mar 21, 2024

ojeda Mar 21, 2024

GuillaumeGomez Mar 22, 2024

Urgau commented Mar 21, 2024

This comment has been minimized.

notriddle commented Mar 22, 2024

GuillaumeGomez commented Mar 22, 2024

GuillaumeGomez commented Mar 22, 2024 •

edited

Loading

Urgau left a comment

rustbot commented Mar 23, 2024

GuillaumeGomez commented Mar 23, 2024

notriddle commented Mar 23, 2024

bors commented Mar 23, 2024

Urgau left a comment •

edited

Loading

ojeda commented Mar 23, 2024

RalfJung commented Mar 24, 2024 •

edited

Loading

GuillaumeGomez commented Mar 24, 2024

	if rustdoc_options.cfgs.len() + rustdoc_options.check_cfgs.len() > 1_000
	if rustdoc_options.cfgs.len() + rustdoc_options.check_cfgs.len() > 50

rustdoc --test: Prevent reaching the maximum size of command-line by using files for arguments if there are too many #122840

rustdoc --test: Prevent reaching the maximum size of command-line by using files for arguments if there are too many #122840

Conversation

GuillaumeGomez commented Mar 21, 2024

rustbot commented Mar 21, 2024

rustbot commented Mar 21, 2024

GuillaumeGomez commented Mar 21, 2024

Urgau Mar 21, 2024

Choose a reason for hiding this comment

Urgau Mar 21, 2024

Choose a reason for hiding this comment

ojeda Mar 21, 2024

Choose a reason for hiding this comment

GuillaumeGomez Mar 22, 2024

Choose a reason for hiding this comment

Urgau commented Mar 21, 2024

This comment has been minimized.

notriddle commented Mar 22, 2024

Footnotes

GuillaumeGomez commented Mar 22, 2024

GuillaumeGomez commented Mar 22, 2024 • edited Loading

Urgau left a comment

Choose a reason for hiding this comment

Footnotes

rustbot commented Mar 23, 2024

GuillaumeGomez commented Mar 23, 2024

notriddle commented Mar 23, 2024

bors commented Mar 23, 2024

Urgau left a comment • edited Loading

Choose a reason for hiding this comment

ojeda commented Mar 23, 2024

RalfJung commented Mar 24, 2024 • edited Loading

GuillaumeGomez commented Mar 24, 2024

`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many #122840

`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many #122840

GuillaumeGomez commented Mar 22, 2024 •

edited

Loading

Urgau left a comment •

edited

Loading

RalfJung commented Mar 24, 2024 •

edited

Loading