Page MenuHomePhabricator

Bump minimum toolchain version
ClosedPublic

Authored by jfb on Fri, Jan 25, 3:54 PM.

Details

Summary

The RFC on moving past C++11 got good traction:

http://lists.llvm.org/pipermail/llvm-dev/2019-January/129452.html

This patch therefore bumps the toolchain versions according to our policy:

llvm.org/docs/DeveloperPolicy.html#toolchain

We’ll only migrate to hard-error (and start using new features) when we get consensus to do so, at the earliest end of March 2019. The patch will allow us to start warning developers, especially those who only compile branches (in this case, LLVM 8).

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
smeenai added inline comments.Fri, Jan 25, 4:24 PM
CMakeLists.txt
26 ↗(On Diff #183647)

I meant a CMake option, similar to how LLVM_FORCE_USE_OLD_HOST_TOOLCHAIN (which appears unused now) is defined: https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/CMakeLists.txt$386-387

jfb updated this revision to Diff 183663.Fri, Jan 25, 4:55 PM
  • Use an option.
jfb marked 2 inline comments as done.Fri, Jan 25, 4:56 PM
jfb added inline comments.
CMakeLists.txt
26 ↗(On Diff #183647)

Ah gotcha! Fixed (and drive-by fix LLVM_FORCE_USE_OLD_HOST_TOOLCHAIN to LLVM_FORCE_USE_OLD_TOOLCHAIN).

jyknight added inline comments.Mon, Jan 28, 12:13 PM
docs/GettingStarted.rst
231 ↗(On Diff #183663)

I think we should keep the documentation saying the actual minimum version, too. Maybe:

We recommend (and will require in the next release) at least:
<new version list>

We require at least:
<old version list>

jfb updated this revision to Diff 183954.Mon, Jan 28, 1:43 PM
jfb marked an inline comment as done.
  • Mention older versions too.
jfb marked 2 inline comments as done.Mon, Jan 28, 1:43 PM
jfb added inline comments.
docs/GettingStarted.rst
231 ↗(On Diff #183663)

Great point, changed!

jyknight accepted this revision.Mon, Jan 28, 3:02 PM

OK, let's do it.

But better watch the buildbots to see if any get broken by the new requirements (hopefully we have at least one that will be?), and add the extra_cmake_args as needed.

This revision is now accepted and ready to land.Mon, Jan 28, 3:02 PM
jfb marked an inline comment as done.Mon, Jan 28, 3:05 PM

OK, let's do it.

But better watch the buildbots to see if any get broken by the new requirements (hopefully we have at least one that will be?), and add the extra_cmake_args as needed.

You mean: pass a cmake argument so the buildbots don't fail? That would seem to defeat the purpose, no?

hubert.reinterpretcast added inline comments.
cmake/modules/CheckCompilerVersion.cmake
65 ↗(On Diff #183954)

libstdc++4.8 should also warn now, right?

jfb updated this revision to Diff 183971.Mon, Jan 28, 3:36 PM
jfb marked 2 inline comments as done.
  • Detect libstdc++ version.
jfb added a comment.Mon, Jan 28, 3:36 PM

@jyknight can you confirm that this latest change works for you? I think it's exactly your environment :)

cmake/modules/CheckCompilerVersion.cmake
65 ↗(On Diff #183954)

The comment above was pretty bad... Updated to use __GLIBCXX__.

hubert.reinterpretcast marked an inline comment as done.Mon, Jan 28, 4:28 PM
hubert.reinterpretcast added inline comments.
cmake/modules/CheckCompilerVersion.cmake
65 ↗(On Diff #183954)

Thanks for the fix!

90 ↗(On Diff #183971)

Typo: "support.Ignoring".

jfb updated this revision to Diff 183986.Mon, Jan 28, 4:30 PM
  • Missing space.
jfb marked an inline comment as done.Mon, Jan 28, 4:30 PM
In D57264#1374606, @jfb wrote:

OK, let's do it.

But better watch the buildbots to see if any get broken by the new requirements (hopefully we have at least one that will be?), and add the extra_cmake_args as needed.

You mean: pass a cmake argument so the buildbots don't fail? That would seem to defeat the purpose, no?

We need to be aware of which of the llvm project's official buildbots need upgrading. If any are broken, ISTM that the best thing to do is to mark each such individual bot that needs to be upgraded by setting the the LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN cmake argument in the buildbot configuration, to allow them to continue to build. Then, we know where things stand, and over the next month, the owners can upgrade as necessary, without it being an emergency.

Well, really, we should probably have at least one bot using the old version intentionally (assuming there are any now), so we can continue testing it until it's desupported.

The other options -- leaving bots broken (and unable to test any other changes) until someone can get around to upgrading it, or reverting this commit until all bots all been upgraded -- seem strictly worse.

In D57264#1374659, @jfb wrote:

@jyknight can you confirm that this latest change works for you? I think it's exactly your environment :)

Well, the environment in question doesn't actually use CMake, so this change won't affect it.

danilaml added inline comments.
docs/GettingStarted.rst
236 ↗(On Diff #183986)

Above or below?

jfb updated this revision to Diff 184109.Tue, Jan 29, 9:35 AM
jfb marked 2 inline comments as done.
  • Above / below
erichkeane accepted this revision.Tue, Jan 29, 9:37 AM
In D57264#1374659, @jfb wrote:

@jyknight can you confirm that this latest change works for you? I think it's exactly your environment :)

Well, the environment in question doesn't actually use CMake, so this change won't affect it.

Ah right!

In D57264#1374606, @jfb wrote:

OK, let's do it.

But better watch the buildbots to see if any get broken by the new requirements (hopefully we have at least one that will be?), and add the extra_cmake_args as needed.

You mean: pass a cmake argument so the buildbots don't fail? That would seem to defeat the purpose, no?

We need to be aware of which of the llvm project's official buildbots need upgrading. If any are broken, ISTM that the best thing to do is to mark each such individual bot that needs to be upgraded by setting the the LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN cmake argument in the buildbot configuration, to allow them to continue to build. Then, we know where things stand, and over the next month, the owners can upgrade as necessary, without it being an emergency.

Well, really, we should probably have at least one bot using the old version intentionally (assuming there are any now), so we can continue testing it until it's desupported.

The other options -- leaving bots broken (and unable to test any other changes) until someone can get around to upgrading it, or reverting this commit until all bots all been upgraded -- seem strictly worse.

Maybe I'm missing something obvious... do we have a buildbot configuration? I just looked at some bots and I see them running cmake with e.g.:

cmake -DCMAKE_BUILD_TYPE=Release -DLLVM_BUILD_TESTS=ON -DLLVM_ENABLE_ASSERTIONS=OFF -DLLVM_OPTIMIZED_TABLEGEN=ON -DCLANG_BUILD_EXAMPLES=OFF -GNinja -DLLVM_ENABLE_WERROR=OFF -DCMAKE_INSTALL_PREFIX=../../install/stage1 '-DLLVM_LIT_ARGS="-v"' ../../llvm

or

cmake -GNinja '-DLLVM_ENABLE_PROJECTS=clang;lld' '-DLLVM_ENABLE_RUNTIMES=compiler-rt;libcxx;libcxxabi;libunwind' -DBOOTSTRAP_LLVM_ENABLE_LTO=OFF -DLLVM_ENABLE_LTO=OFF -DCMAKE_INSTALL_PREFIX=/b/fuchsia-x86_64-linux/llvm.install -DFUCHSIA_SDK=/b/fuchsia-x86_64-linux/fuchsia.sdk -C../llvm.src/clang/cmake/caches/Fuchsia.cmake -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_WERROR=ON -DLLVM_OPTIMIZED_TABLEGEN=ON ../llvm.src/llvm

To speed things up I just audited *all* bots in http://lab.llvm.org:8011/buildslaves (there's so many!). Some of them are down, I therefore have no idea what they run. Here are the bots that will definitely break, with their maintainers:

Galina Kistanova <gkistanova@gmail.com> @gkistanova
am1i-slv1 -- gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
as-bldslv4 -- Microsoft (R) Visual Studio (R) 2015 (14.0)
ps4-buildslave2 -- Microsoft (R) Visual Studio (R) 2015 (14.0)

Hexagon QA <llvm.buildmaster@quicinc.com> @kparzysz @colinl
hexagon-build-02 -- gcc (Ubuntu 4.9.2-10ubuntu13) 4.9.2
hexagon-build-03 -- gcc (Ubuntu 4.9.2-10ubuntu13) 4.9.2

Vitaly Buka <vitalybuka@google.com> @vitalybuka
sanitizer-buildbot6 -- gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4

Reid Kleckner <rnk@google.com> @rnk
sanitizer-windows -- Microsoft (R) Visual Studio (R) 2015 (14.0)

Ilia Taraban <mstester.llvm@gmail.com> @itaraban
windows7-buildbot -- Microsoft (R) Visual Studio (R) 2015 (14.0)

To expedite things I've tagged them above, and Im going to email them individually so that they either pass LLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN to their bots, update the bot to a newer compiler version, or entirely turn down their bot.

jfb added a comment.Wed, Jan 30, 12:54 PM

Update: all the bots *except* @gkistanova's and @itaraban's are updated.

jfb added a comment.Wed, Jan 30, 10:31 PM
In D57264#1377635, @jfb wrote:

Update: all the bots *except* @gkistanova's and @itaraban's are updated.

All the bots should now be updated! Unless there are any new comments, I'll commit this tomorrow.

rnk added a comment.Wed, Jan 30, 11:45 PM

I didn't finish updating sanitizer-windows and I have no more time to work on bot maintenance this week (trainings + new team member). If you want to set the cmake option to warn instead of letting the bot go red, I'd appreciate it.

hubert.reinterpretcast marked an inline comment as done.Thu, Jan 31, 6:32 AM
hubert.reinterpretcast added inline comments.
cmake/modules/CheckCompilerVersion.cmake
73 ↗(On Diff #184109)

I can find __GLIBCXX__ on GCC 6.x toolchains, but I haven't found it on older ones.

#include <ciso646>
extern char x[__GLIBCXX__]; // error: '__GLIBCXX__' was not declared in this scope
danilaml added inline comments.Thu, Jan 31, 6:51 AM
cmake/modules/CheckCompilerVersion.cmake
73 ↗(On Diff #184109)

It seems to be there but it looks like "#include <ciso646>" doesn't define it for older compilers for some reason.

cmake/modules/CheckCompilerVersion.cmake
73 ↗(On Diff #184109)

Yes, I see it now. The other thing is that maybe we could check the libstdc++ version for the other-than-known compilers as well (i.e., if we detect libstdc++, then check the version). Although that could be a later patch.

jfb updated this revision to Diff 184521.Thu, Jan 31, 9:08 AM
  • Use iosfwd instead of ciso646 to find GLIBCXX, and don't use an apostrophe in the error message.
jfb marked 3 inline comments as done.Thu, Jan 31, 9:10 AM

Interesting that libstdc++ wasn't including its own config file in ciso646! I tested it out in godbolt, and iosfwd seems to have that config in all the versions.

jfb added a comment.Thu, Jan 31, 9:12 AM
In D57264#1378370, @rnk wrote:

I didn't finish updating sanitizer-windows and I have no more time to work on bot maintenance this week (trainings + new team member). If you want to set the cmake option to warn instead of letting the bot go red, I'd appreciate it.

Apologies, I'd misunderstood our earlier conversation. Where would I make a change to affect your bots' cmake setup?

jfb added a comment.Thu, Jan 31, 10:33 AM
In D57264#1378812, @jfb wrote:
In D57264#1378370, @rnk wrote:

I didn't finish updating sanitizer-windows and I have no more time to work on bot maintenance this week (trainings + new team member). If you want to set the cmake option to warn instead of letting the bot go red, I'd appreciate it.

Apologies, I'd misunderstood our earlier conversation. Where would I make a change to affect your bots' cmake setup?

@jyknight directed me, here's the fix: D57525

jfb added a comment.Thu, Jan 31, 1:33 PM

Everything should now be in place. Speak soon or forever hold your peace 😉

Closed by commit rL352811: Bump minimum toolchain version (authored by jfb, committed by ). · Explain WhyThu, Jan 31, 3:12 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 31, 3:12 PM
jfb reopened this revision.Thu, Jan 31, 3:30 PM

Reverted in r352814 because some bots are still breaking. I'm contacting their authors and will try again soon.

This revision is now accepted and ready to land.Thu, Jan 31, 3:30 PM
jfb added a subscriber: arsenm.Thu, Jan 31, 3:43 PM

@arsenm pointed me at his output. At a minimum, check_cxx_source_compiles does a compile, so I need to add main to all of these! That'll fix some of the issues, but I think some of the bots were still sad even with that fix.

jfb updated this revision to Diff 184639.Thu, Jan 31, 3:47 PM
  • Add main to libstdc++ test.
jfb added a comment.Thu, Jan 31, 3:57 PM

I looked through the bots that failed, and it seems they were all hitting this. There might be bots that didn't pick up the change between my commit and the revert though. I'll wait a bit for the pipeline to clear, and I'll resubmit the patch with the fixed main.

Closed by commit rL352834: Bump minimum toolchain version (authored by jfb, committed by ). · Explain WhyThu, Jan 31, 8:33 PM
This revision was automatically updated to reflect the committed changes.
jfb reopened this revision.Thu, Jan 31, 8:44 PM

Reverted again in r352835, looks like we still have some sad bots. I'll look into it!

This revision is now accepted and ready to land.Thu, Jan 31, 8:44 PM
jfb updated this revision to Diff 184675.Thu, Jan 31, 8:52 PM
  • Relax libstdc++ check as suggested by @jyknight, it'll make odd configurations less sad.
Closed by commit rL352951: Bump minimum toolchain version (authored by jfb, committed by ). · Explain WhyFri, Feb 1, 9:15 PM
This revision was automatically updated to reflect the committed changes.
jfb reopened this revision.Fri, Feb 1, 10:01 PM

Reverted for (I hope) the last time because of:

polly-amd64-linux
polly-arm-linux

They both have old versions of libstdc++ and recent clang. Once fixed the toolchain bump should stick.

This revision is now accepted and ready to land.Fri, Feb 1, 10:01 PM
jfb added a comment.Mon, Feb 4, 11:30 AM
In D57264#1381951, @jfb wrote:

Reverted for (I hope) the last time because of:

polly-amd64-linux
polly-arm-linux

They both have old versions of libstdc++ and recent clang. Once fixed the toolchain bump should stick.

I haven't heard back from bot owners. I'll opt them out of the toolchain checks for now: https://reviews.llvm.org/D57701

In D57264#1383646, @jfb wrote:
In D57264#1381951, @jfb wrote:

Reverted for (I hope) the last time because of:

polly-amd64-linux
polly-arm-linux

They both have old versions of libstdc++ and recent clang. Once fixed the toolchain bump should stick.

I haven't heard back from bot owners. I'll opt them out of the toolchain checks for now: https://reviews.llvm.org/D57701

The bots haven't updated yet, I sent a ping to @gkistanova, @grosser and @adasgupt.

Closed by commit rL353374: Bump minimum toolchain version (authored by jfb, committed by ). · Explain WhyWed, Feb 6, 9:19 PM
This revision was automatically updated to reflect the committed changes.
jfb added a comment.Wed, Feb 6, 9:19 PM
In D57264#1385384, @jfb wrote:
In D57264#1383646, @jfb wrote:
In D57264#1381951, @jfb wrote:

Reverted for (I hope) the last time because of:

polly-amd64-linux
polly-arm-linux

They both have old versions of libstdc++ and recent clang. Once fixed the toolchain bump should stick.

I haven't heard back from bot owners. I'll opt them out of the toolchain checks for now: https://reviews.llvm.org/D57701

The bots haven't updated yet, I sent a ping to @gkistanova, @grosser and @adasgupt.

Bots were restarted, trying again...

jfb added a comment.Wed, Feb 6, 10:19 PM

Looks like clang-with-lto-ubuntu is still on an old version of GCC. Let's see if more bots are broken, I'm not sure how many stragglers we have.

jfb added a subscriber: tstellar.Thu, Feb 7, 2:07 PM

Regarding libstdc++: the check is currently conservatively correct, but not thorough enough, because older revisions of GCC still get bug fix release which pass the date check. Past GCC 7 we can use _GLIBCXX_RELEASE, but before this @tstellar points out how LLDB does it. I don't know if we have to fix this issue: we simply fail to warn a small subset of libstdc++ users.

In D57264#1389686, @jfb wrote:

Regarding libstdc++: the check is currently conservatively correct, but not thorough enough, because older revisions of GCC still get bug fix release which pass the date check. Past GCC 7 we can use _GLIBCXX_RELEASE, but before this @tstellar points out how LLDB does it. I don't know if we have to fix this issue: we simply fail to warn a small subset of libstdc++ users.

My local RHEL 7.5 machines all pass the date check by accident. The date shows as 20150623.