[BUG] aiaiai-test-patchset removes obj dirs too early

[Brian Norris]

Hi,

This one has been a productivity staller, now that I'm finding machines
that will expose the race condition... it looks like this commit is very
broken:

commit 2115f7ff059260732208d9330d8d8db17734dfab
Author: Robert Jarzmik <robert.jarzmik at free.fr>
Date:   Thu Nov 6 20:46:50 2014 +0100

    aiaiai-test-patchset: optimize disk space

It tries to remove the obj directories as soon as the before + after
builds are done, but it overlooks the fact that the bisectability test
is being forked in the background.

How would y'all recommend fixing this? I can see two options:

1. revert the "optimization"
2. sync with (i.e., 'wait') the bisectability test within
   test_configuration(), before removing the build artifacts

According to one test, option 1 works fine.

However, the parallelism that exposed this bug looks like a problem too,
so I suppose it's better to fix this properly, a la option 2. Patch
below. I'm not sure if it has any performance implications, or if it's
really just a good patch to fix up a flaw in the design. Comment away!

Regards,
Brian

Subject: [PATCH] aiaiai-test-patchset: sync with bisection test before moving
 on

For whatever reason (parallelism, I suppose?) we fork the bisectability
test to the background and continue -- we only sync with this test after
all configs are completed. This is bad for a number of reasons.

1. commit 2115f7ff0592 ("aiaiai-test-patchset: optimize disk space")
   introduced a race condition, where we might try to blow away the
   build objects before the bisection test is complete. This is really bad.

2. Theoretically, we could have more than one bisection test forked to
   the background, if there are more than 1 running config. But we
   clobber $pid_bisect every time we run test_configuration(), so we
   effectively only sync with the last one. This is *usually* OK,
   because the prior config can likely complete by the time subsequent
   config(s) complete, but that's still technically a race.

So, let's move the 'wait $pid_bisect' into the test_configuration()
function, to make sure the test is complete before we move to the next
config (and remove the build objects). While this removes a little bit
of parallelism, I suspect that parallelism was unintentional.

Signed-off-by: Brian Norris <computersforpeace at gmail.com>
---
 aiaiai-test-patchset | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/aiaiai-test-patchset b/aiaiai-test-patchset
index 85fd0575dd4b..92ca3dfe8a40 100755
--- a/aiaiai-test-patchset
+++ b/aiaiai-test-patchset
@@ -248,6 +248,10 @@ test_configuration()
 	aiaiai-diff-log $verbose $preserve "$tmpdir/diff-for-diff-log" "$log1.stderr.log" \
 		        -w "$tmpdir" "$log2.stderr.log" > "$tmpdir/$config.stderr.diff"
 
+	# MUST sync with bisect test before removing build artifacts
+	[ -z "$pid_bisect" ] || wait "$pid_bisect" || die "aiaiai-test-bisectability failed"
+	verbose "Done with bisectability test"
+
 	if [ -n "$preserve" ]; then
 		message "Preserving objdirs: $obj1 $obj2"
 	else
@@ -537,7 +541,6 @@ done
 
 [ -z "$checkpatch" ]   || wait "$pid_checkpatch" || die "checkpatch failed"
 [ -z "$keywords" ]     || wait "$pid_keywords" || die "aiaiai-match-keywords failed"
-[ -z "$pid_bisect" ]   || wait "$pid_bisect"   || die "aiaiai-test-bisectability failed"
 
 # Print the results
 
-- 
2.5.0.rc2.392.g76e840b