Skip to content

/rust

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

msvc: Get codegen-units working #26869

Merged
merged 2 commits into from Jul 9, 2015

Conversation

Reviewers
No reviews
Assignees

@nrc nrc

Labels
regression-from-stable-to-nightly relnotes
Projects
None yet
Milestone
No milestone
5 participants
@alexcrichton @rust-highfive @nrc @bors @pcwalton
@alexcrichton
Copy link
Member

alexcrichton commented Jul 7, 2015

This commit alters the implementation of multiple codegen units slightly to be
compatible with the MSVC linker. Currently the implementation will take the N
object files created by each codegen unit and will run ld -r to create a new
object file which is then passed along. The MSVC linker, however, is not able to
do this operation.

The compiler will now no longer attempt to assemble object files together but
will instead just pass through all the object files as usual. This implies that
rlibs may not contain more than one object file (if the library is compiled with
more than one codegen unit) and the output of -C save-temps will have changed
slightly as object files with the extension 0.o will not be renamed to o
unless requested otherwise.

@rust-highfive rust-highfive assigned pcwalton Jul 7, 2015

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Jul 7, 2015

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 7, 2015

r? @nrc

cc @brson

@rust-highfive rust-highfive assigned nrc and unassigned pcwalton Jul 7, 2015

@alexcrichton alexcrichton force-pushed the alexcrichton:fix-msvc-sepcomp branch 2 times, most recently from d084b21 to 1bfd21f Jul 7, 2015

@nrc

This comment has been minimized.

Sign in to view
Copy link
Member

nrc commented Jul 7, 2015

Did you test this on all the platforms? There were a lot of subtle linking bugs the first time around, and testing on the bots does not currently test codegen-units.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 7, 2015

This passes the test suite on 64-bit linux, but it looks like we do have a bunch of codegen-units tests? The src/test/run-{make,pass}/sepcomp-* tests are all run on all platforms I believe. I've verified that this passes tests on 64-bit MSVC, but I haven't tested on OSX or GNU Windows yet (figured the bots would take care of that)
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member

nrc commented Jul 7, 2015

It would be good to know that we can bootstrap with codegen-units=4 on Linux and OSX, I think the full bootstrap exercises a bunch of code paths the tests miss
@alexcrichton
msvc: Get codegen-units working 
This commit alters the implementation of multiple codegen units slightly to be
compatible with the MSVC linker. Currently the implementation will take the N
object files created by each codegen unit and will run `ld -r` to create a new
object file which is then passed along. The MSVC linker, however, is not able to
do this operation.

The compiler will now no longer attempt to assemble object files together but
will instead just pass through all the object files as usual. This implies that
rlibs may not contain more than one object file (if the library is compiled with
more than one codegen unit) and the output of `-C save-temps` will have changed
slightly as object files with the extension `0.o` will not be renamed to `o`
unless requested otherwise.
d23239b

@alexcrichton alexcrichton force-pushed the alexcrichton:fix-msvc-sepcomp branch from 1bfd21f to 6987d50 Jul 8, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 8, 2015

Good catch @nrc, this ended up causing the bootstrap to fail due to #14344 so I added a commit to fix that as well.
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member

nrc commented Jul 8, 2015

Do you know how big an effect on compile times this will have? I'm wondering if it is worth keeping around both paths so we don't affect compile times where we don't need to. (I imagine it is not worth the extra complexity, but if the effect is enormous...)
@nrc

This comment has been minimized.

Sign in to view
Copy link
Member

nrc commented Jul 8, 2015

@bors: r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 8, 2015

📌 Commit 6987d50 has been approved by nrc
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 8, 2015

If you create a "hello world" dylib this commit increases the time from 200ms to 290ms, the breakdown of the timings as:

  time: 0.022; rss: 42MB        altering std-7e44814b.rlib
  time: 0.015; rss: 50MB        altering collections-7e44814b.rlib
  time: 0.010; rss: 59MB        altering rustc_unicode-7e44814b.rlib
  time: 0.007; rss: 67MB        altering rand-7e44814b.rlib
  time: 0.010; rss: 75MB        altering alloc-7e44814b.rlib
  time: 0.007; rss: 83MB        altering libc-7e44814b.rlib
  time: 0.033; rss: 92MB        altering core-7e44814b.rlib

I do think that creating dylibs isn't exactly common, but this is definitely unfortunate. I'm hoping that we can stop shelling out to ar ASAP by moving llvm-ar.cpp into LLVM proper (e.g. use it as a library) which should reduce these times to basically 0 (hopefully).

