Make compressed rmeta contain compressed data length after header

[CraftSpider]

Fixes #90056, which is caused by link.exe introducing padding to the .rustc section, since it assumes this will have no effect besides allowing it to possibly use the extra space in future links.

[rust-highfive]

r? @fee1-dead

(rust-highfive has picked a reviewer for you, use r? to override)

[fee1-dead]

r? rust-lang/compiler

[ChrisDenton]

Can rust not be made more resilient to zero padding in .rustc so that it does indeed have no effect? Fighting linker expectations feels wrong even if there's a workaround.

[oli-obk]

r? @ehuss (commented on the issue)

[CraftSpider]

It may be possible to do that, but I'm not sure it's the best path forwards - I imagine it would effectively be removing some sanity checks on the length of the compressed data, and instead just decompressing the first N bytes. This is only a guess though, as I'm not actually incredibly familiar with how rustc does its compressed metadata writing.

[CraftSpider]

I also feel I should note that, as far as I'm aware, either only the Windows linker does this, or we already tell other linkers not to. I wouldn't consider this change to be working against linker expectations in general, just disabling a slightly odd behavior on Windows (I wouldn't think adding extra null bytes to unrecognized sections is, in general, a safe thing to do)

[bjorn3]

Maybe only disable it for the dylib and proc-macro crate types? bin and cdylib don't have crate metadata.

[bjorn3]

On the other hand there are crates which use custom sections that will likely break with padding too. For example inventory.

[ehuss]

I am almost certainly not the right person to review this. I do not know what the implications are of disabling incremental linking. Is there anyone on the compiler team familiar with linking (link.exe especifically) and rust's metadata?

I also wonder if a better solution would be to track the length of the compressed data instead of assuming the section data length is correct? The METADATA_HEADER already lives outside of the compressed stream. It could be extended to include the metadata length (it already has a position offset which gets filled in after encoding).

It also seems a little surprising that the linker is padding a section. I wonder if it is related to this:

The size of this data in the file is indicated by the SizeOfRawData field. If SizeOfRawData is less than VirtualSize, the remainder is padded with zeros.

Perhaps they always feel at liberty to pad things?

[bjorn3]

The METADATA_HEADER already lives outside of the compressed stream. It could be extended to include the metadata length (it already has a position offset which gets filled in after encoding).

Only the literal represented by METADATA_HEADER (rust\0x00\0x00\0x00\0x06) has a copy outside of the compressed stream. No other part of the header is outside of the compressed stream. That includes the crate root offset and the rustc version. Both are inside the compressed stream after another copy of METADATA_HEADER.

rust/compiler/rustc_codegen_ssa/src/back/metadata.rs

Lines 281 to 282 in addacb5

	let mut compressed = rustc_metadata::METADATA_HEADER.to_vec();
	FrameEncoder::new(&mut compressed).write_all(metadata.raw_data()).unwrap();

Hex dump of section '.rustc':
  0x00000000 72757374 00000006 ff060000 734e6150 rust........sNaP
  0x00000010 7059000d 9f00e4dc e1798080 04307275 pY.......y...0ru

[ehuss]

Ah, I misread that about the position. But I assume those lines could be changed to something like:

let mut compressed = rustc_metadata::METADATA_HEADER.to_vec(); 
compressed.write_all(length).unwrap();
FrameEncoder::new(&mut compressed).write_all(metadata.raw_data()).unwrap(); 

where length is the length of metadata.raw_data() encoded in some format (like a 32-bit big endian or something)?

[bjorn3]

That should work. Rust-analyzer will need to be updated to reflect this in the proc macro loading code though.

[CraftSpider]

I'll work on making the necessary changes, since it seems like consensus is that supporting this is worth the change. Should I make a PR to rust-analyzer in sync with this change?

[bjorn3]

Rust-analyzer is now a subtree, so you could make the changes in the same PR to this repo. Make sure to keep support for the current format though.

[apiraino]

By reading the latest comments it seems this PR will be waiting on work happening on rust-analyzer. I'll signal this with s-waiting-on-author - feel free to request a review of his PR again with @rustbot ready, thanks!

@rustbot author

[pnkfelix]

r? @wesleywiser

[eddyb]

Ah, I misread that about the position. But I assume those lines could be changed to something like:

let mut compressed = rustc_metadata::METADATA_HEADER.to_vec(); 
compressed.write_all(length).unwrap();
FrameEncoder::new(&mut compressed).write_all(metadata.raw_data()).unwrap(); 

where length is the length of metadata.raw_data() encoded in some format (like a 32-bit big endian or something)?

@ehuss @CraftSpider If you do this change, don't forget to bump the (small) version number in METADATA_HEADER, otherwise the two encodings will get confused for one another (i.e. we try to guarantee that mixing versions of rustc, say stable and nightly, in the same build dir, will not cause unpredictable failure modes due to format mixing - that's why we have a "major" format version in METADATA_HEADER on top of the "rustc version" string later on, because the version string couldn't be correctly found if we change fundamental aspects of the encoding).

[CraftSpider]

I'll make sure to do so, I did start on implementing this change. I'll push an update once I have something working hopefully.

[Dylan-DPC]

@CraftSpider any updates on this?

[wesleywiser]

Compiler changes look good to me!

[wesleywiser]

@Veykril or @lnicola, would you mind double checking that the r-a changes are ok? Thanks! 🙂

[lnicola]

The rust-analyzer changes were merged in rust-lang/rust-analyzer#14153, but we'll double-check once this hits nightly.

[CraftSpider]

@rustbot label: -S-waiting-on-author +S-waiting-on-review

[wesleywiser]

Thanks @CraftSpider!

@bors r+

[bors]

📌 Commit 79f0021 has been approved by wesleywiser

It is now in the queue for this repository.

[matthiaskrgr]

@bors rollup=never let's get this out of the queue

[bors]

⌛ Testing commit 79f0021 with merge a512c6c...

[bors]

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing a512c6c to master...

[rust-timer]

Finished benchmarking commit (a512c6c): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.