Page MenuHomePhabricator

[CMake] Fix LLVM build non-determinism on RHEL
ClosedPublic

Authored by amyk on Jul 16 2019, 1:33 PM.

Details

Summary

On RHEL, the OS tooling (ar, ranlib) is not deterministic by default. Therefore, we cannot get bit-for-bit identical builds.

The goal of this patch is that it adds the flags required to force determinism.

Diff Detail

Event Timeline

amyk created this revision.Jul 16 2019, 1:33 PM
llvm/cmake/modules/HandleLLVMOptions.cmake
155

Checking for > 2.22 introduces the possibility that the check passes for patch/fix levels of 2.22 when we actually cared about 2.23.

amyk marked an inline comment as done.Aug 20 2019, 6:48 AM
amyk added inline comments.
llvm/cmake/modules/HandleLLVMOptions.cmake
155

@hubert.reinterpretcast
The regex used only picks up the first two 2.22, 2.23, etc. It doesn't pick up any subversion, e.g. 2.23.1

Here's some example output:

$ ranlib --version
GNU ranlib (GNU Binutils for Ubuntu) 2.26.1

Within cmake, ([0-9]+\\.[0-9]+) will pick up 2.26, so the check would be correct. Is this an acceptable change?

This patch LGTM.

llvm/cmake/modules/HandleLLVMOptions.cmake
155

Yes; thanks for pointing that out. Sorry for the noise.

This revision is now accepted and ready to land.Aug 20 2019, 7:19 AM

I think it might be better checking ${CMAKE_RANLIB} rD t.a works, because the user may specify llvm-ar as CMAKE_RANLIB.

MaskRay added a subscriber: hans.Aug 20 2019, 7:54 AM

binutils 2.22 was released in 2011. Do we want to support binutils that old? If not, we should probably not do such change, but rather bump the version requirement https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

CC @hans why may know more about these problems.

hans added a reviewer: jfb.Aug 20 2019, 8:13 AM

binutils 2.22 was released in 2011. Do we want to support binutils that old? If not, we should probably not do such change, but rather bump the version requirement https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

CC @hans why may know more about these problems.

I don't really. JF is the master of supported versions of things.

tstellar added a subscriber: serge-sans-paille.
jfb added a comment.Aug 20 2019, 2:13 PM

binutils 2.22 was released in 2011. Do we want to support binutils that old? If not, we should probably not do such change, but rather bump the version requirement https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library

CC @hans why may know more about these problems.

I don't really. JF is the master of supported versions of things.

We have a process for toolchain bumps. Someone needs to write a proposal and shepherd it through. I’m on vacation, won’t be me 🙂

Which version of RHEL is this patch meant for? Since the jump to C++14, on RHEL <= 7, you will need to compile llvm with the devtoolset toolchain, which has a newer version of binutils. This fix is probably only needed on the release/9.x branch (which can still be built by the system toolchain on RHEL7) and not trunk.

Which version of RHEL is this patch meant for? Since the jump to C++14, on RHEL <= 7, you will need to compile llvm with the devtoolset toolchain, which has a newer version of binutils. This fix is probably only needed on the release/9.x branch (which can still be built by the system toolchain on RHEL7) and not trunk.

It is possible to build with Clang and libc++ and not use a newer version of binutils for trunk.

Which version of RHEL is this patch meant for? Since the jump to C++14, on RHEL <= 7, you will need to compile llvm with the devtoolset toolchain, which has a newer version of binutils. This fix is probably only needed on the release/9.x branch (which can still be built by the system toolchain on RHEL7) and not trunk.

It is possible to build with Clang and libc++ and not use a newer version of binutils for trunk.

Is this the use case this patch is intended to fix?

Which version of RHEL is this patch meant for? Since the jump to C++14, on RHEL <= 7, you will need to compile llvm with the devtoolset toolchain, which has a newer version of binutils. This fix is probably only needed on the release/9.x branch (which can still be built by the system toolchain on RHEL7) and not trunk.

It is possible to build with Clang and libc++ and not use a newer version of binutils for trunk.

Is this the use case this patch is intended to fix?

I may be out of date on the info, but earlier in the year, we did have both devtoolset and Clang+libc++ builds on RHEL <= 7.

amyk added a comment.Oct 15 2019, 12:34 PM

The intention was simply to fix non determinism when building on RHEL, and the version checks were simply to make sure that this patch only gets applied to versions of ar/ranlib that support it.

I didn't realize that the r option on ar also creates a new archive that we can simply test for support on, so I think simply removing all notions of versioning is the correct route to go. If any of the steps fail (non-support), it falls back to using the default so should not cause any new failures. Even with DTS7, we still do not get reproducible builds, so this patch should still apply to all branches.

amyk updated this revision to Diff 225241.Oct 16 2019, 9:21 AM

Updated with appropriate diff.

tstellar added inline comments.Oct 16 2019, 9:48 AM
llvm/cmake/modules/HandleLLVMOptions.cmake
149–150

Can you make this comment more specific about which versions of RHEL and also ar and randlib this is intended for? If we ever drop support for these older tools, this would make it easier to identify and remove this section.

Otherwise, LGTM.

amyk updated this revision to Diff 225493.Oct 17 2019, 12:11 PM

Updated with more specific comment.

amyk marked an inline comment as done.Oct 17 2019, 12:11 PM
amyk added inline comments.
llvm/cmake/modules/HandleLLVMOptions.cmake
149–150

Thanks for pointing that out. The comment has been updated.

amyk added a comment.Tue, Nov 12, 8:53 PM

Ping.

Does anyone else have any specific comments regarding this patch?

tstellar accepted this revision.Wed, Nov 13, 5:53 AM

LGTM. Should we also backport this to the 9.0 branch?

According to https://reviews.llvm.org/D64817#1638218 , shall we just push the change to the release/9.x branch? binutils 2.22 becomes more and more irrelevant.

amyk added a comment.Thu, Nov 21, 9:08 PM

@tstellar Yes, I will commit this patch and also open a bugzilla for back porting.

This revision was automatically updated to reflect the committed changes.