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

Update to LLVM 9 trunk #62592

Merged
merged 7 commits into from
Jul 17, 2019
Merged

Update to LLVM 9 trunk #62592

merged 7 commits into from
Jul 17, 2019

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jul 11, 2019

Following the preparatory changes in #62474, this updates the LLVM submodule to https://github.com/rust-lang/llvm-project/tree/rustc/9.0-2019-07-12 and:

  • Changes the LLVM Rust bindings to account for the new SubtargetSubTypeKV.
  • Adjusts a codegen test for the new form of the byval attribute that takes a type.
  • Makes a PGO codegen test more liberal with regard to order and linkage.
  • Builds InstrProfilingPlatformWindows.c as part of libprofiler_builtins.
  • Moves registration of additional passes (in particular sanitizers) to the end of the module pass manager.
  • Disables LLDB on builders.

r? @alexcrichton

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2019
@cuviper
Copy link
Member

cuviper commented Jul 11, 2019

Can you also update the branch name in .gitmodules?
(useful for git submodule update --remote src/llvm-project)

@nikic nikic force-pushed the actually-update-llvm branch from 462bab4 to 4aeffb2 Compare July 11, 2019 17:57
@nikic
Copy link
Contributor Author

nikic commented Jul 11, 2019

@cuviper Done!

@alexcrichton
Copy link
Member

This looks like it's moving the submodule to 56b8425750e78974fcc8c64429739bc8c2d84f08 which exists but the tip of https://github.com/rust-lang/llvm-project/commits/rustc/9.0-2019-07-08 is rust-lang/llvm-project@bd791b5, was there perhaps a forgotten push to the branch?

@nikic
Copy link
Contributor Author

nikic commented Jul 11, 2019

@alexcrichton The submodule points to the tip of rust-lang/llvm-project#19. If you prefer you can merge that first and I'll update the reference here. I don't have push access to the llvm-project repo.

@alexcrichton
Copy link
Member

@bors: r+ p=1

Er oops sorry forgot about that! I pushed that commit up to avoid creating an extra merge commit and we should be good to go now :)

I'm gonna raise the priority of this since it's highly likely to bounce at least once due to llvm issues, and we should weed those out ASAP. Note that this is also likely to time out while the sccache caches are populated.

@bors
Copy link
Contributor

bors commented Jul 11, 2019

📌 Commit 4aeffb28c1a7b9b5c13712e0097a18e6981cd87e has been approved by alexcrichton

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2019
@bors
Copy link
Contributor

bors commented Jul 11, 2019

⌛ Testing commit 4aeffb28c1a7b9b5c13712e0097a18e6981cd87e with merge ee77050c65f1ea1c37f81e6429e78c6fded991d7...

@bors
Copy link
Contributor

bors commented Jul 11, 2019

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 11, 2019
@nikic
Copy link
Contributor Author

nikic commented Jul 11, 2019

Linker error on macos:

Undefined symbols for architecture x86_64:
  "___isOSVersionAtLeast", referenced from:
  llvm::sys::fs::copy_file(llvm::Twine const&, llvm::Twine const&) in librustc_llvm-c1fd4cbc1eee8759.rlib(Path.cpp.o)
ld: symbol(s) not found for architecture x86_64

@alexcrichton
Copy link
Member

Hm so I've seen that for the longest time in the macOS_static build of curl-rust on azure. I've never been able to track it down further beyond the usage of __builtin_available around here. I suspect this is coming out of https://github.com/rust-lang/llvm-project/blob/56b8425750e78974fcc8c64429739bc8c2d84f08/llvm/lib/Support/Unix/Path.inc#L1157 which is presumably a recent change to OSX.

This is very likely related to our usage of MACOSX_DEPLOYMENT_TARGET in one way or another, although that could also just be a wild guess. The easiest fix would be to just patch that out and not worry about it

@nikic
Copy link
Contributor Author

nikic commented Jul 11, 2019

The correct solution here is probably to either link compiler-rt into librustc_llvm on darwin, or maybe include os_version_check.c in https://github.com/rust-lang-nursery/compiler-builtins/blob/5e06435c291211a6443d3b29b948faa3ccaaa180/build.rs#L199. But I don't have a macos system to test, so I'll probably go with patching it out...

@nikic nikic force-pushed the actually-update-llvm branch from 4aeffb2 to 37be9db Compare July 12, 2019 14:16
@nikic
Copy link
Contributor Author

nikic commented Jul 12, 2019

Updated submodule to include rust-lang/llvm-project#21.

@alexcrichton
Copy link
Member

@bors: r+ delegate+

