Skip to content

Commit 8d7dcd3

Browse files
authored
Merge branch 'main' into jb/native_allocs
2 parents 17da47c + b5abc18 commit 8d7dcd3

19 files changed

Lines changed: 315 additions & 116 deletions

File tree

.github/scripts/prepare_reports.sh

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ cp ddprof-test/javacore*.txt test-reports/ || true
1212
cp ddprof-test/build/hs_err* test-reports/ || true
1313
cp -r ddprof-lib/build/tmp test-reports/native_build || true
1414
cp -r ddprof-test/build/reports/tests test-reports/tests || true
15+
cp build/logs/gdb-watchdog.log test-reports/ || true
1516
cp -r /tmp/recordings test-reports/recordings || true
1617
find ddprof-lib/build -name 'libjavaProfiler.*' -exec cp {} test-reports/ \; || true
1718

.github/workflows/cache_java.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ jobs:
175175
sudo apt-get install -y curl zip unzip
176176
- name: Download SDKMAN! from Artifact
177177
if: steps.check-cache.outputs.cache-hit != 'true'
178-
uses: actions/download-artifact@v7
178+
uses: actions/download-artifact@v8
179179
with:
180180
name: sdkman-installation-amd64
181181
path: sdkman
@@ -286,7 +286,7 @@ jobs:
286286
sudo apt-get install -y curl zip unzip
287287
- name: Download SDKMAN! from Artifact
288288
if: steps.check-cache.outputs.cache-hit != 'true'
289-
uses: actions/download-artifact@v7
289+
uses: actions/download-artifact@v8
290290
with:
291291
name: sdkman-installation-aarch64
292292
path: sdkman

.github/workflows/nightly.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ jobs:
2121
if: failure()
2222
steps:
2323
- name: Download all failure artifacts
24-
uses: actions/download-artifact@v7
24+
uses: actions/download-artifact@v8
2525
with:
2626
pattern: failures-*
2727
path: ./artifacts

.github/workflows/release-validated.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ jobs:
148148
runs-on: ubuntu-latest
149149
steps:
150150
- name: Check test results
151-
if: ${{ inputs.skip_tests != true && needs.pre-release-tests.result != 'success' }}
151+
if: ${{ inputs.dry_run != true && inputs.skip_tests != true && needs.pre-release-tests.result != 'success' }}
152152
run: |
153153
echo "::error::Pre-release tests failed. Cannot proceed with release."
154154
exit 1

.github/workflows/test_workflow.yml

Lines changed: 58 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -104,12 +104,25 @@ jobs:
104104
export LIBC=glibc
105105
export SANITIZER=${{ matrix.config }}
106106

