Static memory snapshot: add support for Unsafe, VarHandle, and AtomicFieldUpdater #429

dmitrii-artuhov · 2024-11-29T16:28:26Z

Partly resolves issue #389.

This PR is aimed to add support for tracking fields that are accessed via:

Unsafe API
VarHandle API
Java's AtomicFieldUpdater

Notably,System.arraycopy(..) is currently not supported.

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt

ndkoval · 2024-12-12T12:13:12Z

Please note that you don't need to have special support for atomicfu -- it compiles to either AFU or VarHandle

ndkoval · 2024-12-12T20:13:34Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/Utils.kt

@@ -25,6 +25,57 @@ import java.util.*
 import kotlin.coroutines.*
 import kotlin.coroutines.intrinsics.*

+fun shouldTransformClass(className: String): Boolean {


It seems the code was moved from somewhere, but I do not see it removed in the original location.

Also, I would suggest keeping the transformation-related logic in the corresponding package.

ndkoval · 2024-12-12T20:15:00Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/AtomicFieldUpdaterNames.kt

@@ -37,11 +37,22 @@ internal object AtomicFieldUpdaterNames {
            val offsetField = updater.javaClass.getDeclaredField("offset")
            val offset = UNSAFE.getLong(updater, UNSAFE.objectFieldOffset(offsetField))

-            return findFieldNameByOffsetViaUnsafe(targetType, offset)
+            return findFieldNameByOffsetViaUnsafe(targetType, offset).let { fieldName ->
+                if (fieldName != null) AtomicFieldUpdaterDescriptor(targetType, fieldName)


Please always add { .. } when the else branch is present.

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt

eupp

Please also don't forget to rebase on develop.

eupp · 2025-01-08T10:40:55Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt

+            isVarHandle(owner) &&
+            (
+                VarHandleNames.isInstanceVarHandle(owner) && staticMemorySnapshot.isTracked(params.firstOrNull()) ||
+                VarHandleNames.isArrayVarHandle(owner) && staticMemorySnapshot.isTracked(params.firstOrNull()) ||


Why do you check staticMemorySnapshot.isTracked in case of VarHandle API, but not in case of AFU or Unsafe APIs?

Also, wouldn't the staticMemorySnapshot.trackField methods check itself if the object is tracked or not?

eupp · 2025-01-08T10:47:01Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/AtomicFieldUpdaterNames.kt

+                else {
+                    null
+                }
+            }


Using let is overkill here.

val fieldName = findFieldNameByOffsetViaUnsafe(targetType, offset) ?: return null return AtomicFieldUpdaterDescriptor(targetType, fieldName)

eupp · 2025-01-08T10:51:56Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/SnapshotTracker.kt

-    private fun getDeclaringClass(obj: Any?, className: String, fieldName: String): Class<*> {
-        return Class.forName(className).let {
+    private fun getDeclaringClass(obj: Any?, clazz: Class<*>, fieldName: String): Class<*> {
+        return clazz.let {


using let is overkill here

eupp · 2025-01-08T10:52:51Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/SnapshotTracker.kt

+    }
+
+    fun trackField(obj: Any?, accessClassName: String, fieldName: String) {
+        if (obj != null && obj !in trackedObjects) return


Please replace this line and all related lines with isTracked(obj)

eupp · 2025-01-08T10:54:55Z

...org/jetbrains/kotlinx/lincheck_test/strategy/modelchecking/snapshot/ArraysAPISnapshotTest.kt

+        override fun verifyResults(scenario: ExecutionScenario?, results: ExecutionResult?): Boolean {
+            checkForExceptions(results)
+            check(intArray === refIntArray)
+            check(intArray.contentEquals(intArrayOf(2, 1, 4, 3, 6, 5, 8, 7, 10, 9)))


please extract intArrayOf(2, 1, 4, 3, 6, 5, 8, 7, 10, 9) into separate variable

eupp · 2025-01-08T10:57:41Z

...etbrains/kotlinx/lincheck_test/strategy/modelchecking/snapshot/CollectionsAPISnapshotTest.kt

+        override fun verifyResults(scenario: ExecutionScenario?, results: ExecutionResult?): Boolean {
+            checkForExceptions(results)
+            check(intList === refIntList)
+            check(intList.toIntArray().contentEquals(intArrayOf(2, 1, 4, 3, 6, 5, 8, 7, 10, 9)))


please extract intArrayOf(2, 1, 4, 3, 6, 5, 8, 7, 10, 9) into separate variable

.../kotlinx/lincheck_test/strategy/modelchecking/snapshot/VarHandleModificationsSnapshotTest.kt

…teger', etc)

…ity methods of Arrays and Collections classes

eupp

Few final nitpicks.

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/SnapshotTracker.kt

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt

eupp · 2025-01-16T20:50:16Z

@ndkoval I am good with the current state of this PR. What about you? Should we merge it?

ndkoval · 2025-01-17T13:18:06Z

...org/jetbrains/kotlinx/lincheck_test/strategy/modelchecking/snapshot/ArraysAPISnapshotTest.kt

+
+private val arrayValue = intArrayOf(2, 1, 4, 3, 6, 5, 8, 7, 10, 9)
+
+// TODO: parallel operations are not supported because of java.lang.ClassCastException:


Should this TODO be left?

Arrays API has parallel* versions of methods, I added this todo to add them later, when I fix the issue with System.arraycopy.

I updated the TODO message a little bit.

ndkoval · 2025-01-17T13:18:45Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/AtomicFieldUpdaterNames.kt

@@ -24,7 +24,7 @@ import java.util.concurrent.atomic.AtomicReferenceFieldUpdater
 internal object AtomicFieldUpdaterNames {

    @Suppress("DEPRECATION")
-    internal fun getAtomicFieldUpdaterName(updater: Any): String? {
+    internal fun getAtomicFieldUpdaterName(updater: Any): AtomicFieldUpdaterDescriptor? {


Should the function be renamed?

ndkoval · 2025-01-17T13:20:28Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/SnapshotTracker.kt

-            else it.findField(fieldName).declaringClass
-        }
+    private fun getDeclaringClass(obj: Any?, clazz: Class<*>, fieldName: String): Class<*> {
+        return if (obj != null) clazz


When using both clauses in if, please wrap the branches with brackets, it makes the flow more visible when reading the code

ndkoval · 2025-01-17T13:30:53Z

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt

+                staticMemorySnapshot.trackField(obj, afuDesc.targetType, afuDesc.fieldName)
+            }
+            // TODO: System.arraycopy
+            // TODO: reflexivity


dmitrii-artuhov changed the base branch from develop to trace-debugger-static-memory November 29, 2024 16:28

dmitrii-artuhov commented Nov 29, 2024

View reviewed changes

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt Outdated Show resolved Hide resolved

dmitrii-artuhov force-pushed the trace-debugger-system-calls branch from 881a74a to fb813ab Compare December 2, 2024 21:10

dmitrii-artuhov mentioned this pull request Dec 2, 2024

Improve support of static initialization blocks and statically-reachable memory #389

Open

dmitrii-artuhov changed the title ~~Static memory snapshot: add support for System.arraycopy, Unsafe, VarHandle, AFU, kotlinx.atomicfu, and reflexivity~~ Static memory snapshot: add support for System.arraycopy, Unsafe, VarHandle, AFU, kotlinx.atomicfu Dec 2, 2024

dmitrii-artuhov changed the title ~~Static memory snapshot: add support for System.arraycopy, Unsafe, VarHandle, AFU, kotlinx.atomicfu~~ Static memory snapshot: add support for Unsafe, VarHandle, AFU, kotlinx.atomicfu Dec 9, 2024

dmitrii-artuhov marked this pull request as ready for review December 12, 2024 11:13

dmitrii-artuhov requested a review from eupp December 12, 2024 11:13

Base automatically changed from trace-debugger-static-memory to develop December 12, 2024 11:17

dmitrii-artuhov force-pushed the trace-debugger-system-calls branch from d95fd4d to 2fd1a05 Compare December 12, 2024 13:32

ndkoval reviewed Dec 12, 2024

View reviewed changes

src/jvm/main/org/jetbrains/kotlinx/lincheck/strategy/managed/ManagedStrategy.kt Show resolved Hide resolved

eupp requested changes Jan 8, 2025

View reviewed changes

dmitrii-artuhov added 15 commits January 13, 2025 17:30

Add static memory snapshot collecting and restoring

e5d9aa0

Add tests and fixes to snapshot algorithm

99168f6

Make snapshot algorithm lazy

9ed10c0

Add energetic traverse for non-transformed classes (such as 'AtomicIn…

fdf1371

…teger', etc)

Refactor code

fee3305

Add energetic snapshot recording for non-transformed classes

6c3a5a8

Enable snapshot restoring for all tests, fix representation tests

e52524e

Add minor refactoring

5b0ed36

Add tests for java atomics and atomicfu classes for snapshot restoring

ae97606

Add tracking for fields of objects the same type as constructor owner

ba523b0

Fix todo with subclass check in SnapshotTrackerTransformer

4a73fef

Add some PR fixes

985ce52

Add fixes to the PR

944df24

Track calls to System.arraycopy, Unsafe, VarHandles, and Java AFU

d9b1527

Remove useless comments

08b1e28

dmitrii-artuhov added 9 commits January 13, 2025 17:32

Add tests for unsafe, varhandle, java afu, kotlinx.atomicfu, and util…

e6463d3

…ity methods of Arrays and Collections classes

Fix compilation

2332e3d

Suppress deprecation in test

fb8d697

Remove 'System.arraycopy' support

eaab6df

Refactor VarHandle methods processing

2cfa9cd

Unite ignored section in 'ManagedStrategy::beforeMethodCall(...)' method

0816d23

Add minor performance improvements

2f52158

Minor code style fixes

ece1510

Fix PR comments

dce3e40

dmitrii-artuhov force-pushed the trace-debugger-system-calls branch from 2ef2af9 to dce3e40 Compare January 13, 2025 16:48

Fix PR

7b76466

dmitrii-artuhov requested a review from eupp January 14, 2025 12:51

eupp requested changes Jan 15, 2025

View reviewed changes

Add PR fixes

4670f59

dmitrii-artuhov requested a review from eupp January 16, 2025 17:09

eupp approved these changes Jan 16, 2025

View reviewed changes

eupp requested a review from ndkoval January 16, 2025 20:49

ndkoval changed the title ~~Static memory snapshot: add support for Unsafe, VarHandle, AFU, kotlinx.atomicfu~~ Static memory snapshot: add support for Unsafe, VarHandle, and AtomicFieldUpdater Jan 17, 2025

ndkoval reviewed Jan 17, 2025

View reviewed changes

Fix PR comments

6696a7b

ndkoval approved these changes Jan 17, 2025

View reviewed changes

eupp merged commit 0cbfcaf into develop Jan 22, 2025
16 checks passed

eupp deleted the trace-debugger-system-calls branch January 22, 2025 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static memory snapshot: add support for Unsafe, VarHandle, and AtomicFieldUpdater #429

Static memory snapshot: add support for Unsafe, VarHandle, and AtomicFieldUpdater #429

dmitrii-artuhov commented Nov 29, 2024 •

edited by ndkoval

Loading

ndkoval commented Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

ndkoval Dec 12, 2024

eupp left a comment

eupp Jan 8, 2025

eupp Jan 8, 2025

eupp Jan 8, 2025

eupp Jan 8, 2025

eupp Jan 8, 2025

eupp Jan 8, 2025

eupp left a comment

eupp commented Jan 16, 2025

ndkoval Jan 17, 2025

dmitrii-artuhov Jan 17, 2025

ndkoval Jan 17, 2025

ndkoval Jan 17, 2025

ndkoval Jan 17, 2025


		private val arrayValue = intArrayOf(2, 1, 4, 3, 6, 5, 8, 7, 10, 9)

		// TODO: parallel operations are not supported because of java.lang.ClassCastException:

Static memory snapshot: add support for Unsafe, VarHandle, and AtomicFieldUpdater #429

Static memory snapshot: add support for Unsafe, VarHandle, and AtomicFieldUpdater #429

Conversation

dmitrii-artuhov commented Nov 29, 2024 • edited by ndkoval Loading

ndkoval commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eupp left a comment

Choose a reason for hiding this comment

eupp commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrii-artuhov commented Nov 29, 2024 •

edited by ndkoval

Loading