From 32476eb6aadffbe2cbff7a10833d69dc38a87303 Mon Sep 17 00:00:00 2001 From: Roman Kennke Date: Thu, 22 Oct 2026 14:53:55 +0200 Subject: [PATCH] Require trace IDs to increase in creation order during calibration KlassTraceId::findOffset() accepted any offset at which the probes held distinct values within ID_SLACK of each other after >> 16. On aarch64 17u a pointer field at offset 24 passed that next to the real trace ID at 168, so calibration found two candidates, refused both, and fjmethodid=lazy stayed off for the JVM (CI: glibc-17-debug-aarch64). JFR assigns trace IDs from one global counter as each klass is created, and the probes are created one after another, so their IDs strictly increase, mostly by exactly one. plausibleIds() now requires both: values in creation order, and at least half the steps +1. Pointers neither follow creation order nor step by exactly one. The first attempt in a JVM regularly sees steps of 2 to 4 and is now rejected, so calibrate() makes up to six attempts instead of four. Co-Authored-By: Claude Opus 5.5 --- .../src/main/cpp/hotspot/klassTraceId.cpp | 39 +++++++++-------- .../src/main/cpp/hotspot/klassTraceId.h | 15 ++++--- ddprof-lib/src/test/cpp/klassTraceId_ut.cpp | 43 ++++++++++++++++++- 3 files changed, 71 insertions(+), 26 deletions(-) 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);