So this'll definitely slow us down now, but hopefully we can make it up in the future! Also unfortunately I don't think there's a case where we can avoid these extra copies on a dylib b/c they're technically needed for correctness (to ensure that all code makes it into the dylib).

Another possible route in the future for improvement is to separate the notion of an intermediate dylib and a final dylib (e.g. a dylib to be used again in Rust and a dylib to be used in C). If we're creating a dylib to be used in C we don't need to do any of these alterations because we want the linker to eliminate as much as possible. Unfortunately we don't know which case is which, so we have to assume they're all intermediate libs in which case everything is needed.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 8, 2015

⌛️ Testing commit 6987d50 with merge 0598375...

bors added a commit that referenced this pull request Jul 8, 2015

@bors
Auto merge of #26869 - alexcrichton:fix-msvc-sepcomp, r=nrc 
This commit alters the implementation of multiple codegen units slightly to be
compatible with the MSVC linker. Currently the implementation will take the N
object files created by each codegen unit and will run `ld -r` to create a new
object file which is then passed along. The MSVC linker, however, is not able to
do this operation.

The compiler will now no longer attempt to assemble object files together but
will instead just pass through all the object files as usual. This implies that
rlibs may not contain more than one object file (if the library is compiled with
more than one codegen unit) and the output of `-C save-temps` will have changed
slightly as object files with the extension `0.o` will not be renamed to `o`
unless requested otherwise.
0598375
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 8, 2015

💔 Test failed - auto-mac-32-opt
@alexcrichton
trans: Link rlibs to dylibs with --whole-archive 
This commit starts passing the `--whole-archive` flag (`-force_load` on OSX) to
the linker when linking rlibs into dylibs. The primary purpose of this commit is
to ensure that the linker doesn't strip out objects from an archive when
creating a dynamic library. Information on how this can go wrong can be found in
issues #14344 and #25185.

The unfortunate part about passing this flag to the linker is that we have to
preprocess the rlib to remove the metadata and compressed bytecode found within.
This means that creating a dylib will now take longer to link as we've got to
copy around the input rlibs to a temporary location, modify them, and then
invoke the linker. This isn't done for executables, however, so the "hello
world" compile time is not affected.

This fix was instigated because of the previous commit where rlibs may not
contain multiple object files instead of one due to codegen units being greater
than one. That change prevented the main distribution from being compiled with
more than one codegen-unit and this commit fixes that.

Closes #14344
Closes #25185
Loading status checks…
9bc8e6d

@alexcrichton alexcrichton force-pushed the alexcrichton:fix-msvc-sepcomp branch from 6987d50 to 9bc8e6d Jul 8, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Jul 8, 2015

@bors: r=nrc 9bc8e6d

bors added a commit that referenced this pull request Jul 8, 2015

@bors
Auto merge of #26869 - alexcrichton:fix-msvc-sepcomp, r=nrc 
This commit alters the implementation of multiple codegen units slightly to be
compatible with the MSVC linker. Currently the implementation will take the N
object files created by each codegen unit and will run `ld -r` to create a new
object file which is then passed along. The MSVC linker, however, is not able to
do this operation.

The compiler will now no longer attempt to assemble object files together but
will instead just pass through all the object files as usual. This implies that
rlibs may not contain more than one object file (if the library is compiled with
more than one codegen unit) and the output of `-C save-temps` will have changed
slightly as object files with the extension `0.o` will not be renamed to `o`
unless requested otherwise.
Loading status checks…
9f26f14
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Jul 8, 2015

⌛️ Testing commit 9bc8e6d with merge 9f26f14...
@bors

@bors bors merged commit 9bc8e6d into rust-lang:master Jul 9, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@alexcrichton alexcrichton deleted the alexcrichton:fix-msvc-sepcomp branch Jul 10, 2015

@alexcrichton alexcrichton referenced this pull request Jul 14, 2015

Merged

Upgrade to rustc 1.3.0-dev (fddfd089b 2015-07-10) #6591

@pcwalton

This comment has been minimized.

Sign in to view
Copy link
Contributor

pcwalton commented Jul 14, 2015

NB: This was a breaking change for Servo. Please put this in a release note because we may not be the only ones to be broken because of it.

@alexcrichton alexcrichton added the relnotes label Jul 14, 2015

@alexcrichton alexcrichton referenced this pull request Aug 13, 2015

Closed

Linkage regression in servo with 1.2 #27043

@alexcrichton alexcrichton added the regression-from-stable-to-nightly label Aug 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.