[BUG] aiaiai-test-patchset removes obj dirs too early
computersforpeace at gmail.com
Tue Aug 4 16:10:55 PDT 2015
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
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!
Subject: [PATCH] aiaiai-test-patchset: sync with bisection test before moving
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
@@ -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"
@@ -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
More information about the aiaiai