107-
mkdir -p build/logs
108-
./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \
109-
| tee -a build/test-raw.log \
110-
| python3 -u .github/scripts/filter_gradle_log.py
111-
EXIT_CODE=${PIPESTATUS[0]}
112-
107+
MAX_ATTEMPTS=1
108+
if [[ "${{ matrix.config }}" == "asan" && "${{ matrix.java_version }}" =~ (j9|ibm) ]]; then
109+
MAX_ATTEMPTS=2
110+
fi
111+
112+
for attempt in $(seq 1 $MAX_ATTEMPTS); do
113+
mkdir -p build/logs
114+
./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \
115+
| tee -a build/test-raw.log \
116+
| python3 -u .github/scripts/filter_gradle_log.py
117+
EXIT_CODE=${PIPESTATUS[0]}
118+
119+
if [ $EXIT_CODE -eq 0 ]; then break; fi
120+
if [ $attempt -lt $MAX_ATTEMPTS ]; then
121+
echo "::warning::Attempt $attempt failed (exit $EXIT_CODE), retrying..."
122+
./gradlew --stop 2>/dev/null || true
123+
fi
124+
done
125+
113126
if [ $EXIT_CODE -ne 0 ]; then
114127
echo "glibc-${{ matrix.java_version }}-${{ matrix.config }}-amd64" >> failures_glibc-${{ matrix.java_version }}-${{ matrix.config }}-amd64.txt
115128
exit 1
@@ -327,7 +340,7 @@ jobs:
327340
sudo apt update -y
328341
sudo apt remove -y g++
329342
sudo apt autoremove -y
330-
sudo apt install -y curl zip unzip clang make build-essential binutils
343+
sudo apt install -y curl zip unzip clang make build-essential binutils gdb
331344
# Install debug symbols for system libraries
332345
sudo apt install -y libc6-dbg
333346
if [[ ${{ matrix.java_version }} =~ "-zing" ]]; then
@@ -350,11 +363,44 @@ jobs:
350363
export LIBC=glibc
351364
export SANITIZER=${{ matrix.config }}
352365
353-
mkdir -p build/logs
354-
./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \
355-
| tee -a build/test-raw.log \
356-
| python3 -u .github/scripts/filter_gradle_log.py
357-
EXIT_CODE=${PIPESTATUS[0]}
366+
# For ASAN: launch a gdb watchdog that dumps all native threads before the
367+
# 30-minute Gradle timeout kills the JVM, so we can diagnose hangs in native code.
368+
if [[ "${{ matrix.config }}" == "asan" ]]; then
369+
mkdir -p build/logs
370+
(
371+
sleep 1500 # 25 minutes — fires before the 30-min Gradle timeout
372+
echo "::warning::GDB watchdog triggered after 25 minutes"
373+
for pid in $(pgrep -f 'java.*ddprof-test'); do
374+
echo "=== Thread dump for PID $pid ===" >> build/logs/gdb-watchdog.log
375+
gdb -batch -ex 'thread apply all bt full' -p "$pid" >> build/logs/gdb-watchdog.log 2>&1 || true
376+
done
377+
) &
378+
GDB_WATCHDOG_PID=$!
379+
fi
380+
381+
MAX_ATTEMPTS=1
382+
if [[ "${{ matrix.config }}" == "asan" && "${{ matrix.java_version }}" =~ (j9|ibm) ]]; then
383+
MAX_ATTEMPTS=2
384+
fi
385+
386+
for attempt in $(seq 1 $MAX_ATTEMPTS); do
387+
mkdir -p build/logs
388+
./gradlew -PCI -PkeepJFRs :ddprof-test:test${{ matrix.config }} --no-daemon --parallel --build-cache --no-watch-fs 2>&1 \
389+
| tee -a build/test-raw.log \
390+
| python3 -u .github/scripts/filter_gradle_log.py
391+
EXIT_CODE=${PIPESTATUS[0]}
392+
393+
if [ $EXIT_CODE -eq 0 ]; then break; fi
394+
if [ $attempt -lt $MAX_ATTEMPTS ]; then
395+
echo "::warning::Attempt $attempt failed (exit $EXIT_CODE), retrying..."
396+
./gradlew --stop 2>/dev/null || true
397+
fi
398+
done
399+
400+
# Kill the watchdog if tests finished before it fired
401+
if [[ -n "${GDB_WATCHDOG_PID:-}" ]]; then
402+
kill "$GDB_WATCHDOG_PID" 2>/dev/null || true
403+
fi
358404
359405
if [ $EXIT_CODE -ne 0 ]; then
360406
echo "glibc-${{ matrix.java_version }}-${{ matrix.config }}-aarch64" >> failures_glibc-${{ matrix.java_version }}-${{ matrix.config }}-aarch64.txt

AGENTS.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,17 @@ The profiler uses a sophisticated double-buffered storage system for call traces
357357
- **Atomic Operations**: Instance ID management and counter updates use atomics
358358
- **Memory Allocation**: Minimize malloc() in hot paths, use pre-allocated containers
359359

