Bug 528145 - Breakpoints are not working with remote attach launch

[umairsair]

Looking at the logs, it seems that the regression is caused at 8bec791
where support for multi-process was added. We removed breakpoints
tracking support from final launch sequence and moved it to debug new
process and attach to process logic but none of these are run for remote
attach launch, hence breakpoint tracking is not started for remote
attach launch.

To fix the problem, IGDBProcesses.attachDebuggerToProcess(..) is updated
to handle remote attach launch as well instead of final launch sequence
handling it.

This commit is created after reverting 7bddb5f and 96839a0 which is the
older fix done to fix this issue and the other commit was to fix the
regression caused by the old fix.

The problem with older fix was that for non-stop mode, attach to process
was not working for remote launches when there is already a process
being debugged. Note that to use this feature, gdbserver should be
started with --multi option.

[jonahgraham]

I have added me and a couple of others to review this change. If neither of the other two can look soon I will schedule a deeper dive on this next week.

[jld01]

These changes look reasonable to me. It appears that the GDBJtagDSFFinalLaunchSequence will not be affected but perhaps @umairsair could confirm that he has considered this case.

[umairsair]

These changes look reasonable to me. It appears that the GDBJtagDSFFinalLaunchSequence will not be affected but perhaps @umairsair could confirm that he has considered this case.

Yes, it does not impact.. I tried "GDB Hardware Debugging" launch and it seems to work fine.. it is not attach launch, so it is not impacted..

[umairsair]

@jonahgraham a kind reminder..

[jonahgraham]

@jonahgraham a kind reminder..

Thanks - sorry it hasn't made it to the top of my list.

@jld01 if you feel your review is sufficient, please go ahead and merge it.

[jonahgraham]

I don't think I understand the scope of the problem. Please can you provide what the steps are to reproduce this problem?

I started gdbserver --multi :1234 and made a C/C++ Attach to Application with Debugger as gdbserver in Debugger tab, and set to non-stop. I am able to Debug New Executable multiple times.

I am using gdb/gdbserver 10.2 on linux.

[jonahgraham]

I am able to Debug New Executable multiple times.

To clarify, I was able to debug new exec multiple times without this patch.

[umairsair]

I am able to Debug New Executable multiple times.

To clarify, I was able to debug new exec multiple times without this patch.

The problem is with connecting to existing process.

1. Start a process with debug new exec.
2. Now select connect and ask debugger to connect to any process, it'll not connect and process requested to be attached will not appear in debug view..

Description in first comment also mentions the same..

The problem with older fix was that for non-stop mode, attach to process
was not working for remote launches when there is already a process
being debugged.

[umairsair]

corrected last comment..

[jonahgraham]

Thanks for step-by-step. I can reproduce it and see it is fixed. Now back to the review.

[jonahgraham]

Approved - subject to fixing up the API problems, which I think can be done like this:

$ git diff
diff --git a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/FinalLaunchSequence.java b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/FinalLaunchSequence.java
index 67ee548755..19b7786043 100644
--- a/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/FinalLaunchSequence.java
+++ b/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/FinalLaunchSequence.java
@@ -134,6 +134,7 @@ public class FinalLaunchSequence extends ReflectionSequence {
                                        //
                                        // "stepSetSourceLookupPath",   //$NON-NLS-1$
 
+                                       "stepRemoteConnection", //$NON-NLS-1$
                                        // For all launches except attach ones
                                        "stepNewProcess", //$NON-NLS-1$
                                        // For all attach launch only
@@ -546,6 +547,17 @@ public class FinalLaunchSequence extends ReflectionSequence {
                rm.done();
        }
 
+       /**
+        * This method used to be used to handle remote connections, that code has been folded
+        * into stepAttachToProcess which uses the equivalent implementation in
+        * fProcService.attachDebuggerToProcess
+        */
+       @Execute
+       @Deprecated(forRemoval = true)
+       public void stepRemoteConnection(final RequestMonitor rm) {
+               rm.done();
+       }
+
        /**
         * Start a new process if we are not dealing with an attach session
         * i.e., a local session, a remote session or a post-mortem (core) session.

and the same for the other removed method. I don't know how important it is for it to be in the array still.

This needs entry in N&N. Also as I recommend we deprecate for removal, in the CHANGELOG-API.md too.

Let me know if you need help or guidance on writing these items.

[jonahgraham]

The final thing is it would be nice to have a test that covered this use case. It is somewhat out of the ordinary and I think it has a high chance of regressing again. I don't know what it would take to test, but I would consider looking at https://github.com/eclipse-cdt/cdt/blob/5007c8b3371a77ccd389660c24893d8f89b6579c/dsf-gdb/org.eclipse.cdt.tests.dsf.gdb/src/org/eclipse/cdt/tests/dsf/gdb/tests/nonstop/GDBMultiNonStopRunControlTest.java

[umairsair]

Approved - subject to fixing up the API problems, which I think can be done like this:

what will happen if a sub-class is overriding stepRemoteConnection and doing connection in its own way? this may cause unexpected behaviors because now connection is being done in attachDebuggerToProcess..

[jonahgraham]

Yes it may, hence N&N entry. I am certainly open to ideas that improve the situation, but the API contract of these methods is pretty undefined.

[umairsair]

what could be the issues if we remove the API? I think its better that we get compile time error instead of getting strange runtime issue and others might be reporting issues..

[jonahgraham]

You get runtime errors when removing API too. We can add an API exception and remove the methods.

It is a bit of a balancing act, and I'm not thrilled by the over exporting of API in this case that we are stuck with forever.

If we stay true to the API you should restructure the change. One idea is that we pull out the remaining method and call it from the other API methods with flags to say which behavior to do. But that seems like a lot of contrived code.

The other option is to create a whole new FinalLaunchSequence and deprecate the existing one with no changes, like we did for gnumakefilebuilder, and then use the new FinalLaunchSequence.

[umairsair]

BTW should I squash all commits or keep the revert commits separate?

[jonahgraham]

You can squash them, or I will when I submit them. I'll keep the commit messages of all the squashed commits because the reference to when reverted commits are included is useful.

[Torbjorn-Svensson]

Sorry to be late on the review.
I've run this with the way we are launching remote sessions, and it appears to work as expected.

Other than a few questions and a typo, I think this is good to go.

[jonahgraham]

Thank you @umairsair for creating these tests.

[jonahgraham]

I am about to provide some new commits. @umairsair if you are around today and can review them that would be great, if not and the tests pass I am going to merge this so it can get in M1 today and we can pick up additional work later if required.

[umairsair]

LGTM.. Thanks @jonahgraham !

test failure seems a glitch.. can you please re-trigger it?

Update: there was a minor bug, I have fixed it..

[jonahgraham]

Thank you @umairsair for this work. And an extra thank you for writing the new tests, I think that will really help keep this stable as other use cases are worked on.

[jonahgraham]

Just to confirm that I think we made the right decision with how this is implemented now I have come across this code that extends CDT (modified slightly for privacy):

	@Override
	protected String[] getExecutionOrder(String group) {
		if (GROUP_TOP_LEVEL.equals(group)) {
			// Initialize the list with the steps from the base class
			List<String> orderList = new ArrayList<>(Arrays.asList(super.getExecutionOrder(GROUP_TOP_LEVEL)));
			orderList.add(orderList.indexOf("stepAttachRemoteToDebugger") + 1, "newstep")

In this case, "newstep" would have been added as the first item in the launch sequence had we removed stepAttachRemoteToDebugger fully.