This is an archive of the discontinued LLVM Phabricator instance.

[asan] Fix recvfrom.cc testcase failure in large parallel tests run.
ClosedPublic

Authored by m.ostapenko on Feb 26 2016, 2:51 AM.

Details

Summary

This testcase currently fails on sanitizer-x86_64-linux buildbot with following error:

FAIL: AddressSanitizer-i386-linux :: TestCases/Linux/recvfrom.cc (524 of 1166)
******************** TEST 'AddressSanitizer-i386-linux :: TestCases/Linux/recvfrom.cc' FAILED ********************
Script:
--
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m32 /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/asan/TestCases/Linux/recvfrom.cc -o /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/recvfrom.cc.tmp && not  /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/recvfrom.cc.tmp 2>&1 | FileCheck /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/asan/TestCases/Linux/recvfrom.cc
--
Exit Code: 2

Command Output (stderr):
--
FileCheck error: '-' is empty.

--

I haven't managed reproduce this on my local box, but here my thoughts about the issue:
I believe this failure happens because of race on port 1234 between AddressSanitizer-i386-linux and AddressSanitizer-x86_64-linux instances of recvfrom.cc testcase. This patch tries to resolve the issue by trying to use another port if 1234 has already acquired.

Diff Detail

Repository
rL LLVM

Event Timeline

m.ostapenko retitled this revision from to [asan] Fix recvfrom.cc testcase failure in large parallel tests run..
m.ostapenko updated this object.
m.ostapenko added reviewers: kcc, samsonov, eugenis, dvyukov.
m.ostapenko set the repository for this revision to rL LLVM.
m.ostapenko added subscribers: llvm-commits, ygribov.
dvyukov added inline comments.Feb 26 2016, 2:58 AM
test/asan/TestCases/Linux/recvfrom.cc
16

s/kPortNum/portNum/
this is not a constant anymore

40

I don't see where the mutex is locked... where is it?

ygribov added inline comments.Feb 26 2016, 3:01 AM
test/asan/TestCases/Linux/recvfrom.cc
37

SO says (http://stackoverflow.com/questions/1075399/how-to-bind-to-any-available-port) that you can specify port 0 so that kernel selects it automatically for you.

m.ostapenko added inline comments.Feb 26 2016, 3:06 AM
test/asan/TestCases/Linux/recvfrom.cc
37

Nice, thanks!

40

Eh, It seems I attached truncated patch. Sorry about that.

ygribov added inline comments.Feb 26 2016, 3:10 AM
test/asan/TestCases/Linux/recvfrom.cc
50

Why not just init this with all zeros? Currently you implicitly rely on the fact that strlen("123456789") > kBufSize/2 which seems to be unnecessary.

51

I'm not sure you need trivial comments (especially given the self-descriptive name of the variable).

55

Format of messages varies across fprintfs. Probably makes sense to unify them (e.g. by wrapping fprintf + exit into a macro).

60

You don't seem to use this.

63

Shouldn't you abort here?

71

Spurious white before &? Same issue in other places.

Dmitry, Yura, thanks for your nits. I'm updating the diff.

ygribov added inline comments.Feb 26 2016, 6:38 AM
test/asan/TestCases/Linux/recvfrom.cc
20

Maybe wrap in do-while(0) just in case?

25

Still no agreement on ERROR vs. Error. I suggest to move prefix and newline to CHECK_ERROR macro.

30

Perhaps avoid htons?

34

Shouldn't mutexes be checked as well? BTW you could probably avoid mutexes alltogether if you moved server to main thread (and start client thread from it after socket is initialized).

52

Should probably initialialize to avoid UB.

68

I always thought that there's no need for explicit casts to starred voids (ditto for memset, memcpy, etc.).

82

Should this be checked?

Addressing Yura's nits.

m.ostapenko marked 18 inline comments as done.Feb 26 2016, 8:15 AM
dvyukov accepted this revision.Feb 26 2016, 8:43 AM
dvyukov edited edge metadata.

LGTM

This revision is now accepted and ready to land.Feb 26 2016, 8:43 AM

Thanks! I'm going to land this on Monday to be able make quick response on possible failures.

This revision was automatically updated to reflect the committed changes.
filcab added a subscriber: filcab.Feb 29 2016, 3:50 AM

Shouldn't all this be in TestCases/Posix?

compiler-rt/trunk/test/asan/TestCases/Linux/recvfrom.cc
16 ↗(On Diff #49340)

Maybe use perror?

We can even do something like:

#define CHECK_ERROR(p, name)                                                   \
  do {                                                                         \
    if (p) {                                                                   \
      perror(#name);                                                           \
      exit(1);                                                                 \
    } else {                                                                   \
      printf("%s succeeded.\n", #name);                                        \
    }                                                                          \
  }

...
CHECK_ERROR(succeeded < 0, getsockname);
// CHECK: getsockname succeeded.

That way, even though we're slightly more noisy when there's no error (and with the CHECK lines), you should immediately see the proper error string when the test fails, since FileCheck will fail on that line.
(Then again, FileCheck might show the appropriate line already, so no need to switch that part (except for perror).

Shouldn't all this be in TestCases/Posix?

We don't have (easy) access to Mac so we are really scared to break it...

compiler-rt/trunk/test/asan/TestCases/Linux/recvfrom.cc
16 ↗(On Diff #49340)

We thought about doing a more proper macro but later decided to not invest too much time in it. That's throwaway test code used in just one file anyway.