This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Introduce a vDSO aware timing function
ClosedPublic

Authored by cryptoad on Dec 12 2017, 10:20 AM.

Details

Summary

See D40657 & D40679 for previous versions of this patch & description.

A couple of things were fixed here to have it not break some bots.
Weak symbols can't be used with SANITIZER_GO so the previous version was
breakin TsanGo. I set up some additional local tests and those pass now.

I changed the workaround for the glibc vDSO issue: __progname is initialized
after the vDSO and is actually public and of known type, unlike
__vdso_clock_gettime. This works better, and with all compilers.

The rest is the same.

Diff Detail

Event Timeline

cryptoad created this revision.Dec 12 2017, 10:20 AM
krytarowski added inline comments.Dec 12 2017, 10:26 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
723

Can we restrict this symbol to Linux only?

cryptoad added inline comments.Dec 12 2017, 10:30 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
723

I can do that, just to make sure: since it's defined as weak, and the the BSD part will return before that check, shouldn't that work as in on NetBSD?

krytarowski added inline comments.Dec 12 2017, 10:33 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
723

I don't like pollution of symbols.. but I will try to test build it tonight.

cryptoad added inline comments.Dec 12 2017, 10:36 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
723

No worries, I'll hide it in the same define as !SANITIZER_GO.

cryptoad updated this revision to Diff 126582.Dec 12 2017, 10:46 AM
cryptoad marked 4 inline comments as done.

Moving the vDSO check under a SANITIZER_LINUX define.

alekseyshl added inline comments.Dec 12 2017, 11:32 AM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
747

Now it seems that it would be easier to parse if we separate implementations completely:

#if SANITIZER_LINUX && !SANITIZER_GO

...all the defs...

u64 MonotonicNanoTime() {
  timespec ts;
  if (CanUseVDSO()) {
    if (&real_clock_gettime)
      real_clock_gettime(CLOCK_MONOTONIC, &ts);
    else
      clock_gettime(CLOCK_MONOTONIC, &ts);
  } else {
    internal_clock_gettime(CLOCK_MONOTONIC, &ts);
  }
  return (u64)ts.tv_sec * (1000ULL * 1000 * 1000) + ts.tv_nsec;
}

#else // !(SANITIZER_LINUX && !SANITIZER_GO)

u64 MonotonicNanoTime() {
  timespec ts;
  internal_clock_gettime(CLOCK_MONOTONIC, &ts);
  return (u64)ts.tv_sec * (1000ULL * 1000 * 1000) + ts.tv_nsec;
}

#endif // SANITIZER_LINUX && !SANITIZER_GO

Also can do

u64 MonotonicNanoTime() {
  timespec ts;
  MonotonicNanoTimeImpl(&ts)
  return (u64)ts.tv_sec * (1000ULL * 1000 * 1000) + ts.tv_nsec;
}

and define inlined MonotonicNanoTimeImpls for each case, but that is up to you.

cryptoad updated this revision to Diff 126597.Dec 12 2017, 12:03 PM
cryptoad marked an inline comment as done.

As suggested, split the code cleanly between Linux-non-Go and the rest.

cryptoad updated this revision to Diff 126601.Dec 12 2017, 12:11 PM

Correct the last #endif comment of the file while I am at it, since it
didn't include SANITIZER_NETBSD.

krytarowski added inline comments.Dec 12 2017, 12:25 PM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
747

This split looks good!

alekseyshl accepted this revision.Dec 12 2017, 2:06 PM
alekseyshl added inline comments.
lib/sanitizer_common/sanitizer_linux_libcdep.cc
743

No action required at the moment, but this comment sounds a bit silly in a file named sanitizer_LINUX_libcdep.cc.

This revision is now accepted and ready to land.Dec 12 2017, 2:06 PM
cryptoad added inline comments.Dec 12 2017, 2:14 PM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
743

// BSDs & go maybe?

krytarowski added inline comments.Dec 12 2017, 2:18 PM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
743

I agree that this naming and OS-specific vs generic parts deserves refactoring. We are now adding here Solaris support in sanitizer_linux_libcdep.cc. D40898

cryptoad added inline comments.Dec 12 2017, 2:30 PM
lib/sanitizer_common/sanitizer_linux_libcdep.cc
743

Well given this new information I guess Non-Linux makes sense until some sort of refactor happens.

This revision was automatically updated to reflect the committed changes.