commit 83958e3345e5c07a7375f2dbd69e6b45fd0ac835
author Hans Boehm <hboehm@google.com> Fri Jul 08 17:17:47 2022 -0700
committer Hans Boehm <hboehm@google.com> Tue Jul 19 19:34:19 2022 +0000
tree 7493df11827bc80d8d41d377cc16ac06a5fd6c09
parent 78f3d3a7b2d795cc0915218abafb0d5b47aa2225

Remove bad code from ComputeRelativeTimeSpec

The test in the "else if" clause was off by one. But it should never be
exercised anyway. Replace it with a correct DCHECK.
Have ComputeRelativeTimeSpec return false, rather than true, on failure
to compute a valid timespec.

Update num_contenders if lock acquisition results in unexpected
failure. This matters only in that it may put us into a slightly
better state while logging debug information.

Fix an unrelated comment typo.

Bug: 238032384
Test: Build and boot AOSP eng build
Change-Id: Idf3a0b4c349168370079d24457ea134b06cf231e

runtime/alloc_instrumentation.md

diff --git a/runtime/alloc_instrumentation.md b/runtime/alloc_instrumentation.md
index 513bbe3..66e9a6a 100644
--- a/runtime/alloc_instrumentation.md
+++ b/runtime/alloc_instrumentation.md
@@ -17,7 +17,7 @@
 - These in turn are called by `SetStatsEnabled()`, `SetAllocationListener()`, et al, which
 require the mutator lock is not held.
 
-- With a started runtime, `SetEntrypointsInstrumented()` calls `ScopedSupendAll(`) before updating
+- With a started runtime, `SetEntrypointsInstrumented()` calls `ScopedSuspendAll(`) before updating
   the function table.
 
 Mutual exclusion in the dispatch table is thus ensured by the fact that it is only updated while

runtime/base/mutex.cc

diff --git a/runtime/base/mutex.cc b/runtime/base/mutex.cc
index 5709333..01d7e73 100644
--- a/runtime/base/mutex.cc
+++ b/runtime/base/mutex.cc
@@ -59,18 +59,19 @@
 };
 
 #if ART_USE_FUTEXES
+// Compute a relative timespec as *result_ts = lhs - rhs.
+// Return false (and produce an invalid *result_ts) if lhs < rhs.
 static bool ComputeRelativeTimeSpec(timespec* result_ts, const timespec& lhs, const timespec& rhs) {
   const int32_t one_sec = 1000 * 1000 * 1000;  // one second in nanoseconds.
+  static_assert(std::is_signed<decltype(result_ts->tv_sec)>::value);  // Signed on Linux.
   result_ts->tv_sec = lhs.tv_sec - rhs.tv_sec;
   result_ts->tv_nsec = lhs.tv_nsec - rhs.tv_nsec;
   if (result_ts->tv_nsec < 0) {
     result_ts->tv_sec--;
     result_ts->tv_nsec += one_sec;
-  } else if (result_ts->tv_nsec > one_sec) {
-    result_ts->tv_sec++;
-    result_ts->tv_nsec -= one_sec;
   }
-  return result_ts->tv_sec < 0;
+  DCHECK(result_ts->tv_nsec >= 0 && result_ts->tv_nsec < one_sec);
+  return result_ts->tv_sec >= 0;
 }
 #endif
 
@@ -852,7 +853,7 @@
       timespec now_abs_ts;
       InitTimeSpec(true, CLOCK_MONOTONIC, 0, 0, &now_abs_ts);
       timespec rel_ts;
-      if (ComputeRelativeTimeSpec(&rel_ts, end_abs_ts, now_abs_ts)) {
+      if (!ComputeRelativeTimeSpec(&rel_ts, end_abs_ts, now_abs_ts)) {
         return false;  // Timed out.
       }
       ScopedContentionRecorder scr(this, SafeGetTid(self), GetExclusiveOwnerTid());
@@ -869,6 +870,7 @@
             // EAGAIN and EINTR both indicate a spurious failure,
             // recompute the relative time out from now and try again.
             // We don't use TEMP_FAILURE_RETRY so we can recompute rel_ts;
+            num_contenders_.fetch_sub(1);  // Unlikely to matter.
             PLOG(FATAL) << "timed futex wait failed for " << name_;
           }
         }