diff --git a/ddprof-lib/src/main/cpp/hotspot/klassTraceId.cpp b/ddprof-lib/src/main/cpp/hotspot/klassTraceId.cpp index 87bb2e00dd..ed5043b7df 100644 --- a/ddprof-lib/src/main/cpp/hotspot/klassTraceId.cpp +++ b/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) { diff --git a/ddprof-lib/src/main/cpp/hotspot/klassTraceId.h b/ddprof-lib/src/main/cpp/hotspot/klassTraceId.h index e95e019e4f..b808019f91 100644 --- a/ddprof-lib/src/main/cpp/hotspot/klassTraceId.h +++ b/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,8 +40,11 @@ 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; @@ -49,8 +52,8 @@ class KlassTraceId { /** * 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); diff --git a/ddprof-lib/src/test/cpp/klassTraceId_ut.cpp b/ddprof-lib/src/test/cpp/klassTraceId_ut.cpp index 78ffa9eca0..c8a4861ba8 100644 --- a/ddprof-lib/src/test/cpp/klassTraceId_ut.cpp +++ b/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);