Use a u64 for the rmeta root position #118344

saethlin · 2023-11-27T02:02:10Z

Waffle noticed this in #117301 (comment)

We've upgraded the other file offsets to u64, and this one only costs 4 bytes per file. Also the way the truncation was being done before was extremely easy to miss, I sure missed it! It's not clear to me if not having this change effectively made the other upgrades from u32 to u64 ineffective, but we can have it now.

r? @WaffleLapkin

compiler/rustc_metadata/src/rmeta/decoder.rs

compiler/rustc_metadata/src/rmeta/encoder.rs

rustbot · 2023-11-28T01:53:16Z

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

lnicola · 2023-11-28T12:12:10Z

src/tools/rust-analyzer/crates/proc-macro-api/src/version.rs

+    // so 17 bytes in total, and we should check the 17th byte
    // to know the length
-    let mut bytes_before_version = [0u8; 13];
+    let mut bytes_before_version = [0u8; 17];


This breaks support for previous versions, right?

The server will generally match the compiler, but this might still happen with already-built projects.

I think I patched this up by passing more data out of the version match above?

It probably doesn't matter too much, but I think you could leave that as a 17 bytes array, and only read into a prefix slice of it?

That feels like too much brainpower for this code to me.

WaffleLapkin · 2023-12-10T22:50:11Z

Hm, does this ever matter? I.e. should we maybe simply fix the cast and leave it as 32-bit offset?

saethlin · 2023-12-10T23:42:26Z

The crate root is written at the end of an rmeta file. So regardless of what we do elsewhere, we only support rmeta files up to the size that this offset supports.

Unlike the other examples such as #103607 this needs two crates, because you need to write out an rmeta file that is more than 4 GB, then have another crate that tries to use it. include_bytes! a 2 GB file into a const then try to print that const's length from another crate, for example. (I think you're suggesting we do a panicky instead of truncating conversion here? If we did that the first build would ICE not the second)

So without this change, use cases #112934 and #111855 are only fixed if the large crate is not a dependency.

WaffleLapkin · 2023-12-11T02:56:55Z

Thanks for the explanation (I assumed (based on nothing) that the root is at the start)

compiler/rustc_codegen_ssa/src/back/metadata.rs

compiler/rustc_metadata/src/rmeta/decoder.rs

src/tools/rust-analyzer/crates/proc-macro-api/src/version.rs

WaffleLapkin · 2023-12-11T03:11:13Z

src/tools/rust-analyzer/crates/proc-macro-api/src/version.rs

    // Last supported version is:
    // https://github.com/rust-lang/rust/commit/0696e79f2740ad89309269b460579e548a5cd632


I think this should point to your commit updating the format (yes it looks like we forgot to update it 2 times already).

It's so outdated that when I saw this I wasn't sure if it's supposed to point at the oldest or the newest version that's supported. I can point this at the first commit in this PR.

I think it's supposed to point at the newest, because

Last

This points to 6, but the oldest supported version is 5, so I don't think it could have been added as an oldest one

src/tools/rust-analyzer/crates/proc-macro-api/src/version.rs

Co-authored-by: Waffle Maybe <waffle.lapkin@gmail.com>

WaffleLapkin · 2023-12-11T13:27:03Z

@bors r+

bors · 2023-12-11T13:27:06Z

📌 Commit 79bdd24 has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-12-11T17:21:17Z

⌛ Testing commit 79bdd24 with merge 5701093...

bors · 2023-12-11T19:18:18Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 5701093 to master...

rust-timer · 2023-12-11T20:41:17Z

Finished benchmarking commit (5701093): 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)	4.7%	[2.3%, 7.2%]	2
Regressions ❌ (secondary)	1.5%	[1.5%, 1.5%]	1
Improvements ✅ (primary)	-4.1%	[-4.1%, -4.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[-4.1%, 7.2%]	3

Cycles

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.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	-	-	0

Binary size

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.0%	[0.0%, 0.0%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 672.509s -> 670.568s (-0.29%)
Artifact size: 314.21 MiB -> 314.19 MiB (-0.01%)

…Lapkin [AIX] Fix XCOFF metadata rust-lang#118344 accidentally changed the way to get metadata from XCOFF file and broken our internal CI. This PR reverts part of rust-lang#118344 .

Rollup merge of rust-lang#118905 - bzEq:revert-u64-on-xcoff, r=WaffleLapkin [AIX] Fix XCOFF metadata rust-lang#118344 accidentally changed the way to get metadata from XCOFF file and broken our internal CI. This PR reverts part of rust-lang#118344 .

rustbot assigned WaffleLapkin Nov 27, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 27, 2023

This comment has been minimized.

Sign in to view

Mark-Simulacrum reviewed Nov 27, 2023

View reviewed changes

compiler/rustc_metadata/src/rmeta/decoder.rs Show resolved Hide resolved

saethlin force-pushed the rmeta-header-pos branch from 1239003 to c415fdf Compare November 27, 2023 23:25

This comment has been minimized.

Sign in to view

saethlin force-pushed the rmeta-header-pos branch from c415fdf to 424788e Compare November 27, 2023 23:44

Mark-Simulacrum reviewed Nov 28, 2023

View reviewed changes

compiler/rustc_metadata/src/rmeta/encoder.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

saethlin force-pushed the rmeta-header-pos branch from 424788e to 3a7c358 Compare November 28, 2023 01:53

lnicola reviewed Nov 28, 2023

View reviewed changes

Use a u64 for the rmeta root position

b94cfef

saethlin force-pushed the rmeta-header-pos branch from 3a7c358 to b94cfef Compare November 28, 2023 23:04

WaffleLapkin requested changes Dec 11, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2023

This comment has been minimized.

Sign in to view

Apply suggestions from code review

79bdd24

Co-authored-by: Waffle Maybe <waffle.lapkin@gmail.com>

saethlin force-pushed the rmeta-header-pos branch from 5f26659 to 79bdd24 Compare December 11, 2023 04:26

WaffleLapkin approved these changes Dec 11, 2023

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 11, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 11, 2023

bors merged commit 5701093 into rust-lang:master Dec 11, 2023

rustbot added this to the 1.76.0 milestone Dec 11, 2023

saethlin deleted the rmeta-header-pos branch December 11, 2023 19:50

bzEq mentioned this pull request Dec 13, 2023

[AIX] Fix XCOFF metadata #118905

Merged

		// Last supported version is:
		// https://github.com/rust-lang/rust/commit/0696e79f2740ad89309269b460579e548a5cd632

Use a u64 for the rmeta root position #118344

Use a u64 for the rmeta root position #118344

Uh oh!

Conversation

saethlin commented Nov 27, 2023

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

Uh oh!

This comment has been minimized.

rustbot commented Nov 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lnicola Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaffleLapkin commented Dec 10, 2023

Uh oh!

saethlin commented Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WaffleLapkin commented Dec 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saethlin Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

WaffleLapkin commented Dec 11, 2023

Uh oh!

bors commented Dec 11, 2023

Uh oh!

bors commented Dec 11, 2023

Uh oh!

bors commented Dec 11, 2023

Uh oh!

rust-timer commented Dec 11, 2023

Overall result: no relevant changes - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lnicola Nov 29, 2023 •

edited

Loading

saethlin commented Dec 10, 2023 •

edited

Loading

saethlin Dec 11, 2023 •

edited

Loading