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

Better support for `--llvm-root`. #27937

Merged
merged 1 commit into from Dec 14, 2015

Conversation

Reviewers
No reviews
Assignees

@brson brson

Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@DiamondLovesYou @rust-highfive @alexcrichton @brson @wthrowe @bors @jauhien
@DiamondLovesYou
Copy link
Contributor

DiamondLovesYou commented Aug 22, 2015

This handles cases when the LLVM used isn't configured will the 'usual' targets. Also, cases where LLVM is shared are also handled (ie with LD_LIBRARY_PATH etc).

@rust-highfive rust-highfive assigned brson Aug 22, 2015

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Aug 22, 2015

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)
mk/tests.mk Outdated
$$(S)
$$(S) \
"$$(LLVM_LIBDIR_RUSTFLAGS_$(3))" \
"$$(shell $$(LLVM_CONFIG_$(3)) --components)"

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Aug 24, 2015

Member

Can this output be cached in a variable to prevent re-running it?

This comment has been minimized.

Sign in to view
Copy link
@DiamondLovesYou

DiamondLovesYou Aug 25, 2015

Author Contributor

Done.
src/librustc_llvm/lib.rs Outdated
@@ -2327,6 +2328,37 @@ pub unsafe fn debug_loc_to_string(c: ContextRef, tr: DebugLocRef) -> String {
.expect("got a non-UTF8 DebugLoc from LLVM")
}

