Avoid NPE during instrumentation #8317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes a random crash (*) during instrumentation. Notice how `onTailCallReturn` calls `onReturnValue` with `null` frame. Bonus: noticed that for some reason we weren't getting logs for `ExecutionService`. This turned out to be the problem with the logger name which by default was `[enso]` not `[enso.org.enso.interpreter.service.ExecutionService`] and there is some logic there that normalizes the name and assumed a dot after `enso`. This change fixes the logic. (*) ``` [enso.org.enso.interpreter.service.ExecutionService] Execution of function main failed (Cannot invoke "com.oracle.truffle.api.frame.VirtualFrame.materialize()" because "frame" is null). java.lang.NullPointerException: Cannot invoke "com.oracle.truffle.api.frame.VirtualFrame.materialize()" because "frame" is null at org.enso.interpreter.instrument.IdExecutionInstrument$IdEventNodeFactory$IdExecutionEventNode.onReturnValue(IdExecutionInstrument.java:246) at org.enso.interpreter.instrument.IdExecutionInstrument$IdEventNodeFactory$IdExecutionEventNode.onTailCallReturn(IdExecutionInstrument.java:274) at org.enso.interpreter.instrument.IdExecutionInstrument$IdEventNodeFactory$IdExecutionEventNode.onReturnExceptional(IdExecutionInstrument.java:258) at org.graalvm.truffle/com.oracle.truffle.api.instrumentation.ProbeNode$EventProviderChainNode.innerOnReturnExceptional(ProbeNode.java:1395) at org.graalvm.truffle/com.oracle.truffle.api.instrumentation.ProbeNode$EventChainNode.onReturnExceptional(ProbeNode.java:1031) at org.graalvm.truffle/com.oracle.truffle.api.instrumentation.ProbeNode.onReturnExceptionalOrUnwind(ProbeNode.java:296) at org.enso.interpreter.node.ExpressionNodeWrapper.executeGeneric(ExpressionNodeWrapper.java:119) at org.enso.interpreter.node.ClosureRootNode.execute(ClosureRootNode.java:85) at jdk.internal.vm.compiler/org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.executeRootNode(OptimizedCallTarget.java:718) at jdk.internal.vm.compiler/org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.profiledPERoot(OptimizedCallTarget.java:641) at jdk.internal.vm.compiler/org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callBoundary(OptimizedCallTarget.java:574) at jdk.internal.vm.compiler/org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.doInvoke(OptimizedCallTarget.java:558) at jdk.internal.vm.compiler/org.graalvm.compiler.truffle.runtime.OptimizedCallTarget.callDirect(OptimizedCallTarget.java:504) at jdk.internal.vm.compiler/org.graalvm.compiler.truffle.runtime.OptimizedDirectCallNode.call(OptimizedDirectCallNode.java:69) at org.enso.interpreter.node.callable.thunk.ThunkExecutorNode.doCached(ThunkExecutorNode.java:69) at org.enso.interpreter.node.callable.thunk.ThunkExecutorNodeGen.executeAndSpecialize(ThunkExecutorNodeGen.java:207) at org.enso.interpreter.node.callable.thunk.ThunkExecutorNodeGen.executeThunk(ThunkExecutorNodeGen.java:167) ... ```
hubertp
requested review from
4e6,
JaroslavTulach and
Akirathan
as code owners
hubertp
added
the
CI: No changelog needed
4e6
approved these changes
Nov 16, 2023
| @@ -234,7 +234,7 @@ public void onReturnValue(VirtualFrame frame, Object result) { | |||
| functionCallInstrumentationNode.getId(), | |||
| result, | |||
| nanoTimeElapsed, | |||
| frame.materialize(), | |||
| frame == null ? null : frame.materialize(), | |||
There was a problem hiding this comment.
Kotlin ?. safe call operator would be handy here
There was a problem hiding this comment.
Yes, it would be nice.
There was a problem hiding this comment.
Or do: Optional.of(frame).map(f -> f.materialize()).getOrNull() to get more functional feeling.
Akirathan
approved these changes
Nov 17, 2023
| @@ -72,7 +72,7 @@ public final class ExecutionService { | |||
| private final EnsoContext context; | |||
| private final Optional<IdExecutionService> idExecutionInstrument; | |||
| private final NotificationHandler.Forwarder notificationForwarder; | |||
| private final TruffleLogger logger = TruffleLogger.getLogger(LanguageInfo.ID); | |||
| private final TruffleLogger logger = TruffleLogger.getLogger(LanguageInfo.ID, ExecutionService.class); | |||
There was a problem hiding this comment.
Nice observation. Those [enso] ... log messages were confusing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Fixes a random crash (*) during instrumentation. Notice how
onTailCallReturncallsonReturnValuewithnullframe.Bonus: noticed that for some reason we weren't getting logs for
ExecutionService. This turned out to be the problem with the logger name which by default was[enso]not[enso.org.enso.interpreter.service.ExecutionService] and there is some logic there that normalizes the name and assumed a dot afterenso. This change fixes the logic.(*)
Important Notes
Fixes regressions introduced in #8148 and #8162
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.