[libFuzzer] Improve fuzzer-jobs test for Posix.

[mpividori]

Summary

I update the test to check if the process is running after 2 seconds, with:

# Check if libFuzzer is still running.
RUN: kill -0 ${FUZZER_PID}

Before this diff, the test didn't check if libFuzzer is running after 2 seconds, so running it in background and waiting 2 second was useless.

[kcc]

I don't like this test as it is now, but I understand your change even less.
Note that LLVMFuzzer-EmptyTest runs forever until killed (or exhausts -max_total_time=4)

[mpividori]

@kcc No. It doesn't run until killed. It runs until timeout.
kill -0 ${FUZZER_PID}; only checks if the process if running and waits until it finishes with the while loop. But the condition could be false the first time you check. I mean, you are not checking if the process is still running after 2 seconds, you are only waiting.
Because of that, I modified the test to:

• First check if the process is running:

kill -0 ${FUZZER_PID}

This retuns non-zero value if the process is not running.

• Second, wait for the process to finish:

wait ${FUZZER_PID}

Instead of the while loop.

[mpividori]

@kcc Also, if we are not going to check if the process is still running after 2 seconds, it doesn't make sense to run it in background. We could just run it normally and remove the while loop that waits for the process to finish.

[delcypher]

In D29546#668145, @mpividori wrote:

@kcc No. It doesn't run until killed. It runs until timeout.

That's what @kcc said no?

kill -0 ${FUZZER_PID}; only checks if the process if running and waits until it finishes with the while loop. But the condition could be false the first time you check. I mean, you are not checking if the process is still running after 2 seconds, you are only waiting.
Because of that, I modified the test to:

• First check if the process is running:

kill -0 ${FUZZER_PID}

This retuns non-zero value if the process is not running.

Your description of what kill -O ${FUZZER_PID} is right but I think your change is wrong. If any RUN: lines fail then the test as a whole test will fail. That's how lit shell tests work. This means your change to the test will non-deterministically fail depending on when LLVMFuzzer-EmptyTest exits. We don't want that.

• Second, wait for the process to finish:

wait ${FUZZER_PID}

Instead of the while loop.

I didn't know about this shell built-in but again this change doesn't seem right to me. If wait is given a PID that has already finished then it will return a non-zero exit code and the test will fail. We don't want this.

[delcypher]

In D29546#668166, @mpividori wrote:

@kcc Also, if we are not going to check if the process is still running after 2 seconds, it doesn't make sense to run it in background. We could just run it normally and remove the while loop that waits for the process to finish.

@mpividori There is a very good reason for running the test in the background. You should read the commit message where this test was introduced (r278544). It exists to check for a bug that used to exist in LibFuzzer on macOS where workers
would not actually run in parallel and instead would run sequentially. This tries to test for this bad behaviour by starting a run of libFuzzer and while it is running checking that more than one log file exists (in the older buggy version of libfuzzer on macOS only fuzz-0.log would exist in the
initial 4 second window and then after that fuzz-1.log would be created).

This of course means this test is racey (We are hoping that all worker processes have been started within approximately two seconds). I'm not very happy with this test due to its racey behaviour it but I couldn't come up with a better way to test it from lit because it requires observing what LLVMFuzzer-EmptyTest is doing while it is running (rather than checking it's output afterwards which is how we normally write lit tests).

I will happily review a replacement (or even removal) of this test but I don't think your changes improves how things are currently.

Aside: The additional REQUIRES: posix seems like a good idea provided that actually is a feature name provided by LibFuzzer's test configuration on POSIX platforms. I've not checked that it is.

[mpividori]

kill -0 ${FUZZER_PID}; only checks if the process if running and waits until it finishes with the while loop. But the condition could be false the first time you check. I mean, you are not checking if the process is still running after 2 seconds, you are only waiting.
Because of that, I modified the test to:

• First check if the process is running:

kill -0 ${FUZZER_PID}

This retuns non-zero value if the process is not running.

Your description of what kill -O ${FUZZER_PID} is right but I think your change is wrong. If any RUN: lines fail then the test as a whole test will fail. That's how lit shell tests work. This means your change to the test will non-deterministically fail depending on when LLVMFuzzer-EmptyTest exits. We don't want that.

LLVMFuzzer-EmptyTest won't exit. It is an empty test. So, libFuzzer will always run for 4 seconds.

• Second, wait for the process to finish:

wait ${FUZZER_PID}

Instead of the while loop.

I didn't know about this shell built-in but again this change doesn't seem right to me. If wait is given a PID that has already finished then it will return a non-zero exit code and the test will fail. We don't want this.

Ok. I understood that the purpose of this test was to ensure that libFuzzer was running 2 jobs after 2 seconds. So I supposed that was the reason to run the process in background.

With my changes, the test also checks that the process is still running after 2 seconds. But now that I see the purpose of this test was different, I can just abandon this diff.

Thanks for your time.

lib/Fuzzer/test/fuzzer-jobs.test

+REQUIRES: posix
+
 RUN: rm -rf %tmp
 RUN: mkdir %tmp && cd %tmp
 # Create a shared corpus directory
 RUN: rm -rf FuzzerJobsTestCORPUS
 RUN: mkdir FuzzerJobsTestCORPUS
 RUN: rm -f fuzz-{0,1}.log
 # Start fuzzer and in parallel check that the output files
 # that should be created exist.
 RUN: LLVMFuzzer-EmptyTest -max_total_time=4 -jobs=2 -workers=2 FuzzerJobsTestCORPUS > %t-fuzzer-jobs-test.log 2>&1 & export FUZZER_PID=$!
 # Wait a short while to give time for the child processes
 # to start fuzzing
 RUN: sleep 2
 # If the instances are running in parallel they should have created their log
 # files by now.
 RUN: ls fuzz-0.log
 RUN: ls fuzz-1.log
+# Check if libFuzzer is still running.
+RUN: kill -0 ${FUZZER_PID}
 # Wait for libfuzzer to finish.
-# This probably isn't portable but we need a way to block until
-# the fuzzer is done otherwise we might remove the files while
-# they are being used.
-RUN: while kill -0 ${FUZZER_PID}; do : ; done
+RUN: wait ${FUZZER_PID}
 RUN: rm -f fuzz-{0,1}.log
 RUN: rm -rf FuzzerJobsTestCORPUS
 RUN: FileCheck -input-file=%t-fuzzer-jobs-test.log %s
 RUN: rm %t-fuzzer-jobs-test.log
-RUN: cd ../
 
 CHECK-DAG: Job 0 exited with exit code 0
 CHECK-DAG: Job 1 exited with exit code 0