Instrument constructor bodies by zhelenskiy · Pull Request #661 · JetBrains/lincheck

Evgeniy Zhelenskiy (zhelenskiy) · 2025-05-20T14:05:42Z

No description provided.

Nikita Koval (ndkoval) · 2025-05-20T16:23:15Z

 |                                                                                                                                             |   suspendAndCauseFailure() [suspendable: Continuation#1]: 0 at SingleSuspensionPointTraceRepresentationTest.operation(MultipleSuspensionsTests.kt:49) |
 |                                                                                                                                             |     counter ➜ AtomicInteger#1 at SingleSuspensionPointTraceRepresentationTest.suspendAndCauseFailure(MultipleSuspensionsTests.kt:54)                  |
-|                                                                                                                                             |     counter.get(): 1 at SingleSuspensionPointTraceRepresentationTest.suspendAndCauseFailure(MultipleSuspensionsTests.kt:54)                           |
+|                                                                                                                                             |     this$0.counter.get(): 1 at SingleSuspensionPointTraceRepresentationTest.suspendAndCauseFailure(MultipleSuspensionsTests.kt:54)                    |


Can we avoid this$0 here?

I see no way. I consider the cosmetic changes not urgent as this PR is just an extracted step for native calls.

It makes the trace slightly less intuitive. Could you please analyze how to fix it, please? It does not extremely complex.

Evgeniy Zhelenskiy (@zhelenskiy) I've noticed you made a fix for this issue. Could you please check why it does not fix other this$0 occurrences in the trace?

They already existed. Generally, it is an actual legitimate name for outer this parameter in the constructor of an inner class.

This is not something you type in the code, so I would prefer omitting it in the trace.

This is not something you type in the code

I saw such code many times in my life.

You don't have this$0 in the code; why should you have it in the trace?

How would you like to refer to the outer this?

Is outer-this better? Or this@Type?

Nikita Koval (ndkoval) · 2025-05-20T16:24:57Z

 | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
 | $this_runCatching.block(): threw IllegalStateException at BaseRunConcurrentRepresentationTest.testRunWithModelChecker$lambda$1$lambda$0(RunConcurrentRepresentationTests.kt:47) |                                                                                                                                                  |                                                                                                                                                 |
 |   block(): threw IllegalStateException at IncorrectConcurrentLinkedDequeRunConcurrentRepresentationTest.block(RunConcurrentRepresentationTests.kt:349)                          |                                                                                                                                                  |                                                                                                                                                 |
+|     Node#1.item.putObject(null) at ConcurrentLinkedDeque$Node.<init>(ConcurrentLinkedDeque.java:303)                                                                            |                                                                                                                                                  |                                                                                                                                                 |


Can we show constructor calls as method calls? (it looks as we miss a constructor call here)

Unfortunately, there is no simple way to do that.

What are the possible ways?

I'm not sure but it should be very complicated as the constructor call is separated into two parts.

Evgeniy Zhelenskiy (@zhelenskiy), we already have such a logic for reads, wrapping them with "beforeRead" and "afterRead" methods. Would the same approach work here?

please elaborate

I have previously tried doing this for some time and failed, so I think that:

It is not that simple.

It is another issue, and I don't see any reason to mix the PRs.

It is much less urgent.

I don't think adding constructor events to the trace with a dedicated constructor call is the right way to proceed. Besides, I do not see what exactly is not that simple in this discussion. Could you please provide specific obstacles, so we can discuss them and make the right choice?

Also adding Evgeniy Moiseenko (@eupp) to this thread.

As far as I remember, the problems were with stack frames, uninitialized variables, etc.

My argument against doing it now is the following:

the simple solution didn't work;

there is a separate issue for that;

the code is already working without it;

it can be done separately;

blocking this PR blocks native calls;

it is minor by its impact;

it is just a different feature, there is no need to mix up everything in one PR.

Evgeniy Moiseenko (eupp) · 2025-05-20T16:50:11Z

Should resolve #449

Dmitrii Art (dmitrii-artuhov) · 2025-05-27T13:57:38Z

Right now constructors are not fully instrumented, the SharedVariableAccessTransformer has a check inside to omit constructors.
https://github.com/JetBrains/lincheck/blob/allow-instrumenting-constructors/src/jvm/main/org/jetbrains/kotlinx/lincheck/transformation/transformers/SharedMemoryAccessTransformer.kt#L41

The problem with verification error could be reproduced in the following way:

/*
DataStructure::<init>:
   this.a = 10
   this.b = "hello world"
*/

scenario:
val ds: DataStructure? = null
T1                                                T2
ds = DataStructure()                            | ds?.method()
                                                | ds?.a = 10
                                                | ds?.b = "aaaa"

When JVM loads transformed class for DataStructure, it checks if there is a situation of using reference to ds (potentially non-fully constructed object) in multiple threads and will throw VerifyError.
I commented the check inside a SharedVariableAccessTransformer for constructors and multiple tests in our code base fail of because of that.
In a single threaded example (like in this PR) allowing SharedVariableAccessTransformer to insert its hooks is okey and throws no VerifyError. But when running other tests, where the object could be shared between threads during constructor invocation, the JVM some-how detects that and disallows to insert our hooks in constructors of such classes.

If this is not necessary to allow SharedVariableAccessTransformer to insert its hooks in the code, then we don't need to uncomment that check. And the main PR with native calls should be all right, I guess

Evgeniy Moiseenko (eupp) · 2025-05-27T14:39:34Z

Dmitrii Art (@dmitrii-artuhov) thanks for the investigation.

This situation in the scenario you provide looks like unsafe publication. But I wonder how JVM bytecode verifier is capable of detecting it (I thought it cannot). I suspect it has to do some kind of escape analysis to detect this.

Could you please share the whole code snippet you have (using GPMC syntax preferably).

Dmitrii Art (dmitrii-artuhov) · 2025-05-27T15:34:32Z

Cannot exactly reproduce decribed case, but I have come up with other tests:

class TestClass {
  @Test
  fun test() {
    runConcurrentTest(1) {
      val t = thread {} // inside has a `val thread = object : Thread { /* references to outer variable `block` */ }` 
    }
  }
}

class TestClass {
  var field = 0
  inner class Node {
    init { field++ }
  }

  @Test
  fun test() {
    runConcurrentTest(1) {
      val n = Node()
    }
  }
}

both produce VerifyError

Probably I was wrong about unsafe publication example. Here is a different reason for the verification error. The unsafe publication examples I tried to get the same error, but could not. Probably thas is not the case.
I was looking at this stack overflow discussion https://stackoverflow.com/questions/9851813/java-leaking-this-in-constructor when creating this issue #424, and thought we are having leaking this to different threads. But it seems to be something else that is causing VerifyError

Evgeniy Zhelenskiy (zhelenskiy) · 2025-05-27T18:14:23Z

Dmitrii Art (@dmitrii-artuhov) Does it fail both before and after my change?

Dmitrii Art (dmitrii-artuhov) · 2025-05-27T20:09:27Z

Dmitrii Art (@dmitrii-artuhov) Does it fail both before and after my change?

They fail after commenting out this part of the code
https://github.com/JetBrains/lincheck/blob/allow-instrumenting-constructors/src/jvm/main/org/jetbrains/kotlinx/lincheck/transformation/transformers/SharedMemoryAccessTransformer.kt#L41 (e.g. when inserting hooks of SharedVariableAccessTransformer in constructors)

Evgeniy Zhelenskiy (zhelenskiy) · 2025-05-27T21:00:45Z

Then can we just not comment them out? It is unnecessary for the native calls support.

Dmitrii Art (dmitrii-artuhov) · 2025-05-28T01:11:45Z

If tracking field accesses in constructors is not required for native calls, then we are fine, I believe

Evgeniy Zhelenskiy (zhelenskiy) requested review from Dmitrii Art (dmitrii-artuhov) and Evgeniy Moiseenko (eupp) May 20, 2025 14:05

Evgeniy Zhelenskiy (zhelenskiy) self-assigned this May 20, 2025

Evgeniy Zhelenskiy (zhelenskiy) force-pushed the allow-instrumenting-constructors branch from 2ea54e2 to 99efc8c Compare May 20, 2025 14:33

Evgeniy Moiseenko (eupp) changed the title ~~Instruct constructor bodies~~ Instrument constructor bodies May 20, 2025

Nikita Koval (ndkoval) reviewed May 20, 2025

View reviewed changes

Evgeniy Zhelenskiy (zhelenskiy) force-pushed the allow-instrumenting-constructors branch 2 times, most recently from 0f916a2 to 35f191a Compare May 20, 2025 23:42

Nikita Koval (ndkoval) removed the request for review from Dmitrii Art (dmitrii-artuhov) May 22, 2025 11:10

Evgeniy Moiseenko (eupp) mentioned this pull request May 22, 2025

Support java.io, java.nio.file in trace debugger #649

Merged

Evgeniy Moiseenko (eupp) requested a review from Dmitrii Art (dmitrii-artuhov) May 23, 2025 10:32

Dmitrii Art (dmitrii-artuhov) suggested changes May 28, 2025

View reviewed changes

Comment thread ...t/org/jetbrains/kotlinx/lincheck_test/strategy/modelchecking/UninitializedConstructorTest.kt Outdated

Evgeniy Zhelenskiy (zhelenskiy) force-pushed the allow-instrumenting-constructors branch 2 times, most recently from f6b4412 to 524f42e Compare June 10, 2025 23:29

Instruct constructor bodies

7b0bda6

Evgeniy Zhelenskiy (zhelenskiy) force-pushed the allow-instrumenting-constructors branch from 524f42e to 7b0bda6 Compare June 11, 2025 18:14

Conversation

Evgeniy Zhelenskiy (zhelenskiy) commented May 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nikita Koval (ndkoval) May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Evgeniy Zhelenskiy (zhelenskiy) May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Evgeniy Moiseenko (eupp) commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dmitrii Art (dmitrii-artuhov) commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Evgeniy Moiseenko (eupp) commented May 27, 2025

Uh oh!

Dmitrii Art (dmitrii-artuhov) commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Evgeniy Zhelenskiy (zhelenskiy) commented May 27, 2025

Uh oh!

Dmitrii Art (dmitrii-artuhov) commented May 27, 2025

Uh oh!

Evgeniy Zhelenskiy (zhelenskiy) commented May 27, 2025

Uh oh!

Dmitrii Art (dmitrii-artuhov) commented May 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Nikita Koval (ndkoval) May 20, 2025 •

edited

Loading

Evgeniy Zhelenskiy (zhelenskiy) May 24, 2025 •

edited

Loading

Evgeniy Moiseenko (eupp) commented May 20, 2025 •

edited

Loading

Dmitrii Art (dmitrii-artuhov) commented May 27, 2025 •

edited

Loading

Dmitrii Art (dmitrii-artuhov) commented May 27, 2025 •

edited

Loading