Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the metadata lock more robust #34539

Merged
merged 5 commits into from
Jul 2, 2016
Merged

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 28, 2016

Fixes #33778 and friends.

I also needed to add a metadata encoding version to rlibs, as they did not have it before. To keep it backwards-compatible, I added 4 zeroes to the start of the metadata, which are treated as an empty length field by older rustcs.

r? @alexcrichton

@alexcrichton
Copy link
Member

The refactorings here all look good to me, but I thought we already had a system like this for breaking changes to metadata? This constant is in theory updated for all major version changes to metadata, although it was last updated 2015-03-02 so it's not exactly updated that often...

It seems to me that even with a json header we'd still have to actively go and bump the version number, so we'd largely be in the same situation we're in today? I'm curious, though, is there a particular reason you'd want to switch to a json header rather than just a version number stored at the front?

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 30, 2016

@alexcrichton

The loader needs to load a few fields before it can figure out whether it should load a crate or not. I prefer keeping these fields separate from the rest, to prevent us from changing them and causing a recurrence of this issue.

@alexcrichton
Copy link
Member

Yeah the loader will definitely always have to load something at least, I think I'm just not connecting the dots as to why today's implementation (just a version number in the first N bytes) is either better or worse than encoding information as JSON.

We can perhaps provide a better diagnostic in the case where JSON is used because we can be sure that some fields are always there, but I'm not sure it's worth the implementation overhead because this in theory should basically never happen.


In other words, I don't think this is technically buying us anything because if we bumped the metadata version then all historical compilers would automatically be unable to read future compilers' metadata.

@arielb1 arielb1 changed the title Add a JSON header to metadata Make metadata handling more robust Jul 1, 2016
@arielb1 arielb1 changed the title Make metadata handling more robust Make the metadata guard more robust Jul 1, 2016
@arielb1
Copy link
Contributor Author

arielb1 commented Jul 1, 2016

updated to use a simpler approach.

@arielb1 arielb1 added beta-nominated Nominated for backporting to the compiler in the beta channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jul 1, 2016
@arielb1 arielb1 changed the title Make the metadata guard more robust Make the metadata lock more robust Jul 1, 2016
@alexcrichton
Copy link
Member

@bors: r+ 7743606186885c6887ad0c8667bc7106e0facc3a

Thanks @arielb1!

@arielb1
Copy link
Contributor Author

arielb1 commented Jul 1, 2016

Apparently the error message for corrupt metadata is broken:

foo.rs:11:1: 11:18 error: found crate `foo` compiled by an incompatible version of rustc [E0514]
foo.rs:11 extern crate foo;
          ^~~~~~~~~~~~~~~~~
foo.rs:11:1: 11:18 help: please recompile that crate using this compiler (rustc 1.10.0-dev (e324784f7 2016-07-02))
foo.rs:11:1: 11:18 note: crate `foo` path #1: .. compiled by "an unknown compiler"
error: aborting due to previous error

Wait. We don't have metadata encoding version on rlibs. at all.

@bors r- for now. Will re-r= after I fix that.

…sion

check the metadata lock when loading rather than afterwards

Fixes rust-lang#33733
Fixes rust-lang#33015
previously, only .so files included a metadata encoding version, *outside*
of the zlib compressed area. This adds an encoding version inside the metadata
itself, in both .so and .rlib files.

Fixes rust-lang#33778.
@arielb1
Copy link
Contributor Author

arielb1 commented Jul 2, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jul 2, 2016

📌 Commit 42b7c32 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jul 2, 2016

⌛ Testing commit 42b7c32 with merge 7a262d3...

bors added a commit that referenced this pull request Jul 2, 2016
Make the metadata lock more robust

Fixes #33778 and friends.

I also needed to add a metadata encoding version to rlibs, as they did not have it before. To keep it backwards-compatible, I added 4 zeroes to the start of the metadata, which are treated as an empty length field by older rustcs.

r? @alexcrichton
@bors bors merged commit 42b7c32 into rust-lang:master Jul 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants