[TSAN] Parameterize the hard-coded threshold of deflake in tsan test

[anhtuyen]

Summary

A number of testcases in this bucket such as ThreadSanitizer-powerpc64le :: atomic_free3.cpp are designed to deal with intermittent problems. The expected problem does not exist in all executions of the program being tested. The authors of the tests are aware of the issue. They use a script called deflake.bash which runs the executable up to 10 times to deal with the intermittent nature of the tests. By performing more executions, they increase the chances of having at least one of the executions expose the targeted issue.

On test machines with heavier load, the hard-coded threshold of 10 is not sufficient. The intermittent issue is more likely to be hit when the process is on a quiet machine because its threads gets cycles more often. Thus, on a machine with much more active processes, the condition being checked for is less likely to happen on any given execution.

The purpose of this patch is to parameterize the hard-coded threshold used by the compiler-rt/test/tsan/deflake.bash script. The changes to CMakeLists and LIT configuration files are to enable the users to configure the deflake threshold by specifying a positive integer value via a CMake variable. This will be used in the case that a deflake threshold other than 10 is more desirable for their test environment.

--cmake_variables=-D'TSAN_TEST_DEFLAKE_THRESHOLD=<integerValue>'

When the cmake-variable is not defined, the existing value of 10 will be used.

[rnk]

More than 10 retries seems excessive. For this particular issue, I think it would be reasonable to mark the test ; UNSUPPORTED: powerpc64. Adding more retries will make the powerpc64 bots slower and less useful.

[anhtuyen]

In D73707#1850128, @rnk wrote:

More than 10 retries seems excessive. For this particular issue, I think it would be reasonable to mark the test ; UNSUPPORTED: powerpc64. Adding more retries will make the powerpc64 bots slower and less useful.

Thanks for your opinion Reid @rnk, but I disagree with your comment based on the following points.
(1) This is to make the value configurable in order to enable users who see the needs to adjust the deflake threshold to a value more suitable for their environment. The existing threshold of 10 is the default and thus there should be no impact to other bots.
(2) I have run the entire LIT (check-all) more than 10 times on the same dedicated machine, and the results are not much slower at all. On average, the time for the entire LIT will increase by 0.26 second if the threshold is 100, and 0.48s for 200.

	Threshold of 200	Threshold of 100	Threshold of 10 (Existing Value)
1st	124.04s	123.65s	122.64s
2nd	122.93s	123.12s	122.49s
3rd	123.33s	123.26s	123s
4th	123.21s	122.24s	121.93s
5th	122.6s	123.32s	122.48s
6th	123.29s	122.42s	123.02s
7th	123.01s	122.89s	122.37s
8th	122.57s	122.42s	122.63s
9th	122.61s	122.85s	122.6s
10th	123.34s	122.19s	122.95s
11th	122.98s	123.09s	122.47s
Average (in seconds)	123.08s	122.86s	122.6s
Increase in seconds vs baseline	0.48s	0.26s	(baseline)

[rnk]

I think what you are doing is reasonable and practical. I only felt obligated to make the philosophical point that flaky tests are bad, and nobody other than the test owner should have to spend time on them.

In the meantime, I think this is pretty reasonable. Maybe @vitalybuka can help review it.

[anhtuyen]

In D73707#1850870, @rnk wrote:

I think what you are doing is reasonable and practical. I only felt obligated to make the philosophical point that flaky tests are bad, and nobody other than the test owner should have to spend time on them.

In the meantime, I think this is pretty reasonable. Maybe @vitalybuka can help review it.

Thank you very much for your nice word, Reid @rnk . I am very new to LLVM and I hope I can learn more from experts like you.

[hubert.reinterpretcast]

compiler-rt/test/tsan/deflake.bash

13

Please add quotes around variable expansion here and for further expansions of THRESHOLD.

16

Minor nit: It seems that the usual style uses a space after #?

17

Suggest to use || instead of using ! with &&.

19

This can be

while (( THRESHOLD-- )); do

[anhtuyen]

Thank you very much Hubert @hubert.reinterpretcast for your comments. I have changed the script accordingly.

[vitalybuka]

LGTM

compiler-rt/test/tsan/deflake.bash

17

why do you need this?

I'd expect

for i in $(seq 1 ${THRESHOLD}); do

already exits with error

[anhtuyen]

compiler-rt/test/tsan/deflake.bash

17

Hi Vitaly @vitalybuka
Thank you very much for your review and acceptance.
The condition

[[  "${THRESHOLD}" =~ ^[0-9]+$ ]] || exit 1

is to exit early for cases such as that of a negative value.

[delcypher]

compiler-rt/test/tsan/lit.cfg.py

78

@anhtuyen This is breaking our tests. There's a missing space at the end of the substitution which means the command produced is something like

deflake.bash 10/path/to/binary

when it should be

deflake.bash 10 /path/to/binary

[anhtuyen]

compiler-rt/test/tsan/lit.cfg.py

78

Hello @delcypher
This does not happen on our machines, but sorry for breaking your test. The fix is so simple that I can commit it in a few minutes. However, can you give me some info about your machine so that I can test it (if I have a similar one) before pushing the fix.

[delcypher]

compiler-rt/test/tsan/lit.cfg.py

78

I've landed a quick fix in ddd2257f48a83843aa35e7cbd337c3fe0c103598

[anhtuyen]

compiler-rt/test/tsan/lit.cfg.py

78

Yes, mine collided it with yours above! Thank you!

compiler-rt/test/tsan/CMakeLists.txt

if(COMPILER_RT_LIBCXX_PATH AND
   list(APPEND TSAN_TEST_DEPS libcxx_tsan)
   set(TSAN_HAS_LIBCXX True)
 else()
   set(TSAN_HAS_LIBCXX False)
 endif()
 
 set(TSAN_TESTSUITES)
 
+if (NOT DEFINED TSAN_TEST_DEFLAKE_THRESHOLD)
+  set(TSAN_TEST_DEFLAKE_THRESHOLD "10")
+endif()
+
 set(TSAN_TEST_ARCH ${TSAN_SUPPORTED_ARCH})
 if(APPLE)
   darwin_filter_host_archs(TSAN_SUPPORTED_ARCH TSAN_TEST_ARCH)
 endif()
 
 foreach(arch ${TSAN_TEST_ARCH})
   set(TSAN_TEST_APPLE_PLATFORM "osx")
 

compiler-rt/test/tsan/deflake.bash

 #!/usr/bin/env bash
 # This script is used to deflake inherently flaky tsan tests.
 # It is invoked from lit tests as:
-# %deflake mybinary
+# %deflake $THRESHOLD  mybinary
 # which is then substituted by lit to:
-# $(dirname %s)/deflake.bash mybinary
-# The script runs the target program up to 10 times,
+# $(dirname %s)/deflake.bash $THRESHOLD mybinary
+# - When TSAN_TEST_DEFLAKE_THRESHOLD is defined to a positive integer value,
+#   THRESHOLD will be the defined value.
+# - When TSAN_TEST_DEFLAKE_THRESHOLD is not defined, THRESHOLD will be 10.
+# The script runs the target program up to $THRESHOLD times,
 # until it fails (i.e. produces a race report).
 
-for i in $(seq 1 10); do
+THRESHOLD=${1}
+shift
+
+#Early exit if $THRESHOLD is not a non-negative integer
+[[ !  ${THRESHOLD} =~ ^[0-9]+$ ]] && exit 1
+
+for i in $(seq 1 ${THRESHOLD}); do
   OUT=`$@ 2>&1`
   if [[ $? != 0 ]]; then
     echo "$OUT"
     exit 0
   fi
 done
 exit 1

compiler-rt/test/tsan/lit.cfg.py

def build_invocation(compile_flags):
   return " " + " ".join([config.clang] + compile_flags) + " "
 
 config.substitutions.append( ("%clang_tsan ", build_invocation(clang_tsan_cflags)) )
 config.substitutions.append( ("%clangxx_tsan ", build_invocation(clang_tsan_cxxflags)) )
 
 # Define CHECK-%os to check for OS-dependent output.
 config.substitutions.append( ('CHECK-%os', ("CHECK-" + config.host_os)))
 
-config.substitutions.append( ("%deflake ", os.path.join(os.path.dirname(__file__), "deflake.bash") + " "))
+config.substitutions.append( ("%deflake ", os.path.join(os.path.dirname(__file__), "deflake.bash") + " " + config.deflake_threshold))
 
 # Default test suffixes.
 config.suffixes = ['.c', '.cpp', '.m', '.mm']
 
 if config.host_os not in ['FreeBSD', 'Linux', 'Darwin', 'NetBSD']:
   config.unsupported = True
 
 if config.android:
   config.unsupported = True
 
 if not config.parallelism_group:
   config.parallelism_group = 'shadow-memory'
 
 if config.host_os == 'NetBSD':
   config.substitutions.insert(0, ('%run', config.netbsd_noaslr_prefix))

compiler-rt/test/tsan/lit.site.cfg.py.in

 @LIT_SITE_CFG_IN_HEADER@
 
 config.name_suffix = "@TSAN_TEST_CONFIG_SUFFIX@"
 config.tsan_lit_source_dir = "@TSAN_LIT_SOURCE_DIR@"
 config.has_libcxx = @TSAN_HAS_LIBCXX@
 config.apple_platform = "@TSAN_TEST_APPLE_PLATFORM@"
 config.target_cflags = "@TSAN_TEST_TARGET_CFLAGS@"
 config.target_arch = "@TSAN_TEST_TARGET_ARCH@"
+config.deflake_threshold = "@TSAN_TEST_DEFLAKE_THRESHOLD@"
 
 # Load common config for all compiler-rt lit tests.
 lit_config.load_config(config, "@COMPILER_RT_BINARY_DIR@/test/lit.common.configured")
 
 # Load tool-specific config that would do the real work.
 lit_config.load_config(config, "@CMAKE_CURRENT_SOURCE_DIR@/lit.cfg.py")