[compiler-rt][builtins][sanitizers] Update compiler-rt test cases for compatibility with system's toolchain

[amyk]

Summary

On a platforms that we build LLVM on, we have some failing test cases that are related to the system's toolchain. This patch aims to:

• Explicitly add macro guards for a compiler_rt builtins test case
• Change a sanitizer test case to remove the use of std::string (this causes a false positive on our system)

The compiler-rt builtin test case, compiler_rt_logb_test.c compares its computed values to the values of its libm counterpart. Our version of libm seems to be older, and because of this dependency on libm, we see errors and inconsistencies. If a minimum version of GLIBC 2.23 is detected, then these tests are run. The version 2.23 was chosen because it is the version that I found has not been error-prone for these tests.

Another test is related to MSan/sanitizer-common (getpw_getgr.cc). There are two lines that are problematic. A global string is defined at the top of the file, is later assigned in a function, and then the string's c_str() becomes a parameter to a function afterwards. This may be related to using/not using the C++11 ABI, as the test case fails when _GLIBCXX_USE_CXX11_ABI=0 and succeeds when _GLIBCXX_USE_CXX11_ABI=1. We guard the problematic lines, such that we only execute the lines when the macro is set.
Edit (Apr 26): This could be a false positive. Instead of adding the checks for the GLIBCXX macro, we should adjust the test such that we avoid the use of std::string in this test case.

I've added some individuals who have worked with these test cases in the past as reviewers. I apologize if I missed anyone else. Please, if anyone has any comments or concerns on if these are suitable changes for these tests, I would greatly appreciate the feedback.

[hubert.reinterpretcast]

@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.

[hubert.reinterpretcast]

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.

The last category is of concern: Should a bad build of compiler-rt "pass" the tests? This is made more complicated if a good build for the same target can easily be built.

I have observed that projects/compiler-rt/test/builtins/Unit/POWERPC64LELinuxConfig/Output/compiler_rt_logb_test.c.tmp passes when deployed to a machine with newer runtime libraries; so, checking the GLIBC version when building the test seems to be appropriate for that particular test.

However, projects/compiler-rt/test/builtins/Unit/POWERPC64LELinuxConfig/Output/divsc3_test.c.tmp and projects/compiler-rt/test/builtins/Unit/POWERPC64LELinuxConfig/ppc/Output/qdiv_test.c.tmp fail when compiler-rt is built using GCC 4.8.5 and pass when compiler-rt is built on the same machine, targeting the same runtime, using Clang. This is even if the passing and failing test executables are built using the same Clang, targeting the same runtime (aside from compiler-rt). Checking the GLIBC version being used when a test is built seems to be an odd way suppress failures associated with the toolchain used when building compiler-rt. Indeed, with the bad compiler-rt build, the two tests fail even when compiled, linked, and run against GLIBC 2.26.

As for projects/compiler-rt/test/sanitizer_common/msan-powerpc64le-Linux/Posix/Output/getpw_getgr.cc.tmp, this does not appear to have anything to do with the GLIBC version. When the test is built using a toolchain where -D_GLIBCXX_USE_CXX11_ABI=1 is meaningful, the test passes when -D_GLIBCXX_USE_CXX11_ABI=0 is not set.

[amyk]

@hubert.reinterpretcast My apologies, you're right, I forgot to upload the full context. I will do that first, and address your comments afterwards.

[amyk]

I have forgot to attach the full context of my initial patch. I apologize for this.

[rupprecht]

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".

On the other hand, the upcoming policy change for minimum supported compiler versions seems to go further back (e.g. clang 3.5 was released in 2014), so maybe we should "support" glibc versions newer than it. It might be worth starting an RFC on llvm-dev for what other minimum versions are important.

Either way -- which logb test cases fail? (Can you -- locally -- modify test__compiler_rt_logb to return 0 even on errors so it prints out all failures). I'm curious if some bug was fixed, or maybe we're overtesting something.

[amyk]

