compiletest: may truncate compiler output before trying to parse it as json

[nagisa]

Running a ui test will eventually invoke the following function:

rust/src/tools/compiletest/src/read2.rs

Lines 48 to 57 in 4ca19e0

	fn into_bytes(self) -> Vec<u8> {
	match self {
	ProcOutput::Full(bytes) => bytes,
	ProcOutput::Abbreviated { mut head, skipped, tail } => {
	write!(&mut head, "\n\n<<<<<< SKIPPED {} BYTES >>>>>>\n\n", skipped).unwrap();
	head.extend_from_slice(&tail);
	head
	}
	}
	}

through

rust/src/tools/compiletest/src/runtest.rs

Lines 1772 to 1780 in 4ca19e0

	let Output { status, stdout, stderr } =
	read2_abbreviated(child).expect("failed to read output");
	let result = ProcRes {
	status,
	stdout: String::from_utf8_lossy(&stdout).into_owned(),
	stderr: String::from_utf8_lossy(&stderr).into_owned(),
	cmdline,
	};

which can truncate a long output from the compiler. The problem is that UI tests ask the compiler to output its output as json and then attempts to parse it later, for example here:

rust/src/tools/compiletest/src/runtest.rs

Line 3114 in 4ca19e0

let rustfix_input = json::rustfix_diagnostics_only(&proc_res.stderr);

which can fail due to truncation and output very difficult to investigate output in e.g. CI.

Reference: https://rust-lang.zulipchat.com/#narrow/stream/187780-t-compiler.2Fwg-llvm/topic/Legacy.20PM.20removal/near/279479951

[ehuss]

I think this is somewhat a duplicate of #94322 and #92211.

It might be good to consolidate these issues, and come up with a specific suggestion on how to improve it. The limit needs to be there to prevent OOM, and also as a indicator that perhaps rustc is emitting too much data (as #94327 resolved). So I suspect what is desired is a better way to display the error. I haven't thought about it myself, but I think that should be easy to improve.

[nagisa]

Got it, removed E-easy.

I feel like at least a part of this problem is in the fact that the truncation happens indiscriminately at a byte boundary. And another part is that the failure mode is non-deterministic (paths and such can have varying lengths.)

If the intent is to make “too much output” a failure mode, then we should just straight up fail the test, I feel. Possibly kill/SIGPIPE the compilation process when it exceeds the amount of output bytes but don't truncate the output itself when showing it to the user…?

Alternatively, for JSONL(ines) truncating on line boundaries would make things not fail as terribly. Not to mention, the rendered output is duplicated among other information with JSON output and a test will likely be interested in only parts of it, which will make truncation kick in earlier in some instances than others, so maybe giving an opportunity to map the output in a streaming fashion would help?

[pietroalbini]

Also hit this due to #96362 (comment) on my local machine...

[pietroalbini]

Opened a PR to remove the nondeterminism based on the length of the checkout path, which should at least alleviate the problem (abbreviations would still happen, but at least they'd show up on CI): #96551