- User Since
- Aug 27 2014, 8:34 PM (242 w, 1 d)
Wed, Apr 17
Mon, Apr 15
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.
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.
Sat, Apr 13
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.
Wed, Apr 10
Fri, Apr 5
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.
Thu, Apr 4
Wed, Apr 3
@DiggerLin; as discussed off-list, I'll commit this for you with the additional change to include <string.h>.
Tue, Apr 2
Split changes for COMPILER_RT_UNITTEST_LINK_FLAGS out to D60143
Mon, Apr 1
Fri, Mar 29
LGTM with minor changes.
Mon, Mar 25
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.
Sat, Mar 23
Fri, Mar 22
Address review comments, add test, and apply style changes
Thanks @xingxue for the review. I will update for the error handling before committing.
Mar 18 2019
A second ping.
Mar 17 2019
Mar 15 2019
Mar 13 2019
Mar 12 2019
Mar 11 2019
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?
This leaves me wondering where the tests for this file are.
Mar 8 2019
@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.
Mar 7 2019
Mar 6 2019
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.
Mar 5 2019
Address review comment: Add note re: _GLIBCXX_RELEASE
Mar 4 2019
A friendly ping?
Mar 1 2019
I believe all comments on this have been addressed. This LGTM. I'll probably commit this on Monday.