@bors
Copy link
Contributor

bors commented Jul 12, 2019

✌️ @nikic can now approve this pull request

@bors
Copy link
Contributor

bors commented Jul 12, 2019

📌 Commit 37be9dbde72f5d81ef0ac2b1b6d4163e5ef12d8d has been approved by alexcrichton

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2019
@nikic nikic force-pushed the actually-update-llvm branch from 37be9db to 96050fe Compare July 12, 2019 15:09
@nikic
Copy link
Contributor Author

nikic commented Jul 12, 2019

Another submodule update to include rust-lang/llvm-project#20.

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jul 16, 2019

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@nikic
Copy link
Contributor Author

nikic commented Jul 16, 2019

@pietroalbini @Mark-Simulacrum Something weird happened here...

@pietroalbini
Copy link
Member

@bors retry

Bors created the merge commit with the wrong base, so it failed to merge it. Why it did that is a really good question.

@bors
Copy link
Contributor

bors commented Jul 16, 2019

⌛ Testing commit d2c1d1b with merge 38798c6...

bors added a commit that referenced this pull request Jul 16, 2019
Update to LLVM 9 trunk

Following the preparatory changes in #62474, this updates the LLVM submodule to https://github.com/rust-lang/llvm-project/tree/rustc/9.0-2019-07-12 and:

 * Changes the LLVM Rust bindings to account for the new SubtargetSubTypeKV.
 * Adjusts a codegen test for the new form of the byval attribute that takes a type.
 * Makes a PGO codegen test more liberal with regard to order and linkage.
 * Builds InstrProfilingPlatformWindows.c as part of libprofiler_builtins.
 * Moves registration of additional passes (in particular sanitizers) to the end of the module pass manager.
 * Disables LLDB on builders.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Jul 17, 2019

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 38798c6 to master...

@bors bors merged commit d2c1d1b into rust-lang:master Jul 17, 2019
@golddranks
Copy link
Contributor

@nikic What's the reason building LLDB was disabled? I can't find any rationale in this thread.

@nikic
Copy link
Contributor Author

nikic commented Jul 23, 2019

@golddranks This was discussed in rust-lang/llvm-project#19. However, I think the decision might have been made on an incorrect premise, that we're not shipping lldb via rustup. It looks like that's true for Linux, but we did ship it on apple targets, such as https://rust-lang.github.io/rustup-components-history/x86_64-apple-darwin.html.

As the rust lldb patches currently don't have a maintainer, I don't think we'd want to bring those back, but possibly we should be shipping a vanilla LLDB build for those targets where we used to ship something previously? Not sure what the benefit of doing that over just using a system LLDB is though.

@alexcrichton
Copy link
Member

FWIW despite being shipped in rustup it still wasn't really operational or hooked up anywhere. The binary wasn't code-signed so I don't think it worked without extra privileges, and nothing was hooked up to the binary that was installed and the binary was very deeply buried in the sysroot (not added to PATH) or anything. It still effectively (AFAIK) was unused and not well tested. The original purpose was to start down a road of shipping LLDB but it ended up not making a lot of progress, so this saves time in rebasing and on CI while we figure out a different way to move forward.

@golddranks
Copy link
Contributor

golddranks commented Jul 24, 2019

It seems that the lldb-preview compontent has been missing in the nightlies; we have been shipping that experimentally on macOS.

About shipping our own LLDB builds: at least we can then be sure about the interface. There was at least one example of it breaking because the command line interface changed a little ( d6e410b#diff-81ad6c936ea3b391e9adf118059012ed ) I'm not sure how much things tend to change over time, but shipping our own LLDB would at least guarantee that it works with the scripts.

However, I didn't know that we used to patch LLDB. Do you have any idea to what extent the functionality of the scripts depends on the patches? How big the patches are and is there any chances of upstreaming at least some, or are we doomed to play catch up if we support LLDB? Edit: Missed @alexcrichton 's comment. Huh, I guess I'm gonna double check what binary rust-lldb uses!

wangrunji0408 added a commit to rcore-os/rCore that referenced this pull request Sep 23, 2019
bors added a commit to rust-lang-ci/rust that referenced this pull request May 14, 2020
bootstrap: remove lldb dist packaging

The lldb-preview rustup package is missing on every single target, and has never been shipped beyond x86_64-apple-darwin. It was removed in rust-lang#62592 which landed around a year ago, and there's not been demand that we re-enable it since, so we're now removing support entirely to cleanup the code a bit.

The hope is that this will also kill the useless "lldb-preview" row on https://rust-lang.github.io/rustup-components-history/.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants