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

failed to build archive invalid data encountered #65536

Closed
Mark-Simulacrum opened this issue Oct 18, 2019 · 12 comments · Fixed by #66235
Assignees

Comments

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Oct 18, 2019

ea45150 (#59953) appears to introduce some sort of bug in the archive generation, which causes script-servo to fail to build on perf: error: failed to build archive: Invalid data was encountered while parsing the file.

@eddyb, do you think it's worth reverting that PR for now?

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

@Mark-Simulacrum Mark-Simulacrum commented Oct 18, 2019

Hm, seems like this has since fixed itself. I'm going to close since it was probably spurious.

@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Oct 18, 2019

Hmm, that error sounds like a toolchain bug/limitation (it really shouldn't care what we throw in there).
I wonder what the before/after #59953 .rmeta size is for script-servo.

@jdm

This comment has been minimized.

Copy link
Contributor

@jdm jdm commented Nov 6, 2019

I have encountered this locally building Servo's script crate, as has Servo's CI.

@jdm jdm reopened this Nov 6, 2019
@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Nov 6, 2019

@jdm @Mark-Simulacrum Could someone provide the .rmeta size (e.g. from cargo check) before and after #59953? (which I was wondering about in #65536 (comment))

Presumably for Servo itself it would have to be before and after a relevant rustup?

@jdm

This comment has been minimized.

Copy link
Contributor

@jdm jdm commented Nov 6, 2019

Before:

-rw-r--r--  1 jdm  staff   86138724 Nov  6 15:05 target/debug/deps/libscript-c57b17b8b8dc05ef.rmeta

After:

-rw-r--r--  1 jdm  staff  110432968 Nov  6 14:55 target/debug/deps/libscript-d913be2dd3c2bc7a.rmeta
@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Nov 7, 2019

cc @michaelwoerister That's a 28.2% increase, much larger than what we saw for libcore.

@Mark-Simulacrum Would be interesting to have artifact sizes (for benchmarks themselves) on perf although the damage has been done here.

@jdm Okay that's bad but it doesn't increase the number of bits required to hold the number of bytes, maybe the whole rlib size is closer to that threshold?
That's my only guess so far as to why this is failing.

@jdm

This comment has been minimized.

Copy link
Contributor

@jdm jdm commented Nov 7, 2019

@eddyb The pre-change rlib was ~530mb and post-change was in the range of ~550mb iirc.

@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Nov 8, 2019

@jdm Are you sure? I'm talking to @Eijebong and seeing 5.39GiB (older?) rlibs.
Note that there's a hard limit of 10GB for archives.

EDIT: looks like rust.metadata.bin's offset in the rlib, is very close to 4GiB (i.e. 232 bytes), as it's sandwiched between *.rgcu.o and rcgu.bc.z.

EDIT2: that might have all been irrelevant, looks like the contents of the rmeta are the issue here, trying to create an archive with llvm-ar and just the rmeta fails.

@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Nov 8, 2019

$ llvm-objdump -all-headers libscript.bad.rmeta | head
libscript.bad.rmeta:       file format COFF-<unknown arch>

architecture: unknown
start address: 0x00000000

So it turns out that our 4-byte 00 00 00 00 prefix for rmeta is valid a COFF header.
We have that so that very old rustc versions (pre-1.10, see #34539) don't succeed loading new rlibs, but maybe it's been long enough? cc @rust-lang/compiler
(we could also maybe rename the rust.metadata.bin file in rlib, which would presumably prevent older compilers from finding the file at all?)

It's also possible we'll save some time if LLVM stops trying to parse our rmeta format as COFF.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

@Mark-Simulacrum Mark-Simulacrum commented Nov 8, 2019

Do I understand correctly by backwards compatibility we mean that if you're running an old rustc in a directory that happens to have rmeta files generated by a new rustc we'd just run into compilation errors if we didn't have the 4 byte zero prefix? i.e., it wouldn't introduce anything other than maybe odd-looking errors? (I am struggling to understand why exactly we did this in the first place, as we don't guarantee metadata format within one minor release, even...)

It's also possible we'll save some time if LLVM stops trying to parse our rmeta format as COFF.

Hm, I would be ... surprised if LLVM started trying to parse arbitrary sections as various formats (e.g., COFF) but maybe that's normal behavior...

@eddyb

This comment has been minimized.

Copy link
Member

@eddyb eddyb commented Nov 8, 2019

@Mark-Simulacrum rmeta is newer than 1.10, so it'd really be just rlibs, left around in sysroots or target/ dirs (and I don't even think you can get this kind of error with Cargo?).

Hm, I would be ... surprised if LLVM started trying to parse arbitrary sections as various formats (e.g., COFF) but maybe that's normal behavior...

IIUC, COFF is one of the major 3 .o formats (alongside ELF and Mach-O), and .a libraries are archives of .o files, so this is unsurprising to me.
(EDIT: to clarify, what's surprising is that we accidentally made LLVM think we have a valid-ish COFF file, but once it thinks that, everything makes sense)

LLVM has been parsing our rust.metadata.bin as COFF, ever since 1.10, AFAICT.

(Note that this doesn't apply to the special section of a dylib crate, which has entirely different constraints, and also I think we changed its name since 1.10)

bors added a commit that referenced this issue Nov 9, 2019
rustc_metadata: don't let LLVM confuse rmeta blobs for COFF object files.

This has likely been a silent issue since 1.10 but only caused trouble recently (see #65536 (comment)), when recent changes to the `rmeta` schema introduced more opportunities for COFF parse errors.

To prevent any undesired interactions with old compilers, I've renamed the file inside `rlib`s from `rust.metadata.bin` to `lib.rmeta` (not strongly attached to it, suggestions welcome).

Fixes #65536.

<hr/>

Before:
```
$ llvm-objdump -all-headers build/*/stage1-std/*/release/deps/libcore-*.rmeta

build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/libcore-6b9e8b5a59b79a1d.rmeta: file format COFF-<unknown arch>

architecture: unknown
start address: 0x00000000

Sections:
Idx Name          Size     VMA          Type

SYMBOL TABLE:
```

After:
```
$ llvm-objdump -all-headers build/*/stage1-std/*/release/deps/libcore-*.rmeta

llvm-objdump: error: 'build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/libcore-6b9e8b5a59b79a1d.rmeta':
    The file was not recognized as a valid object file
```
@jdm

This comment has been minimized.

Copy link
Contributor

@jdm jdm commented Nov 9, 2019

Thank you so much @eddyb and @Eijebong!

@bors bors closed this in 0fec5ab Nov 10, 2019
@jdm jdm mentioned this issue Nov 12, 2019
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.