pub fn initialize_available_targets() {
macro_rules! init_target(
($cfg:ident $fn_name:ident) => { {

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Aug 24, 2015

Member

The fn_name ident may be better named arch_name

This comment has been minimized.

Sign in to view
Copy link
@DiamondLovesYou

DiamondLovesYou Aug 25, 2015

Author Contributor

Yeah, silly me. Done.
mk/llvm.mk Outdated
@@ -83,7 +83,7 @@ endif
LLVM_LINKAGE_PATH_$(1):=$$(abspath $$(RT_OUTPUT_DIR_$(1))/llvmdeps.rs)
$$(LLVM_LINKAGE_PATH_$(1)): $(S)src/etc/mklldeps.py $$(LLVM_CONFIG_$(1))
$(Q)$(CFG_PYTHON) "$$<" "$$@" "$$(LLVM_COMPONENTS)" "$$(CFG_ENABLE_LLVM_STATIC_STDCPP)" \
$$(LLVM_CONFIG_$(1))
"$$(CFG_ENABLE_LLVM_LIBCPP)" $$(LLVM_CONFIG_$(1))

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Aug 24, 2015

Member

In the configure script it looks like this is CFG_ENABLE_LIBCPP instead of LLVM_LIBCPP, was this defined elsewhere though?

This comment has been minimized.

Sign in to view
Copy link
@DiamondLovesYou

DiamondLovesYou Aug 24, 2015

Author Contributor

Actually it's worse than that: configure doesn't even putvar anything related to libcpp. Nice catch, thanks.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Aug 24, 2015

The travis failures here look like they may not be benign

@DiamondLovesYou DiamondLovesYou force-pushed the DiamondLovesYou:llvm-root-and-shared branch 3 times, most recently from 8bfc6cd to 320cc47 Aug 25, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Aug 25, 2015

Can this hold off on the changes to tests/maketest for now? The changes to compiletest are only needed if tests are linking to the compiler (e.g. fulldeps), but for most targets with custom LLVM builds we probably don't need the compiler itself working there just yet, so fixes like this can come later if absolutely necessary. Same with the changes to run-make as I believe run-make isn't run for target-only targets (as opposed to host+target targets).

I continue to be very wary of landing all these changes as it's quite a deal of difficult logic that isn't tested in any way, so I prefer to minimize the impact as much as possible.
@DiamondLovesYou

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

DiamondLovesYou commented Aug 25, 2015

@alexcrichton They were due to variable expansions happening before llvm-config was built (ie not an issue if make all is run before tidy or check, thus I never caught it locally). Anyway, fixed.

DiamondLovesYou added a commit to DiamondLovesYou/rust that referenced this pull request Aug 26, 2015

@DiamondLovesYou
Make the fulldeps modes independent of their respective non-fulldeps … 
…mode.

This is a change needed for rust-lang#27937, so that `-L $(llvm-config --libdir)` is only
added to RUSTFLAGS for fulldeps based compiletest modes.
Loading status checks…
6dae7de

@DiamondLovesYou DiamondLovesYou referenced this pull request Aug 26, 2015

Closed

Make the fulldeps modes independent of their respective non-fulldeps mode. #28017

@DiamondLovesYou DiamondLovesYou force-pushed the DiamondLovesYou:llvm-root-and-shared branch from 320cc47 to 8a345c5 Aug 26, 2015

@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Aug 27, 2015

Looks like good cleanup. I agree that the additional complexity in compiletest is unwarranted for now. Maybe we can fix this problem some other way and I'm happy to ignore all fulldeps tests on the web for a while.
@DiamondLovesYou

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

DiamondLovesYou commented Sep 9, 2015

@brson I'm not sure I understand what you mean by "I'm happy to ignore all fulldeps tests on the web for a while". Could you explain exactly what you mean?

Also, surely the benefit of the fulldep tests outweighs the pretty minimal additional complexity in compiletest (from calling llvm-config)? I mean I have no problem just disabling the fulldeps if that's way you'd like to go. I can file an issue and then comment out the fulldep test modes in tests.mk (assuming this is what you meant from above).
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Sep 9, 2015

Currently running the fulldeps tests means that the entire compiler has to be built for the target platform, and in this case that probably isn't necessary. I believe @brson means that in the interest of keeping this minimal (e.g. only running the run-pass tests for things like PNaCl) the changes to compiletest shouldn't be necessary because none of those link to LLVM.

Unfortunately "minimal complexity" ends up being bitrotted very quickly without being exercised which ends up porting it forward for a few years or so only to be painfully deleted at some point.

@alexcrichton alexcrichton referenced this pull request Sep 10, 2015

Closed

Don't force system LLVM libs to be linked staticly #28327

@wthrowe

This comment has been minimized.

Sign in to view
Copy link
Contributor

wthrowe commented Sep 10, 2015

Following up on some discussion on #28327: The current patch does not work for my use case, and I don't think the LLVM patch mentioned in the #28327 discussion is going to help, because I don't have a libLLVM.so or libLLVM-${VERSION}.so, so the current llvm-config logic will still apply. My system has all of the individual LLVM libraries installed as separate shared objects, so llvm-config is correctly reporting a long list of linker arguments for shared libraries.
@DiamondLovesYou

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

DiamondLovesYou commented Sep 10, 2015

Is your system's LLVM built with CMake && BUILD_SHARED_LIBS ON? CMake
will build separate shared objects for each component in that case.

I'll have to add an extra flag to llvm-config to fix that.

On Thu, Sep 10, 2015 at 12:14 PM, wthrowe notifications@github.com wrote:

Following up on some discussion on #28327
#28327: The current patch does
not work for my use case, and I don't think the LLVM patch mentioned in the
#28327 #28327 discussion is going
to help, because I don't have a libLLVM.so or libLLVM-${VERSION}.so, so
the current llvm-config logic will still apply. My system has all of the
individual LLVM libraries installed as separate shared objects, so
llvm-config is correctly reporting a long list of linker arguments for
shared libraries.

—
Reply to this email directly or view it on GitHub
#27937 (comment).
@wthrowe

This comment has been minimized.

Sign in to view
Copy link
Contributor

wthrowe commented Sep 10, 2015

Yes, it looks like it uses CMake with -DBUILD_SHARED_LIBS=ON. (This is Gentoo's official llvm-3.7.0 package.)

@tomaka tomaka referenced this pull request Sep 15, 2015

Closed

Make Rust work with emscripten #604

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Sep 17, 2015

☔️ The latest upstream changes (presumably #28421) made this pull request unmergeable. Please resolve the merge conflicts.
@jauhien

This comment has been minimized.

Sign in to view
Copy link
Contributor

jauhien commented Sep 21, 2015

Just fyi: we in Gentoo are interested in the possibility to link Rust to separated LLVM shared libraries, otherwise we can't use the system wide LLVM which is quite sad from the point of view of the overall build time.

jauhien added a commit to jauhien/gentoo-rust that referenced this pull request Sep 23, 2015

@jauhien
[dev-lang/rust] version bump 
LLVM in Gentoo is shipped as static libs only now,
until Rust upstream fixies linking using system wide LLVM is impossible
see rust-lang/rust#27937

-Werror during Rust building was disabled
Unverified
This user has not uploaded their public key yet.
GPG key ID: BBEE937DB2EFA1D4 Learn about signing commits
ee08587
@DiamondLovesYou

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

DiamondLovesYou commented Sep 27, 2015

@jauhien I've just created this patch. It fixes llvm-config for cmake && BUILD_SHARED_LIBS.

@brson brson referenced this pull request Nov 4, 2015

Closed

Dynamic LLVM #29558

@DiamondLovesYou DiamondLovesYou force-pushed the DiamondLovesYou:llvm-root-and-shared branch 3 times, most recently from 95e6097 to f8e9e86 Nov 11, 2015

@DiamondLovesYou

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

DiamondLovesYou commented Nov 24, 2015

Ready to go.
mk/llvm.mk Outdated
$$(LLVM_CONFIG_$(1)) "$(CFG_STDCPP_NAME)"
$(Q)$(CFG_PYTHON) "$$<" "$$@" "$(filter $(shell \
$(CFG_LLVM_ROOT)/bin/llvm-config$(X_$(CFG_BUILD)) \
--components), \

This comment has been minimized.

Sign in to view
Copy link
@alexcrichton

alexcrichton Nov 30, 2015

Member

Can this be replaced with a variable defined below to avoid re-invoking llvm-config?

This comment has been minimized.

Sign in to view
Copy link
@DiamondLovesYou

DiamondLovesYou Dec 3, 2015

Author Contributor

No, because we don't want to link unused LLVM components into rustc.

This comment has been minimized.

Sign in to view
Copy link
@DiamondLovesYou

DiamondLovesYou Dec 3, 2015

Author Contributor

I mean, I don't have a problem with just linking everything, I just thought you guys might not like it.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Nov 30, 2015

I have a feeling that this is likely to break over time (as there's no CI and this looks pretty brittle as so many build system things are), but it seems fine as it should be easy to get back up to speed in the future.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 2, 2015

☔️ The latest upstream changes (presumably #30157) made this pull request unmergeable. Please resolve the merge conflicts.

@DiamondLovesYou DiamondLovesYou force-pushed the DiamondLovesYou:llvm-root-and-shared branch from 7026709 to 049042e Dec 3, 2015

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Dec 11, 2015

Ok, the point about re-invoking llvm-config sounds good to me, can these commits be squashed into one as well? Other than that r=me

@DiamondLovesYou DiamondLovesYou force-pushed the DiamondLovesYou:llvm-root-and-shared branch from 049042e to abc6e7a Dec 12, 2015

@DiamondLovesYou

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

DiamondLovesYou commented Dec 13, 2015

Done!
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Dec 13, 2015

@bors: r+ abc6e7a

bors added a commit that referenced this pull request Dec 13, 2015

@bors
Auto merge of #27937 - DiamondLovesYou:llvm-root-and-shared, r=alexcr… 
…ichton

This handles cases when the LLVM used isn't configured will the 'usual' targets. Also, cases where LLVM is shared are also handled (ie with `LD_LIBRARY_PATH` etc).
cdc7eff
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 13, 2015

⌛️ Testing commit abc6e7a with merge 3b5e8bf...

bors added a commit that referenced this pull request Dec 13, 2015

@bors
Auto merge of #27937 - DiamondLovesYou:llvm-root-and-shared, r=alexcr… 
…ichton

This handles cases when the LLVM used isn't configured will the 'usual' targets. Also, cases where LLVM is shared are also handled (ie with `LD_LIBRARY_PATH` etc).
3b5e8bf
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 13, 2015

💔 Test failed - auto-mac-64-nopt-t
@DiamondLovesYou
Better support for `--llvm-root`. 
This handles cases when the LLVM used isn't configured will the 'usual'
targets. Also, cases where LLVM is shared are also handled (ie with
`LD_LIBRARY_PATH` etc).
Loading status checks…
7bd69f2

@DiamondLovesYou DiamondLovesYou force-pushed the DiamondLovesYou:llvm-root-and-shared branch from abc6e7a to 7bd69f2 Dec 13, 2015

@DiamondLovesYou

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

DiamondLovesYou commented Dec 13, 2015

Whoops, I forgot to push a change to llvm.mk.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Dec 14, 2015

@bors: r+ 7bd69f2
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Dec 14, 2015

⌛️ Testing commit 7bd69f2 with merge f150c17...

bors added a commit that referenced this pull request Dec 14, 2015

@bors
Auto merge of #27937 - DiamondLovesYou:llvm-root-and-shared, r=alexcr… 
…ichton

This handles cases when the LLVM used isn't configured will the 'usual' targets. Also, cases where LLVM is shared are also handled (ie with `LD_LIBRARY_PATH` etc).
f150c17
@bors

@bors bors merged commit 7bd69f2 into rust-lang:master Dec 14, 2015
2 checks passed

2 checks passed

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

@bors bors referenced this pull request Dec 14, 2015

Merged

mk: fix undefined variable warnings #30367

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 16, 2015

@alexcrichton
mk: Use the right llvmdeps.rs file for cross build 
It looks like rust-lang#27937 accidentally switched the llvmdeps file from the target to
the host by accident, so be sure to use the right llvmdeps file which is built
for the target when building rustc_llvm
04f9a3f

@alexcrichton alexcrichton referenced this pull request Dec 16, 2015

Merged

mk: Use the right llvmdeps.rs file for cross build #30414

bors added a commit that referenced this pull request Dec 18, 2015

@bors
Auto merge of #30414 - alexcrichton:fix-cross-builds, r=brson 
It looks like #27937 accidentally switched the llvmdeps file from the target to
the host by accident, so be sure to use the right llvmdeps file which is built
for the target when building rustc_llvm
27d5511

ubsan added a commit to ubsan/rust that referenced this pull request Dec 22, 2015

@alexcrichton @ubsan
mk: Use the right llvmdeps.rs file for cross build 
It looks like rust-lang#27937 accidentally switched the llvmdeps file from the target to
the host by accident, so be sure to use the right llvmdeps file which is built
for the target when building rustc_llvm
c4edb38

fhahn added a commit to fhahn/rust that referenced this pull request Dec 28, 2015

@alexcrichton @fhahn
mk: Use the right llvmdeps.rs file for cross build 
It looks like rust-lang#27937 accidentally switched the llvmdeps file from the target to
the host by accident, so be sure to use the right llvmdeps file which is built
for the target when building rustc_llvm
1b90dfb

@Zoxc Zoxc referenced this pull request Jan 21, 2016

Open

Linking to rustc_llvm with --llvm-root #30970

@sanxiyn sanxiyn referenced this pull request Jul 25, 2016

Closed

atomic-lock-free test doesn't account for LLVM support #35023

vivo75 added a commit to vivo75/vivovl that referenced this pull request Sep 6, 2017

@jauhien @vivo75
[dev-lang/rust] version bump 
LLVM in Gentoo is shipped as static libs only now,
until Rust upstream fixies linking using system wide LLVM is impossible
see rust-lang/rust#27937

-Werror during Rust building was disabled
fa69603
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.