Require trace IDs to increase in creation order during calibration

ddprof-lib/src/main/cpp/hotspot/klassTraceId.cpp

@@ -22,29 +22,32 @@ static inline uint64_t idAt(const void* klass, size_t offset) {
   return raw >> TRACE_ID_SHIFT;
 }
 
-// The probes were created back to back by one thread, so their IDs are
-// distinct and lie within a narrow window: PROBE_COUNT consecutive values
-// plus whatever other threads took meanwhile. Pointers to neighbouring
-// metadata fail this (they are hundreds of bytes apart, so `>> 16` mostly
-// collapses them to one value); so do small integers (equal across probes)
-// and compressed-klass bits in a mark word (far more than ID_SLACK apart).
+// JFR assigns trace IDs from one global counter as each klass is created,
+// and the probes were created back to back by one thread, in the order given.
+// So their IDs strictly increase, mostly by exactly one, and span at most
+// PROBE_COUNT - 1 plus whatever other threads took meanwhile (ID_SLACK).
+// Small integers fail this (equal across probes), and so do the
+// compressed-klass bits of a mark word (far more than ID_SLACK apart).
+// Pointers shifted right by 16 can be distinct and close together (seen for
+// a Klass field at offset 24 on aarch64 17u), but neither follow creation
+// order nor step by exactly one: requiring both keeps them out.
 static bool plausibleIds(const void* const* klasses, int count, size_t offset) {
-  uint64_t min = UINT64_MAX;
-  uint64_t max = 0;
-  for (int i = 0; i < count; i++) {
+  uint64_t first = idAt(klasses[0], offset);
+  uint64_t prev = first;
+  int unit_steps = 0;
+  if (first == 0) {
+    return false;
+  }
+  for (int i = 1; i < count; i++) {
     uint64_t id = idAt(klasses[i], offset);
-    if (id == 0) {
+    if (id <= prev) {
       return false;
     }
-    for (int j = 0; j < i; j++) {
-      if (idAt(klasses[j], offset) == id) {
-        return false;
-      }
-    }
-    min = id < min ? id : min;
-    max = id > max ? id : max;
+    unit_steps += id - prev == 1 ? 1 : 0;
+    prev = id;
   }
-  return max - min <= (uint64_t)(count - 1) + KlassTraceId::ID_SLACK;
+  // At least half of the steps between consecutive probes must be +1.
+  return prev - first <= (uint64_t)(count - 1) + KlassTraceId::ID_SLACK && 2 * unit_steps >= count - 1;
 }
 
 int KlassTraceId::findOffset(const void* const* klasses, int count, size_t klass_size) {

ddprof-lib/src/main/cpp/hotspot/klassTraceId.h

@@ -28,8 +28,8 @@
  * calibrate() makes the JVM create a few array klasses back to back, of a
  * class it defines in a new class loader so that they cannot have existed
  * before, and accepts the one 8-byte-aligned offset within sizeof(Klass) at
- * which all of them hold distinct IDs no further apart than consecutive
- * creation allows.
+ * which their IDs increase in creation order, mostly by one, no further
+ * apart than consecutive creation allows.
  * Builds without JFR, or a layout where no single offset qualifies, leave the
  * offset unknown, and fjmethodid=lazy does not run.
  */
@@ -40,17 +40,20 @@ class KlassTraceId {
   // Array dimensions created by one calibration attempt: one probe klass per
   // dimension.
   static const int PROBE_COUNT = 6;
-  // Attempts each calibrate() call makes, each with a new class loader.
-  static const int MAX_ATTEMPTS = 4;
+  // Attempts each calibrate() call makes, each with a new class loader. The
+  // first attempt in a JVM regularly sees ID steps of 2 to 4 (observed on
+  // 17u, 21u and 25), which plausibleIds() rejects; later attempts step by
+  // one unless other threads create classes meanwhile.
+  static const int MAX_ATTEMPTS = 6;
   // How many trace IDs other threads may consume while the probes are
   // created, i.e. how far apart beyond PROBE_COUNT - 1 the probes' IDs may be.
   static const uint64_t ID_SLACK = 64;
 
   /**
    * Returns the single 8-byte-aligned offset in [8, klass_size) at which the
    * `count` klasses all hold plausible, freshly assigned trace IDs, or -1 if
-   * there is none or more than one. The klasses must be readable for
-   * klass_size bytes.
+   * there is none or more than one. The klasses must be in the order they
+   * were created and readable for klass_size bytes.
    */
   static int findOffset(const void* const* klasses, int count, size_t klass_size);
 

ddprof-lib/src/test/cpp/klassTraceId_ut.cpp

@@ -64,15 +64,54 @@ TEST(KlassTraceIdTest, IgnoresJfrTagBits) {
     EXPECT_EQ(9 * 8, k.find());
 }
 
-TEST(KlassTraceIdTest, ToleratesIdsTakenByOtherThreadsAndAnyOrder) {
+// Other threads may create classes between two probes, taking IDs from the
+// same counter: some steps are larger than one.
+TEST(KlassTraceIdTest, ToleratesIdsTakenByOtherThreads) {
     FakeKlasses k;
-    const uint64_t ids[N] = {5000, 5007, 5003, 5040, 5001, 5002};
+    const uint64_t ids[N] = {5000, 5001, 5009, 5010, 5040, 5041};
     for (int i = 0; i < N; i++) {
         k.words[i][9] = ids[i] << 16;
     }
     EXPECT_EQ(9 * 8, k.find());
 }
 
+// The probes are passed in creation order, so their IDs must increase.
+TEST(KlassTraceIdTest, RejectsIdsOutOfCreationOrder) {
+    FakeKlasses k;
+    const uint64_t ids[N] = {5000, 5001, 5003, 5002, 5004, 5005};
+    for (int i = 0; i < N; i++) {
+        k.words[i][9] = ids[i] << 16;
+    }
+    EXPECT_EQ(-1, k.find());
+}
+
+// Values that increase within the window but hardly ever by exactly one are
+// not a counter's.
+TEST(KlassTraceIdTest, RejectsIdsThatRarelyStepByOne) {
+    FakeKlasses k;
+    k.setTraceIds(9, 5000, 10);
+    EXPECT_EQ(-1, k.find());
+    const uint64_t ids[N] = {5000, 5001, 5009, 5017, 5025, 5026};  // two of five steps are +1
+    for (int i = 0; i < N; i++) {
+        k.words[i][9] = ids[i] << 16;
+    }
+    EXPECT_EQ(-1, k.find());
+}
+
+// What aarch64 17u held in fresh probes, from a CI log: at offset 24 pointers
+// whose upper bits were distinct and within the window, next to the real
+// trace IDs at offset 168. Both used to qualify, so calibration refused.
+TEST(KlassTraceIdTest, RejectsPointersThatLookLikeCloseIds) {
+    FakeKlasses k;
+    const uint64_t at24[N] = {0x0000f5e11aef6610ULL, 0x0000f5e11afb1720ULL, 0x0000f5e11ad7ba20ULL,
+                              0x0000f5e11b11a5a0ULL, 0x0000f5e11b13b540ULL, 0x0000f5e11ae369f0ULL};
+    for (int i = 0; i < N; i++) {
+        k.words[i][3] = at24[i];
+    }
+    k.setTraceIds(21, 0x4173);
+    EXPECT_EQ(168, k.find());
+}
+
 TEST(KlassTraceIdTest, RejectsIdsTooFarApart) {
     FakeKlasses k;
     k.setTraceIds(9, 5000, 1000);