Support step filters when stepping by testforstephen · Pull Request #106 · microsoft/java-debug

testforstephen · 2017-11-14T07:44:35Z

Expose debugFilters options in launch.json to allow user to customize the filters to debug "Just My Code".

The peer PR in vscode side is microsoft/vscode-java-debug#155

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

…tepfilter

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

…to jinbo_stepfilter

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

yaohaizh · 2017-11-22T08:26:10Z

        }
    }

+    /**


Better to put another class ThreadUtility. There will be StackFrameUtility definitely. If all utility in DebugUtility, this file will be bloated.

There are many places to reference to these methods. It would cause many changed files in this PR. Prefer to use a separate PR to do this.

yaohaizh · 2017-11-22T08:27:20Z

                debugEvent.shouldResume = false;
            }
        } else if (event instanceof StepEvent) {
-            ThreadReference stepThread = ((StepEvent) event).thread();


If do nothing, just remove this condition switch

nice.
done.

yaohaizh · 2017-11-22T08:28:07Z

        context.setAttached(true);
        context.setSourcePaths(attachArguments.sourcePaths);
        context.setDebuggeeEncoding(StandardCharsets.UTF_8); // Use UTF-8 as debuggee's default encoding format.
+        context.setDebugFilters(attachArguments.debugFilters);


A thought about AttachRequesthandler & LanuchRequestHandler. There are lots of duplicate code from these two classes. We need refactor out a common base in this case.

nice. Will add some preProcess and postProcess function to handle the common logic.

When implementing launchInTerminal, i have changed the launch logic, would prefer to refactor common base of the attach/launch handler there.
Will send out new PR later.

yaohaizh · 2017-11-22T08:33:23Z

+        return threadStates.get(threadId);
+    }
+
+    private void setPendingStepKind(ThreadReference thread, int kind) {


Should be another ThreadUtility class if you already put something into the DebugUtility.

At the ThreadsRequestHandler constructor, i create a thread state map private Map<Long, ThreadState> threadStates = new HashMap<>();. This method is used to update the state map.

yaohaizh · 2017-11-22T08:35:08Z

        ThreadReference thread = DebugUtility.getThread(context.getDebugSession(), arguments.threadId);
        if (thread != null) {
-            DebugUtility.stepInto(thread, context.getDebugSession().getEventHub());
+            setPendingStepKind(thread, StepRequest.STEP_INTO);


There is already use StepRequest.STEP_INOT/STEP_OVER in the DebugUtility. I would suggest put the same logic either on inside DebugUtility.stepxxx or all on here. But not on different method call stacks.

This logic is to record the thread's stepping state, and putting the code in ThreadRequestHandler is reasonable because ThreadRequestHandler life cycle is debug session dependent.

yaohaizh · 2017-11-22T08:38:11Z

+        return !methodShouldBeFiltered(originalLocation.method(), context) && methodShouldBeFiltered(currentLocation.method(), context);
+    }
+
+    private boolean methodShouldBeFiltered(Method method, IDebugAdapterContext context) {


Naming ShouldFilterMethod for concice

yaohaizh · 2017-11-22T08:38:17Z

+    /**
+     * Return true if the StepEvent's location is a Method that the user has indicated to filter.
+     */
+    private boolean locationShouldBeFiltered(ThreadReference thread, IDebugAdapterContext context) {


ShouldFilterLocation

akaroml · 2017-11-17T06:53:49Z

+     *            the step filters when stepping.
+     * @return the created {@link StepRequest}.
     */
-    public static CompletableFuture<Location> stepOver(ThreadReference thread, IEventHub eventHub) {


Why do we need to change the return value?

One common practice of changing interface is to overload the method by adding parameters. So the old method can call the new method. In this way, the change happens internally without affecting other parts.

In this case, a new method can be added:
public static CompletableFuture<Location> stepOver(ThreadReference thread, IEventHub eventHub, String[] stepFilters)
And in the old method, we change the implementation to call the new one with null or empty string array.

good suggestion.

…to jinbo_stepfilter

akaroml · 2017-11-27T07:30:13Z

+            context.getDebugSession().getEventHub().stepEvents().subscribe(debugEvent -> {
+                handleDebugEvent(debugEvent, context.getDebugSession(), context);
+            });
+        } else {


We can just return here so the "else" branch is not needed.

nice. fixed.

akaroml · 2017-11-27T07:31:09Z

+                setOriginalStackDepth(thread);
+                setOriginalStepLocation(thread);
+                if (command == Command.STEPIN) {
+                    DebugUtility.stepInto(thread, context.getDebugSession().getEventHub(), context.getDebugFilters().stepFilters);


Why isn't anyone using the returned request object?

fixed. the latest change has used the stepRequest info.

akaroml · 2017-11-27T07:38:53Z

+import com.sun.jdi.request.StepRequest;
+
+public class StepRequestHandler implements IDebugRequestHandler {
+    private Map<Long, ThreadState> threadStates;


Who is responsible for removing thread state objects from the collection?

Add logic to remove thread state when ThreadDeathEvent occurs.

akaroml · 2017-11-27T10:38:20Z

+                logger.log(Level.SEVERE, failureMessage);
+                return AdapterUtils.createAsyncErrorResponse(response, ErrorCode.STEP_FAILURE, failureMessage);
+            } catch (IndexOutOfBoundsException ex) {
+                final String failureMessage = String.format("Failed to step because the thread %d doesn't contain any stack frame", threadId);


When would this happen?

At first, thread.frame(i) claims it could throw IndexOutOfBoundsException. And i found a special case that could get this exception. That are system thread Attach Listener and Signal Dispatcher, when pausing them, you could find their stack frames are empty.

akaroml · 2017-11-27T10:40:31Z

+        ThreadReference thread = DebugUtility.getThread(context.getDebugSession(), threadId);
+        if (thread != null) {
+            try {
+                setPendingStepKind(threadId, command);


Do we need to check the thread status before setting it? What if the thread state is already set by other requests?

akaroml · 2017-11-27T10:49:22Z

+            if (pendingStepRequest != null) {
+                DebugUtility.deleteEventRequestSafely(debugSession.getVM().eventRequestManager(), pendingStepRequest);
+            }
+            setPendingStepRequest(threadId, null);


Why not just removeThreadState()?

akaroml · 2017-11-27T10:50:35Z

+            removeThreadState(threadId);
+        } else if (event instanceof StepEvent) {
+            debugEvent.shouldResume = false;
+            ThreadReference thread = ((StepEvent) event).thread();


The space before "event" is not necessary.

Do you mean the whitespace between (StepEvent) and event? checkstyle will need add a space there.

akaroml · 2017-11-27T10:52:06Z

+            debugEvent.shouldResume = false;
+            ThreadReference thread = ((StepEvent) event).thread();
+            long threadId = thread.uniqueID();
+            setPendingStepRequest(threadId, null); // clean up the pending status.


We need to dispose the request before setting it to null, right?

No need to dispose it because DebugUtility.stepXXX method also subscribes StepEvent and it will dispose the StepRequest via JDI.

akaroml · 2017-11-27T10:56:09Z

+                try {
+                    if (getPendingStepKind(threadId) == Command.STEPIN) {
+                        // Check if the step into operation stepped through the filtered code and stopped at an un-filtered location.
+                        if (getOriginalStackDepth(threadId) + 1 < thread.frameCount()) {


Didn't quite get the logic here. Let's sync more tomorrow.

akaroml · 2017-11-27T10:56:14Z

+                        }
+                        // If the ending step location is filtered, or same as the original location where the step into operation is originated,
+                        // do another step of the same kind.
+                        if (shouldFilterLocation(thread, context) || shouldDoExtraStepInto(thread)) {


Didn't quite get the logic here. Let's sync more tomorrow.

akaroml · 2017-11-27T10:57:25Z

+        }
+    }
+
+    private void setPendingStepKind(long threadId, Command kind) {


Kind -> Type

akaroml · 2017-11-27T10:58:53Z

    }

+    public static class DebugFilters {
+        public String[] stepFilters = new String[0];


stepFilters -> classNameFilters

akaroml · 2017-11-27T10:59:28Z

        public boolean supportsRunInTerminalRequest;
    }

+    public static class DebugFilters {


DebugFilters -> StepFilters

andxu · 2017-12-06T05:10:10Z

+                            context.getStepFilters().classNameFilters);
+                }
+                threadState.pendingStepRequest.enable();
+                thread.resume();


use DebugUtility.resumeThread

andxu · 2017-12-06T05:19:41Z

+    }
+
+    private boolean shouldFilterMethod(Method method, IDebugAdapterContext context) {
+        if ((context.getStepFilters().skipStaticInitializers && method.isStaticInitializer())


use simple return

akaroml · 2017-12-06T07:53:05Z

+        StepRequest pendingStepRequest = null;
+        int stackDepth = -1;
+        Location stepLocation = null;
+        Disposable disposable = null;


disposable -> eventSubscription

testforstephen added 4 commits November 10, 2017 15:34

Init commit for step filter

07ec2c3

Add debug filter options in launch.json

61dfc24

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

Refactor config names

24cb8c7

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

fix javadoc

6036641

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

testforstephen mentioned this pull request Nov 14, 2017

Support step filter when stepping microsoft/vscode-java-debug#155

Merged

testforstephen added 3 commits November 15, 2017 09:57

remove unused JDIMethod

00086a0

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

Merge branch 'master' into jinbo_stepfilter

c1475c2

Merge branch 'master' of github.com:Microsoft/java-debug into jinbo_s…

ce91adc

…tepfilter

andxu previously approved these changes Nov 17, 2017

View reviewed changes

Merge branch 'master' into jinbo_stepfilter

45faae5

testforstephen requested review from akaroml and yaohaizh November 18, 2017 09:17

testforstephen added 2 commits November 22, 2017 10:51

resolve merge conflicts

6b1002c

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

Merge branch 'jinbo_stepfilter' of github.com:Microsoft/java-debug in…

586d83b

…to jinbo_stepfilter

testforstephen dismissed andxu’s stale review via 586d83b November 22, 2017 02:52

Remove unnecessary blank space

7878d81

Signed-off-by: Jinbo Wang <jinbwan@microsoft.com>

yaohaizh suggested changes Nov 22, 2017

View reviewed changes

Merge branch 'master' into jinbo_stepfilter

86e3b4a

akaroml requested changes Nov 22, 2017

View reviewed changes

testforstephen added 3 commits November 23, 2017 15:26

Refactor step handler to a standalone StepRequestHandler

d6023f3

Merge branch 'jinbo_stepfilter' of github.com:Microsoft/java-debug in…

0656a3c

…to jinbo_stepfilter

resolve merge conflicts

c9cc031

yaohaizh previously approved these changes Nov 24, 2017

View reviewed changes

akaroml requested changes Nov 27, 2017

View reviewed changes

Add logic to clean up stale thread states and step request

af7d6ee

testforstephen dismissed yaohaizh’s stale review via af7d6ee November 27, 2017 09:41

akaroml requested changes Nov 27, 2017

View reviewed changes

testforstephen added 3 commits November 28, 2017 11:00

fix review comments

6431ce9

Fix the concurrent issue for step request

fdc62f1

Use stateless StepRequestHandler to avoid concurrent issue

6ed3189

testforstephen added 2 commits December 4, 2017 16:38

resolve merge conflicts

3fa4aed

Merge branch 'master' into jinbo_stepfilter

9e4968b

andxu reviewed Dec 6, 2017

View reviewed changes

akaroml requested changes Dec 6, 2017

View reviewed changes

akaroml and others added 2 commits December 6, 2017 15:53

Merge branch 'master' into jinbo_stepfilter

0e4efdb

Resolve review comments

a91a16a

akaroml approved these changes Dec 6, 2017

View reviewed changes

testforstephen merged commit 2828907 into master Dec 6, 2017

testforstephen deleted the jinbo_stepfilter branch December 6, 2017 10:37

testforstephen mentioned this pull request Dec 7, 2017

Support step filter when stepping microsoft/vscode-java-debug#153

Closed

offer8 mentioned this pull request Dec 8, 2017

Only one step request allowed per thread error #120

Closed

testforstephen mentioned this pull request Dec 18, 2017

Should clean up the stale StepRequest microsoft/vscode-java-debug#100

Closed

                       }
                   }
+                  /**

Conversation

testforstephen commented Nov 14, 2017

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!

testforstephen Nov 23, 2017 • 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!

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!

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!

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

testforstephen Nov 23, 2017 •

edited

Loading