KillException and QuitException handling #98

mlvdv · 2016-03-05T00:14:55Z

Clarify and document required behavior for WrapperNodes in the instrumentation framework. Fix a related bug in ProbeNode. Add tests for correct handling.
@chumer please have a look at tests failing for automatically generated wrappers

…rNode behavior

…robeNode

…QuitException

chumer · 2016-03-07T11:34:20Z

It was intentional that KillException and QuitException was not supported for normal instruments only for language instruments.

mlvdv · 2016-03-07T22:28:40Z

Those exceptions are required by the Debugger, which is not a language instrument and which is now broken.

@chumer suggests replacing the exceptions and I agree; see new issue #109.

Meanwhile, this patch is needed to keep the current debugger working correctly until a replacement is in place.

I could simplify the current situation by removing any dependence onQuitException. Only the functionality of KillException is actually required. Is that worth doing?

chumer · 2016-03-09T11:11:35Z

I cannot really come up with a better solution.
So lets only support KillException (CH1) and deprecate QuitException (CH2).

chumer · 2016-03-09T11:16:25Z

...oracle.truffle.api.instrumentation/src/com/oracle/truffle/api/instrumentation/ProbeNode.java


        protected abstract void innerOnDispose(EventContext context, VirtualFrame frame);

        final void onEnter(EventContext context, VirtualFrame frame) {


CH3) onDispose should also support KillException. Any reason not to?

chumer · 2016-03-09T12:36:49Z

@mlvdv Please address CH1-6.

mlvdv · 2016-03-10T01:50:10Z

CH1-2) Already suggest this possibility, so agree.
CH3) Not clear to me this would occur, but for consistency agree
CH4) Agree, as long as semantically equivalent.
CH5) Yes, fixing wrapper generation is what I was requesting. No, a Kill exception must not generate an instrumentation event. Kill means that guest language execution terminates immediately; a subsequent guest language execution event would be meaningless and misleading.
CH6) Either way would work for me, but this makes several of the above moot so there's no point acting on them yet.

MLVDV1) Is there a pattern already documented for what IOException means and how it should be used? Should it hold referred exceptions as you suggest, but why not subclassing? Is such a discussion out of scope for this PR, which is a bug fix on existing code?

woess · 2016-03-10T07:31:49Z

CH5) Yes, fixing wrapper generation is what I was requesting. No, a Kill exception must not generate an instrumentation event.

Isn't this something onReturnExceptional can and should handle? I'd rather have the logic in the handler, and the wrapper as simple as possible.

chumer · 2016-03-10T10:38:38Z

+1 with @woess . Having it in the wrapper would propagate complexity to all languages.

smarr · 2016-03-10T10:50:18Z

Perhaps I am missing something, but I'd actually argue the other way.

If there is an exit/quit 'event', why do I need to handle that in my instrumentation?

This seems to be something that should be handled by the framework/wrappers. I think, I only want language-specific events in the onReturnExceptional(). At least for my current tool, these seems to be the ones I care about. Having to handle exit/quit in tool-specific nodes also seems to ensure that the tools break, because until the recent discussions, I wasn't even aware of these exceptions.

woess · 2016-03-10T11:06:04Z

@smarr: this exception should be handled in the framework (inside ProbeNode) and not propagated to the actual instrumentation observers' onReturnExceptional methods. This way we don't duplicate the catch/rethrow (there are multiple wrapper classes (at least one per language) but only 1 ProbeNode) and effectively make it an implementation detail here.

smarr · 2016-03-10T11:09:18Z

Ah, right, forgot about the ProbeNode, yes, that sounds like the right place. Sorry for the noise.

chumer · 2016-03-10T11:15:56Z

MLVDV1) Is there a pattern already documented for what IOException means and how it should be used? Should it hold referred exceptions as you suggest, but why not subclassing? Is such a discussion out of scope for this PR, which is a bug fix on existing code?

To answer your question: yes it is out of scope of this PR. Lets discuss it here: #116

mlvdv · 2016-03-10T20:28:50Z