@hubert.reinterpretcast Thank you for reviewing and bringing up these concerns, I do understand and you do have a point.

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

I could be mistaken, but doesn't the getpw_getgr test still succeed regardless if -D_GLIBCXX_USE_CXX11_ABI is set (within a toolchain that supports setting this)? Would it be better if we checked for the macro definition _GLIBCXX_USE_CXX11_ABI instead?

[amyk]

@rupprecht That is true. However, isn't it possible that a system's most up to date GLIBC version is still lower than say, GLIBC 2.23? Starting a conversation on which GLIBC versions are important can possibly be something that can be done.

In terms of the compiler_rt_logb test failures, I edited the test to return 0 to see the failures. I see about 104 lines that are outputted, so I've attached my results here.

compiler_rt_logb_results.txt13 KBDownload

[hubert.reinterpretcast]

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.

I could be mistaken, but doesn't the getpw_getgr test still succeed regardless if -D_GLIBCXX_USE_CXX11_ABI is set (within a toolchain that supports setting this)?

I am not sure if _GLIBCXX_USE_CXX11_ABI is 1 by default for all toolchains that support this value of the macro.

Would it be better if we checked for the macro definition _GLIBCXX_USE_CXX11_ABI instead?

That would make more sense, but is having the test "pass" the right thing to do? Users can set -D_GLIBCXX_USE_CXX11_ABI=0 for their applications. Is the failure caused by a genuine problem with libstdc++?

[hubert.reinterpretcast]

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.

[rupprecht]

In D60644#1467710, @amyk wrote:

@rupprecht That is true. However, isn't it possible that a system's most up to date GLIBC version is still lower than say, GLIBC 2.23? Starting a conversation on which GLIBC versions are important can possibly be something that can be done.

In terms of the compiler_rt_logb test failures, I edited the test to return 0 to see the failures. I see about 104 lines that are outputted, so I've attached my results here.

compiler_rt_logb_results.txt13 KBDownload

Interesting, it looks like an issue when handling subnormal values. You could probably limit this to skip test cases where x != 0.0 && (rep & exponentMask) == 0 if a bad glibc is detected, so that at least the normal floating point values are checked. I don't have an old glibc installed to try it out though :)

It might be overkill to do that though. I'm fine with the logb test change. Not familiar with the other tests.

[amyk]

Originally, this patch modified the following to check for the version of GLIBC:

• Compiler-RT builtins test cases: compiler_rt_logb_test.c, divsc3_test.c, ppc/qdiv_test.c
• Sanitizer test case: getpw_getgr.cc

Updated this patch to:

• Change only compiler-rt/test/builtins/Unit/compiler_rt_logb_test.c and compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc tests
• Guard the problematic lines of the getpw_getgr.cc test to check for the macro, _GLIBCXX_USE_CXX11_ABI instead, as this test no longer appears to be an issue related to the GLIBC version. If this macro is set to 1 (meaning, the C++11 ABI is used), then execute the lines
• Other compiler-rt builtin failures are avoided by building with a higher version of GCC, so I will not be modifying other tests

[amyk]

@rupprecht Thanks for the insight regarding compiler_rt_logb_test.c. I have updated the patch slightly, so I am no longer modifying the other compiler-rt builtins test. Does this update look more suitable? If you're OK with the current logb test, I can just keep it as is.

[hubert.reinterpretcast]

compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc

77 ↗