360+
### Atomic Memory Ordering (Critical for arm64)
361+
arm64 has a weakly-ordered memory model (unlike x86 TSO). Incorrect ordering causes real lockups on arm64 that never reproduce on x86.
362+
- **Cross-thread reads**: Always use `__ATOMIC_ACQUIRE` for loads that must see stores from another thread. Never use `__ATOMIC_RELAXED` for cross-thread visibility unless you can prove no ordering dependency exists.
363+
- **Cross-thread writes**: Use `__ATOMIC_RELEASE` for stores that must be visible to other threads. Pair with `__ATOMIC_ACQUIRE` loads.
364+
- **Count + pointer patterns**: When a data structure publishes a count and a separate pointer (two-phase add), both the count load and pointer load need acquire semantics so the reader sees the pointer store that preceded the count increment.
365+
- **Default stance**: When in doubt, use acquire/release. The performance cost is negligible; the correctness cost of relaxed ordering bugs is enormous (silent arm64-only lockups).
366+
367+
### Concurrent Data Structure Iteration
368+
- **NULL gaps**: When iterating a concurrent array (e.g., `CodeCacheArray`), always NULL-check each slot — a slot may be count-allocated but pointer-not-yet-stored.
369+
- **Cursor advancement**: Never permanently advance an iteration cursor past NULL gaps. Stop at the first NULL or track the last contiguous non-NULL entry, so missing entries are retried on the next pass.
370+
360371
### 64-bit Trace ID System
361372
- **Collision Avoidance**: Instance-based IDs prevent collisions across storage swaps
362373
- **JFR Compatibility**: 64-bit IDs work with JFR constant pool indices
@@ -705,3 +716,9 @@ The CI caches JDKs via `.github/workflows/cache_java.yml`. When adding a new JDK
705716
- Always provide tests for bug fixes - test fails before the fix, passes after the fix
706717
- All code needs to strive to be lean in terms of resources consumption and easy to follow -
707718
do not shy away from factoring out self containing code to shorter functions with explicit name
719+
720+
### C/C++ Code Style
721+
- **Indentation**: Match the exact indentation style of the surrounding code in each file. Do not introduce inconsistent indentation — reviewers will flag it.
722+
- **Minimal complexity**: Do not split inline logic into separate helper functions unless the helpers are reused or the original is genuinely hard to follow. Unnecessary splits add indirection without value.
723+
- **Comment precision**: Comments explaining "why" must reference concrete mechanisms (e.g., "ASAN's allocator lock is reentrant" not "internal bookkeeping"). Vague comments get challenged in review. Every claim in a comment must be verifiable from the code or documented behavior of the referenced system (ASAN, glibc NPTL, HotSpot, etc.).
724+
- **No speculative comments**: Do not claim a system (HotSpot, glibc, ASAN) uses a specific mechanism unless you are certain. If unsure, describe the observable symptom instead of guessing the cause.

build-logic/conventions/src/main/kotlin/com/datadoghq/native/util/PlatformUtils.kt

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,4 +304,22 @@ object PlatformUtils {
304304
"or specify one with -Pnative.forceCompiler=/path/to/compiler"
305305
)
306306
}
307+
308+
/**
309+
* Returns true if the test JVM (from JAVA_TEST_HOME or JAVA_HOME) is an OpenJ9/J9 JVM.
310+
* Probes `java -version` stderr output for "J9" or "OpenJ9".
311+
*/
312+
fun isTestJvmJ9(): Boolean {
313+
val javaHome = testJavaHome()
314+
return try {
315+
val process = ProcessBuilder("$javaHome/bin/java", "-version")
316+
.redirectErrorStream(true)
317+
.start()
318+
val output = process.inputStream.bufferedReader().readText()
319+
process.waitFor(10, TimeUnit.SECONDS)
320+
output.contains("J9") || output.contains("OpenJ9")
321+
} catch (_: Exception) {
322+
false
323+
}
324+
}
307325
}

build-logic/conventions/src/main/kotlin/com/datadoghq/profiler/ProfilerTestPlugin.kt

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import org.gradle.api.provider.Property
1515
import org.gradle.api.tasks.Exec
1616
import org.gradle.api.tasks.SourceSetContainer
1717
import org.gradle.api.tasks.testing.Test
18+
import java.time.Duration
1819
import javax.inject.Inject
1920