This PR address two bugs in the new instrumentation code that break the Debugger. The revisions include a fix for one of them and a request for help with another, supported by additional documentation and a test. Some of the discussion so far is inconsistent with the Debggers's requirements. Summary:

The Debugger requires instrumentation framework support when it must terminate immediately (kill) a guest language execution, at least until issue Debugger requirement: terminate guest language execution immediately #109 is addressed.
The debugger kills an execution by throwing KillException while the guest language program is halted in an execution event notification. This means the Debugger might throw from within any one of the three execution event notifications: onEnter(), onReturnValue(), or onReturnExceptional().
KillException must propagate through any instrumentation support code without generating additional guest language execution events, for reasons described in an earlier comment. Among other things, this means that there must be no more calls to execution event notifications onEnter(), onReturnValue(), or onReturnExceptional(). The code change in this PR fixes a propagation bug in the new ProbeNode implementation.
It makes no sense to talk about handling kill in onReturnExceptional(). The execution event onReturnExceptional() notifies other listeners that a guest language Node has returned via exception, which is something entirely different (as @smarr observes).
Additional documentation in this PR describes what language-specific WrapperNode instances must do to support this Debugger. It is a bug that the new auto-wrapper generation does not propagate KillException correctly, and that is the help wanted part of this PR along with a supporting test. I agree that it would be nice if wrappers did not have this responsibility, but at the moment it is a requirement along with the requirement that other parts of language implementations propagate it correctly.
I also agree that it would be nice if KillException were part of a more general approach to exceptions in PolyglotEngine, but until issue Improve exceptions reported by PolyglotEngine #116 is addressed there is no such thing.

I remain open to suggestions about minor variations in implementation, especially if they relate to fast/slow path.

chumer · 2016-03-14T00:50:27Z

I don't agree to 3. and 4.. It makes sense to have the option to cleanly dispose any open resources one might have opened because of a started event. How would you do that in case of a kill event?

what help do you need with the tests? Your list looks already like the perfect specification.

mlvdv · 2016-03-14T04:31:17Z

I don't agree to 3. and 4.

Your objection to 3 is easily addressed. A language implementation is free to catch a KillException and do what it needs as long as it (1) performs no more guest language execution, (2) generates no more execution event notifications, and (3) re-throws the exception when finished. These are the Debugger's requirements at this time.
Your objection to 4 can only be based on a misunderstanding. From the perspective of a client-generated KillException the event notification method onReturnExceptional() is exactly the same as the other two event notification methods and must behave exactly the same way to such an exception.

what help do you need with the tests? Your list looks already like the perfect specification.

I don't believe I need any help with tests; I have included a test that correctly identifies a bug in WrapperNode generation.
What I am requesting is a fix for the bug in WrapperNode auto-generation.
My fix for a similar bug in the new ProbeNode implementation is included.

The Debugger continues to be broken (9 days now) until these two fixes are in place.

woess · 2016-03-14T13:48:23Z

@mlvdv I think there's a misunderstanding here about what it means to handle KillException in ProbeNode.onReturnExceptional. Just because the WrapperNodes call this method, doesn't mean it has to generate an actual return exceptional event (i.e., notify event handlers). So all your 3 points can be addressed in this way. FWIW, the old implementation used to it this way (i.e., catch and rethrow KillException in ProbeNode.returnExceptional).

I'd like to hide the existence of KillException from the guest language as much as possible. Ideally, it would be just an implementation detail of the instrumentation framework and PolyglotEngine. But at least we should avoid duplication of the catch-rethrow logic and leaking it to the guest language (which wrapper nodes are still part of, even when they're generated). It'll also give us more flexibility to do changes around it in the future.

entlicher · 2016-03-14T17:03:38Z

I just got an idea to use ThreadDeath for that purpose.
If KillException extends ThreadDeath, it does not have to be in the APIs.
Language implementors should be advised to let the ThreadDeath propagate and we can catch it at any time as a KillException, which differentiates it from the real ThreadDeath.
Thoughts?