(On Diff #196306)

Both here and below, the test is skipped if _GLIBCXX_USE_CXX11_ABI is not defined at all (for example, if the C++ library is not libstdc++).

[amyk]

compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc

77 ↗

(On Diff #196306)

Yes, you are right. Thank you for pointing that out. Does something like this make sense, and is more suitable?

#if (defined(_GLIBCXX_USE_CXX11_ABI) && (_GLIBCXX_USE_CXX11_ABI)) || \             
    !defined(_GLIBCXX_USE_CXX11_ABI)                                               
  test<group>(&getgrnam, any_group.c_str());                                       
#endif

[hubert.reinterpretcast]

compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc

77 ↗

(On Diff #196306)

Yes. It seems verbose, but I guess the alternative would be a bit cryptic:

#if !defined(_GLIBCXX_USE_CXX11_ABI) || _GLIBCXX_USE_CXX11_ABI

[hubert.reinterpretcast]

compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc

76 ↗

(On Diff #196306)

Since we now have a better understanding of the issue, the solution seems to be that the test should not be using std::string.

diff -U0 a/getpw_getgr.cc b/getpw_getgr.cc
--- a/getpw_getgr.cc    2019-03-08 12:56:26.499543426 -0500
+++ b/getpw_getgr.cc    2019-04-25 10:06:56.240191290 -0400
@@ -11 +10,0 @@
-#include <string>
@@ -13 +12 @@
-std::string any_group;
+std::unique_ptr<char []> any_group;
@@ -51,2 +50,5 @@
-  if (any_group.empty())
-    any_group = result->gr_name;
+  if (!any_group) {
+    auto len = strlen(result->gr_name);
+    any_group.reset(new char[len + 1]);
+    memcpy(any_group.get(), result->gr_name, len + 1);
+  }
@@ -75 +77 @@
-  test<group>(&getgrnam, any_group.c_str());
+  test<group>(&getgrnam, any_group.get());
@@ -94 +96 @@
-  test_r<group>(&getgrnam_r, any_group.c_str());
+  test_r<group>(&getgrnam_r, any_group.get());

[amyk]

compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc

76 ↗

(On Diff #196306)

Yes. I saw the reply on the mailing list; thank you for that. I was going to look into something like this. I will try these changes and report back.

This could be a silly question, but are there any other alternatives to unique_ptr, or is there a specific reason we should use this?

[hubert.reinterpretcast]

compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc

76 ↗

(On Diff #196306)

unique_ptr is resource leak resistant. That is why using it is a good idea.

[amyk]

With the assistance of @hubert.reinterpretcast, I have updated the sanitizer-common test case, getpw_getgr.cc to avoid the use of std::string as this can result in a false positive.

The compiler_rt_logb_test.c test case remains the same; with checks for the GLIBC version macro in place.

[hubert.reinterpretcast]

compiler-rt/test/builtins/Unit/compiler_rt_logb_test.c

42 ↗

(On Diff #196778)

I think the test should continue to run on non-GLIBC systems.

  // Do not the run the compiler-rt logb test case if using GLIBC version
  // < 2.23. Older versions might not compute to the same value as the
  // compiler-rt value.
#if !defined(__GLIBC__) || (defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 23))

The __GLIBC_PREREQ checking used in the above is based on how compiler-rt/test/sanitizer_common/TestCases/Linux/mprobe.cc uses that macro.

[amyk]

Updated the compiler_rt_logb_test.c to:

• run on non-GLIBC systems
• consistent use of _GLIBC_PREREQ macros in compiler-rt

Thank you @hubert.reinterpretcast for the assistance once again.

[amyk]

compiler-rt/test/builtins/Unit/compiler_rt_logb_test.c

42 ↗

(On Diff #196778)

Yeah, what you're saying makes sense. I have updated the patch to reflect this and for more consistent use of the _GLIBC_PREREQ macro. Thank you.

[hubert.reinterpretcast]

LGTM.

[rupprecht]

Thanks!

compiler-rt/test/builtins/Unit/compiler_rt_logb_test.c

64 ↗

(On Diff #196914)

Can you include a log statement that the test isn't being run?

#else
    printf("skipped\n");
#endif

[amyk]

compiler-rt/test/builtins/Unit/compiler_rt_logb_test.c

64 ↗

(On Diff #196914)

Yes, I will add that. Thanks.

compiler-rt/trunk/test/builtins/Unit/compiler_rt_logb_test.c

 }
 
 double cases[] = {
     1.e-6, -1.e-6, NAN, -NAN, INFINITY, -INFINITY, -1,
     -0.0,  0.0,    1,   -2,   2,        -0.5,      0.5,
 };
 
 int main() {
+  // Do not the run the compiler-rt logb test case if using GLIBC version
+  // < 2.23. Older versions might not compute to the same value as the
+  // compiler-rt value.
+#if !defined(__GLIBC__) || (defined(__GLIBC_PREREQ) && __GLIBC_PREREQ(2, 23))
   const unsigned N = sizeof(cases) / sizeof(cases[0]);
   unsigned i;
   for (i = 0; i < N; ++i) {
     if (test__compiler_rt_logb(cases[i])) return 1;
   }
 
   // Test a moving 1 bit, especially to handle denormal values.
   // Test the negation as well.
   rep_t x = signBit;
   while (x) {
     if (test__compiler_rt_logb(fromRep(x))) return 1;
     if (test__compiler_rt_logb(fromRep(signBit ^ x))) return 1;
     x >>= 1;
   }
   // Also try a couple moving ones
   x = signBit | (signBit >> 1) | (signBit >> 2);
   while (x) {
     if (test__compiler_rt_logb(fromRep(x))) return 1;
     if (test__compiler_rt_logb(fromRep(signBit ^ x))) return 1;
     x >>= 1;
   }
+#else
+  printf("skipped\n");
+#endif
 
   return 0;
 }

compiler-rt/trunk/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc

 // RUN: %clangxx %s -o %t && %run %t
 // UNSUPPORTED: ios
 
 #include <assert.h>
 #include <grp.h>
 #include <memory>
 #include <pwd.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-#include <string>
 
-std::string any_group;
+std::unique_ptr<char []> any_group;
 const int N = 123456;
 
 void Check(const char *str) {
   if (!str)
     return;
   assert(strlen(str) != N);
 }
 

 }
 
 void Check(const group *result) {
   Check(result->gr_name);
   Check(result->gr_passwd);
   assert(result->gr_gid != N);
   for (char **mem = result->gr_mem; *mem; ++mem)
     Check(*mem);
-  if (any_group.empty())
-    any_group = result->gr_name;
+  if (!any_group) {
+    auto length = strlen(result->gr_name);
+    any_group.reset(new char[length + 1]);
+    memcpy(any_group.get(), result->gr_name, length + 1);
+  }
 }
 
 template <class T, class Fn, class... Args>
 void test(Fn f, Args... args) {
   T *result = f(args...);
   Check(result);
 }
 
 template <class T, class Fn, class... Args>
 void test_r(Fn f, Args... args) {
   T gr;
   T *result;
   char buff[10000];
   assert(!f(args..., &gr, buff, sizeof(buff), &result));
   Check(&gr);
   Check(result);
 }
 
 int main(int argc, const char *argv[]) {
   test<passwd>(&getpwuid, 0);
   test<passwd>(&getpwnam, "root");
   test<group>(&getgrgid, 0);
-  test<group>(&getgrnam, any_group.c_str());
+  test<group>(&getgrnam, any_group.get());
 
 #if !defined(__ANDROID__)
   setpwent();
   test<passwd>(&getpwent);
   setgrent();
   test<group>(&getgrent);
 
 #if !defined(__APPLE__)
   setpwent();
   test_r<passwd>(&getpwent_r);
   setgrent();
   test_r<group>(&getgrent_r);
 #endif
 
   test_r<passwd>(&getpwuid_r, 0);
   test_r<passwd>(&getpwnam_r, "root");
 
   test_r<group>(&getgrgid_r, 0);
-  test_r<group>(&getgrnam_r, any_group.c_str());
+  test_r<group>(&getgrnam_r, any_group.get());
 
 #if defined(__linux__)
   auto pwd_file = [] {
     return std::unique_ptr<FILE, decltype(&fclose)>(fopen("/etc/passwd", "r"),
                                                     &fclose);
   };
   auto gr_file = [] {
     return std::unique_ptr<FILE, decltype(&fclose)>(fopen("/etc/group", "r"),