2021
/**
@@ -254,6 +255,17 @@ class ProfilerTestPlugin : Plugin<Project> {
254255
"asan" -> testTask.onlyIf { PlatformUtils.locateLibasan() != null }
255256
"tsan" -> testTask.onlyIf { PlatformUtils.locateLibtsan() != null }
256257
}
258+
259+
// J9+ASAN: isolate each test class in its own JVM to limit crash blast radius
260+
if (testConfig.configName == "asan") {
261+
testTask.doFirst {
262+
if (PlatformUtils.isTestJvmJ9()) {
263+
testTask.setForkEvery(1)
264+
}
265+
}
266+
// Kill hung ASAN test tasks after 30 minutes
267+
testTask.timeout.set(Duration.ofMinutes(30))
268+
}
257269
}
258270
}
259271

build.gradle.kts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ plugins {
1616
id("com.datadoghq.native-root")
1717
}
1818

19-
version = "1.39.0"
19+
version = "1.40.0"
2020

2121
apply(plugin = "com.dipien.semantic-version")
2222
version = findProperty("ddprof_version") as? String ?: version

ddprof-lib/src/main/cpp/codeCache.h

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -257,34 +257,47 @@ class CodeCache {
257257
class CodeCacheArray {
258258
private:
259259
CodeCache *_libs[MAX_NATIVE_LIBS];
260-
volatile int _count;
260+
volatile int _reserved; // next slot to reserve (CAS by writers)
261+
volatile int _count; // published count (all indices < _count have non-NULL pointers)
261262
volatile size_t _used_memory;
262263

263264
public:
264-
CodeCacheArray() : _count(0), _used_memory(0) {
265+
CodeCacheArray() : _reserved(0), _count(0), _used_memory(0) {
265266
memset(_libs, 0, MAX_NATIVE_LIBS * sizeof(CodeCache *));
266267
}
267268

268269
CodeCache *operator[](int index) const { return __atomic_load_n(&_libs[index], __ATOMIC_ACQUIRE); }
269270

270-
int count() const { return __atomic_load_n(&_count, __ATOMIC_RELAXED); }
271+
// All indices < count() are guaranteed to have a non-NULL pointer.
272+
int count() const { return __atomic_load_n(&_count, __ATOMIC_ACQUIRE); }
271273

274+
// Pointer-first add: reserve a slot via CAS on _reserved, store the
275+
// pointer with RELEASE, then advance _count. Readers see count() grow
276+
// only after the pointer is visible, so indices < count() never yield NULL.
272277
void add(CodeCache *lib) {
273-
int index = __atomic_fetch_add(&_count, 1, __ATOMIC_RELAXED);
274-
if (index < MAX_NATIVE_LIBS) {
275-
__atomic_fetch_add(&_used_memory, lib->memoryUsage(), __ATOMIC_RELAXED);
276-
__atomic_store_n(&_libs[index], lib, __ATOMIC_RELEASE);
278+
int slot = __atomic_load_n(&_reserved, __ATOMIC_RELAXED);
279+
do {
280+
if (slot >= MAX_NATIVE_LIBS) return;
281+
} while (!__atomic_compare_exchange_n(&_reserved, &slot, slot + 1,
282+
true, __ATOMIC_RELAXED, __ATOMIC_RELAXED));
283+
assert(__atomic_load_n(&_libs[slot], __ATOMIC_RELAXED) == nullptr);
284+
__atomic_fetch_add(&_used_memory, lib->memoryUsage(), __ATOMIC_RELAXED);
285+
// Store pointer before publishing count. The RELEASE here pairs with
286+
// the ACQUIRE load in operator[]/at() and count().
287+
__atomic_store_n(&_libs[slot], lib, __ATOMIC_RELEASE);
288+
// Advance _count to publish the new slot. Spin until our slot is next
289+
// in line, preserving contiguous ordering when multiple adds race.
290+
while (__atomic_load_n(&_count, __ATOMIC_RELAXED) != slot) {
291+
// wait for preceding slots to publish
277292
}
293+
__atomic_store_n(&_count, slot + 1, __ATOMIC_RELEASE);
278294
}
279295

280296
CodeCache* at(int index) const {
281297
if (index >= MAX_NATIVE_LIBS) {
282-
return nullptr;
298+
return nullptr;
283299
}
284-
CodeCache* lib = nullptr;
285-
while ((lib = __atomic_load_n(&_libs[index], __ATOMIC_ACQUIRE)) == nullptr);
286-
287-
return lib;
300+
return __atomic_load_n(&_libs[index], __ATOMIC_ACQUIRE);
288301
}
289302

290303
size_t memoryUsage() const {

0 commit comments

Comments
 (0)