entlicher · 2016-03-14T17:07:35Z

The Javadoc of ThreadDeath says: "An application should catch instances of this class only if it must clean up after being terminated asynchronously. If ThreadDeath is caught by a method, it is important that it be rethrown so that the thread actually dies."
Therefore it's a good practice to rethrow it already, we just can take the advantage of that.

… ProbeNode.

mlvdv · 2016-03-14T17:51:26Z

CH3) onDispose should also support KillException. Any reason not to?

No reason not to; fixed.

mlvdv · 2016-03-14T17:56:32Z

I'd like to hide the existence of KillException from the guest language as much as possible. Ideally, it would be just an implementation detail of the instrumentation framework and PolyglotEngine. But at least we should avoid duplication of the catch-rethrow logic and leaking it to the guest language (which wrapper nodes are still part of, even when they're generated). It'll also give us more flexibility to do changes around it in the future.

@woess Can this part of the discussion be forwarded to #109 or #116?

woess · 2016-03-14T18:00:06Z

@mlvdv ok

mlvdv · 2016-03-14T22:29:53Z

Apologies to @woess and others. My persistent misinterpretation of an earlier comment kept me from understanding the follow-up until his most recent comment caused me to look back at the code more closely. I withdraw the request for changes to auto-generation of wrappers, and I'll clean up the handling of the exceptions.

…uired to handle KillException or QuitException specially.

… with no dependence for special handling by WrapperNodes.

* master: (41 commits) Remove JavaInteropSpeedTest. Turning checkstyle on for the 'api.vm' project Fix merge problems. Instrumentation: simple local variable rename Instrumentation: rename the two package private implementation classes of Instrumenter for clarity about their roles. Instrumentation: rename the awkwardly named (non-public) class InstrumentationInstrumenter to ClientInstrumenter (for external clients of instrumentation, as opposed to the sibling class LanguageInstrumenter). Documentation cleanup for new Instrumentation code: - Correct many Javadoc uses of obsolete class names (e.g. Instrumentation was renamed to TruffleInstrument) - Rename many variable names accordingly (e.g. "instrumentation" -> "instrument") - Avoid constructs that appear unnatural to native English speakers, e.g. "instrumentations", "instrumenters" - Rework some Javadoc explanations for clarity: more uniform terminology, diction - Fix typos No need to pre-initialize the debugger with Debugger.find, enough to call executionEvent.getDebugger Lazy initialization of debugger once somebody calls ExecutionEvent.getDebugger Make sure ExecutionEvent is always delivered BranchingNode example snippet can reference real Node class Fomatting changes Adding end of line to two files containing snippets Debug: temporary strategy for printing node tags in debugging code, simplified AST printing in debugging code Debug: update for recent changes in stack iteration Temporarily disable instrumentation tests as they seem to fail on JDK9. Fix Truffle archive merging for multiple files. use appropriate line endings when merging properties files Fix Truffle compilation issues with interop.java Ignore JavaInteropSpeedTest (should be converted into a microbenchmark). ... # Conflicts: # truffle/com.oracle.truffle.api.instrumentation/src/com/oracle/truffle/api/instrumentation/ProbeNode.java

chumer · 2016-03-16T19:30:11Z

KillException is still not specified in PolyglotEngine.
Given that KillException was thrown before by PoylgotEngine we can go along I think. We need to do a bigger change with #116 so we can probably also specify this properly with that change.

KillException and QuitException handling

mlvdv · 2016-03-16T21:42:32Z

We need to do a bigger change with #116 so we can probably also specify this properly with that change.

Agreed.

…E.COM/truffle:feature/trufffle-profiler-upgrade to master * commit '5a9dc5d71c1abff093367681d78ed5ec9088df82': Profiler: class Javadoc Profiler: rename TruffleProfilerTest to Profiler test for name consistency Profiler: public methods now throw IllegalStateException if profiler has been disposed. Profiler: invert and rename the Instrument and Client API class, now ProfilerInstrument and Profiler respectively, other minor cleanups and fixes. TruffleProfiler: replace property-based test access with a documented, tested, client API. Allows dynamic changing of parameters specifying what is being collected, for use in interactive contexts.

