Closed
Description
See #45225 (comment) for an old bug rearing its ugly head. cc @rust-lang/infra
Metadata
Metadata
Assignees
Labels
Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.Category: A feature request, i.e: not implemented / a PR.Relevant to the compiler team, which will review and decide on the PR/issue.Relevant to the infrastructure team, which will review and decide on the PR/issue.
Type
Projects
Milestone
Relationships
Development
No branches or pull requests
Activity
alexcrichton commentedon Oct 14, 2017
AFAIK we have no reason to stick to LLVM 3.7 other than "it's what we stuck to awhile ago". Along those lines I think it'd be fine to bump the minimum version to 3.9, it's already at least 2 releases old anyway.
I think so long as the builder with 3.7 pre-installed is updated to 3.9 we should be good to go!
petrochenkov commentedon Oct 14, 2017
cc @sanxiyn, due to #37912, any reasons to keep compatibility now?
Mark-Simulacrum commentedon Oct 14, 2017
It'd be good to reach out to distros as well, in case any of them need the LLVM 3.7 dependency, but I don't know how best to do that.
aidanhs commentedon Oct 15, 2017
Looking at some popular distros and versions of LLVM available:
So no reason to stick to LLVM 3.7 from that quick look.
est31 commentedon Oct 15, 2017
Which clang/gcc version does llvm 3.9 require? Is it newer or older than the one provided by centos 6? Also ping @infinity0 @cuviper
cuviper commentedon Oct 15, 2017
I think EL6's gcc is already too old for even LLVM 3.7. For internal builds I've been using devtoolset-gcc and just the bundled rust-llvm.
For EPEL7 I use 3.9, as you said, and same for Fedora 25. F26+ and the coming rust-toolset for RHEL7 are all on 4.0. So a minimum of 3.9 is fine for me.
infinity0 commentedon Oct 15, 2017
Sounds good to me - the Debian and Ubuntu rustc package already uses the same specific version of LLVM as what rustc uses, just minus most of the rust patches. (As opposed to the default Debian system version.)
TBH I was not even aware that Rust supported LLVM 3.7. Quite often some tests break when I run the build against an unpatched-but-otherwise-same-version of LLVM that rust uses - currently 4.0 - sometimes on tier 1 platforms. I just noticed
src/ci/
where there is one job to test builds against an Ubuntu system LLVM 3.7. I'm very surprised that this is even green, but given that you've had this for a while could I submit a PR to add another job that tests against Ubuntu's system LLVM 4.0?aidanhs commentedon Oct 15, 2017
@infinity0 my personal perspective is that tier 1 platforms implicitly include the LLVM version they're built with as part of the platform - I don't know if we want to gate on lots of LLVM versions on different distros given that the submodule is the only recommended one. Making sure things work on one vanilla LLVM with reasonably broad support (3.9 appears to be the inclination) seems like a reasonable compromise for the sake of convenience?
infinity0 commentedon Oct 16, 2017
Whose convenience, though? At the moment it's testing Ubuntu LLVM 3.7 from 16.04 and (if I understand correctly) the proposal is to change this to 3.9. But this doesn't match any distribution's current configuration - Ubuntu 16.04 already has LLVM 4.0 so the rustc package (which it takes from Debian) would eventually just use that instead of 3.9.
It seems very strange to gate merges on tests passing in Ubuntu's vanilla LLVM 3.9, when they don't necessarily pass on vanilla LLVM 4.0 which is the base of rust's fork, which is available on the newest (and not-so newest) suites of both Debian and Ubuntu.
CI testing with vanilla LLVM 3.9, would only help Ubuntu maintainers backport Debian unstable packages back to Ubuntu 14.04, and it would only help it in a very minor way because: (1) They build and test on 6 architectures. (2) They'd have to patch the Debian package to make it use an older LLVM version - not hard but a "paper-cut" time consumer. (3) The Debian package uses extra
./configure
settings not present in this rust CI test, so the test results might still fail when they come to it. Then (4) if you're serious about this particular case, you'd have to continue supporting LLVM 3.9 all the way to early 2019, otherwise it doesn't really save them any work - i.e. they would still end up having to backport LLVM 4.0 to 14.04 when you drop support for LLVM 3.9.Fedora currently ignore test failures, which I'm considering for Debian as well because it's too time-consuming to keep a clean sheet, and this logic would eventually make its way into Ubuntu. I believe all other distributions either don't run tests, or also ignore test failures. All of these factors IMO makes this whole "gating PR merges on LLVM 3.9 test success" a bit pointless.
To make it not pointless, it would be good to have some level of co-ordination between us all on which vanilla LLVM versions we should target and which tests failures (that occur on vanilla LLVM but pass on rust's fork) should be seen as unacceptable vs acceptable to ship. Debian/Ubuntu's LLVM is quite open to accepting backported patches that help rust - I've done this several times already. I'd guess other distributions would be similarly OK with these patches, if Rust formally states that they are "urgent". It's just really really hard to tell from a maintainer's perspective because I'm not super-involved with the details, and so my time incentives are pushing me to just simply ignoring test failures altogether.
infinity0 commentedon Oct 16, 2017
So as a proposal, maybe (a) add the ability for
min-llvm-version
to distinguish between vanilla LLVM and rust's fork, e.g.// min-llvm-version 4.0+rust
and (b) prioritise vanilla-but-current LLVM versions in CI tests because that is what most distributions are targeting, and where the "main work" is done. Then if resources allow, perhaps (c) have a second CI test for an older version but only gate it on the build succeeding and ignore any test failures - because that is what distributions are doing in practise anyways.eddyb commentedon Oct 16, 2017
I don't understand why the extreme complication or why Ubuntu in particular matters.
eddyb commentedon Oct 16, 2017
I proposed 3.9 because that's the first version which doesn't miscompile rustc itself aftee my changes. We need to test that minimum version, so we allocate one configuration to it (i.e. the one currently testing 3.7).
That's it. Nothing else in our policy changes. If I had any say, I'd suggest "just" dropping support for non-bundled LLVM as this sort of thing keeps happening, but some distros have minor (size?) concerns so it's not that easy.
16 remaining items