In D60644#1466135, @rupprecht wrote:

Part of me thinks this is bad -- if the version of glibc is old, then the host toolchain should just upgrade it. glibc 2.23 was released in 2016-02-19, so I think using something older than that is on the border of being called "not a modern c++ toolchain".

We talk of needing a "modern C++ toolchain" for building the project; however, in the case of compiler_rt_logb_test, we are talking not of the build environment, but rather the supporting libraries on a system where compiler-rt may be deployed. Perhaps the tests are currently set up to run in the build environment, but is not a stretch to imagine that the tests can be run as part of an installer for deploying the build.

In D60644#1467676, @amyk wrote:

In terms of qdiv_test and divsc3_test, do you think checking perhaps checking the version of GCC is a more suitable fix?

The current pattern being used in the patch reports tests as passing when failures are being suppressed. An approach using XFAIL (based on the GCC version used to build compiler-rt) would be more suitable. The failure should also be filed as a bug (and the bug mentioned when adding the XFAIL) since the affected version of GCC is still currently supported as a build compiler (albeit deprecated). I am not sure if there is any motivation to really fix said bug; but, if there is no such motivation, then I also question the motivation behind building with the affected version of GCC and the motivation for suppressing the failures when such a build is done. In other words, if your goal is to build with a compiler that causes these test failures, then what needs to be done in the end is to implement a workaround in the compiler-rt source (and not the tests) for the build compiler problem. If people are happy to just avoid building with the affected version of GCC, then I think it makes sense to just leave these two tests alone.

There may be different levels of failures in play:

• Correct compiler-rt behaviour with:
  • Bad build environment for the test: Building the test against a compatible newer toolchain and the same compiler-rt would result in passing behaviour.
  • Bad run environment for the test: In this case, deploying the test executable to a system with sufficiently new runtime libraries would result in passing behaviour.
• Incorrect compiler-rt behaviour due to a bad build environment: In this case, the test executable will intrinsically fail.

@amyk; the patch as posted is missing context lines. Please refer to http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

@pjeeva01, please post the delta assuming a base that has D60211 incorporated.

In D58951#1456409, @peter.smith wrote:

As I understand it compiler-rt is built by make/ninja from CMake derived files so CMake only features such as CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT will affect the make/ninja build files. The tests are run by llvm-lit so we wouldn't expect CMake only features to affect the output unless we convert them to something llvm-lit understands. We've also got a situation where cross-compiling and native tests have different options available to them:

• Cross compiled test runs can use COMPILER_RT_TEST_COMPILER_CFLAGS to pass in --gcc-toolchain=... --sysroot=... duplicating any use of CMAKE_C(XX)_EXTERNAL_TOOLCHAIN and CMAKE_SYSROOT, but at least it is possible.

That is also my understanding. I also believe that it is working sufficiently today for people using such configurations, or at least sufficiently so that improving the situation was not seen as being urgent.

@DiggerLin; as discussed off-list, I'll commit this for you with the additional change to include <string.h>.

Split changes for COMPILER_RT_UNITTEST_LINK_FLAGS out to D60143

In D58951#1451485, @peter.smith wrote:

If I've understood this correctly the patch introduces COMPILER_RT_TOOLCHAIN_CFLAGS which we don't expect users to set directly, but instead set -gcc-toolchain if the compiler is clang and CMAKE_CXX_COMPILER_EXTERNAL_TOOLCHAIN is set?

Yes.

LGTM with minor changes.

Ping 3. We've been using this locally to support our build environments with non-default native toolchains. I believe this patch is generally applicable for such a use case.

@xingxue, thanks for the review. I've updated the patch; please let me know if you have concerns with the updates. I'll probably commit this later this week.

Address review comments, add test, and apply style changes

Thanks @xingxue for the review. I will update for the error handling before committing.

A second ping.

@asb @lhames @majnemer, based on D36826, I believe that your guidance would be helpful here regarding the use of llvm_unreachable, report_fatal_error, and assert.

I believe that the conditions being checked for with llvm_unreachable in this patch are of the debug-only variety; however, some input would be appreciated regarding the choice of using llvm_unreachable instead of report_fatal_error or assert.

I think all comments have been addressed or otherwise responded to.
@jasonliu: Since this patch is dependent on D58930, I would suggest that you commit it after D58930. A friendly reminder to include the patch attribution for @andusy. @apaprocki, and @WuZhao. Thanks.

A friendly ping?

In D58250#1425142, @joerg wrote:

Why do you exclude clang? I would expect LLVM at this point to not support visibility attributes on XCOFF either?

This leaves me wondering where the tests for this file are.

@jasonliu, you have had a number of patches committed into the project already (D22698, D22702, D34649). Please go ahead with requesting commit access, and commit this patch with the additional fix when you are ready.

D59048 has been posted with some initial OSTargetInfo changes. Noticeable differences include not defining _ALL_SOURCE and focus on 64-bit long double. This is consistent with the invocation provided with the recent v16.1 release of the IBM XL C/C++ compiler for AIX that incorporates Clang-based technology.

I'd be happy to commit this patch later this week, but I would appreciate suggestions on alternative methods to suppress the "unused" warning for the internal-linkage helper function.

Ping. I am looking for some input on whether we want to pursue additional testing.

Address review comment: Add note re: _GLIBCXX_RELEASE

A friendly ping?

I believe all comments on this have been addressed. This LGTM. I'll probably commit this on Monday.