[Backport][GR-60578] Upgrade ASM dependency from 9.5 to 9.7.1.

mlvdv added 4 commits March 4, 2016 16:03

Instrumentation: expand and correct documentation for required Wrappe…

be1fcf9

…rNode behavior

Instrumentation: fix handling of KillException and QuitException in P…

d762a02

…robeNode

Instrumentation: add tests for correct handling of KillException and …

b76cae7

…QuitException

Instrumentation: Javadoc typos

4bf4dce

mlvdv added bug help wanted oracle-emp labels Mar 5, 2016

mlvdv assigned chumer Mar 5, 2016

chumer reviewed Mar 9, 2016
View reviewed changes

Instrumentation: add correct KillException handling to onDispose() in…

2b9a59a

… ProbeNode.

mlvdv removed the help wanted label Mar 14, 2016

mlvdv added 3 commits March 14, 2016 16:58

Instrumentation: revise documentation for WrapperNodes, no longer req…

1eff220

…uired to handle KillException or QuitException specially.

Instrumentation: handle QuitException and KillException in Probe node…

ef5c4ca

… with no dependence for special handling by WrapperNodes.

Debugging: Deprecate QuitException and remove all uses.

3cea280

jtulach mentioned this pull request Mar 15, 2016

Debugger requirement: terminate guest language execution immediately #109

Closed

mlvdv added 3 commits March 15, 2016 13:56

Instrumentation: Javadoc fix

bf660cb

Merge branch 'master' into kill-quit-exception-handling

a97471d

chumer added the accept label Mar 16, 2016

mlvdv added a commit that referenced this pull request Mar 16, 2016

Merge pull request #98 from mlvdv/kill-quit-exception-handling

42d7370

KillException and QuitException handling

mlvdv merged commit 42d7370 into oracle:master Mar 16, 2016

mlvdv deleted the kill-quit-exception-handling branch March 21, 2016 21:42

brunoborges unassigned chumer Jan 8, 2018

jerboaa pushed a commit to jerboaa/graal that referenced this pull request Aug 21, 2025

Merge pull request oracle#98 from bulasevich/GR-60578

5aa8c45

[Backport][GR-60578] Upgrade ASM dependency from 9.5 to 9.7.1.


		protected abstract void innerOnDispose(EventContext context, VirtualFrame frame);

		final void onEnter(EventContext context, VirtualFrame frame) {

KillException and QuitException handling #98

KillException and QuitException handling #98

Uh oh!

Conversation

mlvdv commented Mar 5, 2016

Uh oh!

chumer commented Mar 7, 2016

Uh oh!

mlvdv commented Mar 7, 2016

Uh oh!

chumer commented Mar 9, 2016

Uh oh!

chumer Mar 9, 2016

Choose a reason for hiding this comment

Uh oh!

chumer commented Mar 9, 2016

Uh oh!

mlvdv commented Mar 10, 2016

Uh oh!

woess commented Mar 10, 2016

Uh oh!

chumer commented Mar 10, 2016

Uh oh!

smarr commented Mar 10, 2016

Uh oh!

woess commented Mar 10, 2016

Uh oh!

smarr commented Mar 10, 2016

Uh oh!

chumer commented Mar 10, 2016

Uh oh!

mlvdv commented Mar 10, 2016

Uh oh!

chumer commented Mar 14, 2016

Uh oh!

mlvdv commented Mar 14, 2016

Uh oh!

woess commented Mar 14, 2016

Uh oh!

entlicher commented Mar 14, 2016

Uh oh!

entlicher commented Mar 14, 2016

Uh oh!

mlvdv commented Mar 14, 2016

Uh oh!

mlvdv commented Mar 14, 2016

Uh oh!

woess commented Mar 14, 2016

Uh oh!

mlvdv commented Mar 14, 2016

Uh oh!

chumer commented Mar 16, 2016

Uh oh!

mlvdv commented Mar 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants