implement Jump to Line #1042

Trass3r · 2020-09-06T16:42:58Z

fixes microsoft/vscode-cpptools#1025

src/MICore/CommandFactories/gdb.cs

src/MICore/CommandFactories/lldb.cs

src/OpenDebugAD7/AD7DebugSession.cs

Trass3r · 2020-09-06T19:17:22Z

Done.
Interestingly it won't return multiple targets as gdb's info line only returns one for:

template <typename T>
int foo(T t)
{
    return 0;
}

src/OpenDebugAD7/AD7DebugSession.cs

gregg-miskelly · 2020-09-08T18:04:45Z

src/OpenDebugAD7/AD7DebugSession.cs

+            var response = new GotoTargetsResponse();
+
+            var source = responder.Arguments.Source;
+            // TODO: handle this for disassembly debugging


// TODO: handle this for disassembly debugging [](start = 12, length = 46)

I feel like the right want to handle disassembly is to add a gotoInstruction request which would take a instructionReference and offset (like "InstructionBreakpoint")

src/MIDebugEngine/AD7.Impl/AD7Thread.cs

src/MIDebugEngine/AD7.Impl/AD7MemoryAddress.cs

src/MIDebugEngine/AD7.Impl/AD7Engine.cs

src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs

gregg-miskelly · 2020-09-08T18:16:41Z

src/MICore/CommandFactories/MICommandFactory.cs

+        /// <summary>
+        /// Jumps to a specified target location
+        /// </summary>
+        abstract public Task ExecJump(string filename, int line);


abstract public Task ExecJump(string filename, int line); [](start = 8, length = 57)

Same

src/DebugEngineHost.VSCode/VSCode/HandleCollection.cs

src/OpenDebugAD7/AD7DebugSession.cs

WardenGnaw

Made some fixes in 9696770

@gregg-miskelly

We get some weirdness since GoToRequest assumes the process is always stopped but the way GDB handles -exec-jump, it causes it to resume the process, so a temporary breakpoint is used.
However, in between the GoTo Request and before we get stopped at the temporary breakpoint, we get a StackTraceRequest. But since we are not stopped, we return no frames.

Thoughts on how to address this issue and my other comment about freezing other threads in a comment below?

WardenGnaw · 2020-10-29T00:33:12Z

src/MICore/CommandFactories/gdb.cs

+        private async Task JumpInternal(string target)
+        {
+            // temporary breakpoint + jump
+            await _debugger.CmdAsync("-break-insert -t " + target, ResultClass.done);


With this temporary breakpoint not registered, when we get a stopping event it will be unknown and returned as an exception.

MIEngine/src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs

Line 1179 in 728f403

_callback.OnException(thread, "Unknown breakpoint", "", 0);

I think we need to treat this as an async break.

What do you mean by not registered?

On stopping and when it is a breakpoint, we go through a list of breakpoints:

MIEngine/src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs

Line 1132 in 728f403

AD7BoundBreakpoint[] bkpt = _breakpointManager.FindHitBreakpoints(bkptno, addr, frame, out fContinue);

If if there are none, we assume it is an entrypoint breakpoint, embedded, we hit a bp pending deletion, or it is an exception.

Indeed. It does receive a breakpoint-modified event and disp is "del" fwiw.

<-1067-break-insert -t *0x5555555573B1 ->1067^done,bkpt={number="3",type="breakpoint",disp="del",enabled="y",addr="0x00005555555573b1",func="main(int, "... <-1068-exec-jump *0x5555555573B1 ->1068^running ->*running,thread-id="all" ->=breakpoint-modified,bkpt={number="3",type="breakpoint",disp="del",enabled="y",addr="0x00005555555573b1",func="main(int, "... ->*stopped,reason="breakpoint-hit",disp="del",bkptno="3",frame={addr="0x00005555555573b1",func="main",args=[{name="argc",value="1"},{name="argv",value="0x7fffffffdb68"}],file="../test.cpp",fullname="/tmp/test.cpp",line="28",arch="i386:x86-64"},thread-id="1",stopped-threads="all",core="6" ->=breakpoint-deleted,id="3"

src/MICore/CommandFactories/gdb.cs

Trass3r · 2020-11-06T15:55:15Z

src/OpenDebugAD7/AD7DebugSession.cs

                        }
-                        targets.Add(new GotoTarget(m_gotoCodeContexts.Create(codeContext), contextName, line));
+
+                        int codeContextId = m_nextContextId++;


Wouldn't this need to be atomic if multiple simultaneous requests are expected?

gregg-miskelly · 2021-01-08T19:17:45Z

src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs

+        public async Task Jump(string filename, int line)
+        {
+            await MICommandFactory.ExecJump(filename, line);
+        }


I think we want to remove all the overloads that use filename and line number since we always seem to use the overload that take the address.

src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs

src/OpenDebugAD7/AD7DebugSession.cs

gregg-miskelly · 2021-01-11T19:12:34Z

src/OpenDebugAD7/AD7DebugSession.cs

+                    }
+                    BeforeContinue();
+                    builder.CheckHR(thread.SetNextStatement(null, gotoTarget));
+                }


} [](start = 16, length = 1)

We need to throw if this if is false.

WardenGnaw · 2021-01-29T02:10:18Z

We are looking into splitting GoToTargets and the GoTo Request.

Trass3r · 2021-01-29T22:23:19Z

@WardenGnaw Squashed and also removed the obsolete resource string.

I also added a test commit using set $pc = instead of the jump command (see https://sourceware.org/gdb/current/onlinedocs/gdb/Jumping.html).
This avoids all the potential temporary breakpoint problems discussed earlier but lacks the sanity checks that lead to Unreasonable jump request if jumping to a different function for example. We have some checks in SetNextStatement/CanSetNextStatement which are VS-only though. I guess that filtering should be done in the GotoTargets request already.

Other than that gdb's jump cmd is equal to set $pc + continue it seems and lldb's is just set $pc under the hood.

Trass3r · 2021-01-30T01:03:48Z

src/MIDebugEngine/AD7.Impl/AD7Engine.cs

@@ -216,6 +216,7 @@ public int Jump(ulong address)
                _engineCallback.OnError(EngineUtils.GetExceptionDescription(e));
                return Constants.E_ABORT;
            }
+            DebuggedProcess.ThreadCache.MarkDirty();


This was needed to update the current line in the UI. Is it the right place?

Trass3r · 2021-01-30T12:01:14Z

src/OpenDebugAD7/AD7DebugSession.cs

@@ -1344,7 +1344,7 @@ protected override void HandleGotoRequestAsync(IRequestResponder<GotoArguments>
                        if (!m_threads.TryGetValue(responder.Arguments.ThreadId, out thread))
                            throw new AD7Exception("Unknown thread id: " + responder.Arguments.ThreadId.ToString(CultureInfo.InvariantCulture));
                    }
-                    BeforeContinue();
+                    //BeforeContinue();
                    builder.CheckHR(thread.SetNextStatement(null, gotoTarget));


Ok to remove?

You probably want BeforeContinue. The other debug adapter that the VS debugger team owns currently doesn't clear our handle collections on SetNextStatement, but you are probably correct that it should. But we should check VS Code and make sure it is happy.

Yeah idk technically it doesn't run the program but on the other hand the current line of code needs to be updated. The thread cache invalidation worked in VSCode.

Trass3r · 2021-01-30T12:02:34Z

src/MICore/CommandFactories/gdb.cs

@@ -201,6 +201,8 @@ public override Task ExecJump(string filename, int line)

        public override Task ExecJump(ulong address)
        {
+            return _debugger.ConsoleCmdAsync("set $pc = " + string.Format(CultureInfo.InvariantCulture, "0x{0:X}", address), false);


The question is if some kind of error handling is needed.

Ideally yes, but since this is a non-MI command, there aren't wonderful options for knowing it is succeeded. I am not sure if GDB supports assigning to $pc through MI commands instead (-var-assign?)

Looks like it but requires a variable object incl. creation/deletion (possibly just once): https://sourceware.org/gdb/current/onlinedocs/gdb/GDB_002fMI-Variable-Objects.html

FabianoGK · 2021-03-12T08:33:20Z

If I may ask, what is the state of this pull request? Is there additional things to discuss?
I'm really excited to see this ready for vscode-cppdbg!

Trass3r · 2021-03-12T08:48:23Z

See all the comments after my last commit.

FabianoGK · 2021-03-13T00:48:31Z

Eclipse GDB debugger has two commands around -exec-jump: Move to Line, and Resume at Line.

In the Move to Line, it does insert a breakpoint and then jumps:

480,319 100-break-insert -t -p 1 -f /projects/test/test.cpp:160
480,326 100^done,bkpt={number="7",type="breakpoint",disp="del",enabled="y",addr="0x0004fb78",func="main(int, char**)",file="../src/test.cpp",fullname="/projects/test/test.cpp",line="160",thread-groups=["i1"],thread="1",thread="1",times="0",original-location="/projects/test/test.cpp:160"}
480,326 (gdb) 
480,327 101-exec-jump --thread 1 /projects/test/test.cpp:160
480,333 101^running
480,333 *running,thread-id="all"
480,333 (gdb) 
480,345 =breakpoint-modified,bkpt={number="7",type="breakpoint",disp="del",enabled="y",addr="0x0004fb78",func="main(int, char**)",file="../src/test.cpp",fullname="/projects/test/test.cpp",line="160",thread-groups=["i1"],thread="1",thread="1",times="1",original-location="/projects/test/test.cpp:160"}
480,365 ~"\nTemporary breakpoint "
480,366 ~"7, main (argc=1, argv=0xbefffd94) at ../src/test.cpp:160\n"
480,367 ~"160\t\tTest();\n"
480,367 *stopped,reason="breakpoint-hit",disp="del",bkptno="7",frame={addr="0x0004fb78",func="main",args=[{name="argc",value="1"},{name="argv",value="0xbefffd94"}],file="../src/test.cpp",fullname="/projects/test/test.cpp",line="160"},thread-id="1",stopped-threads="all",core="0"
480,367 =breakpoint-deleted,id="7"

In the Resume at Line, it just does the jump:

688,000 171-exec-jump --thread 1 /projects/test/test.cpp:131
688,009 171^running
688,009 *running,thread-id="all"
688,009 (gdb)

Visual Studio Set Next Statement behavior is the same as Eclipse's Move to Line. In other words, it does not continue the program execution.

Trass3r · 2021-05-04T21:49:22Z

480,327 101-exec-jump --thread 1 /projects/test/test.cpp:160
480,333 101^running
480,333 *running,thread-id="all"

That --thread parameter doesn't seem to exist:
https://sourceware.org/gdb/current/onlinedocs/gdb/GDB_002fMI-Program-Execution.html

FabianoGK · 2021-05-04T22:27:27Z

The Context Management talks about the ‘--thread’ parameter:

each MI command should explicitly specify which thread and frame to operate on. To make it possible, each MI command accepts the ‘--thread’ and ‘--frame’ options, the value to each is GDB global identifier for thread and frame to operate on.

https://sourceware.org/gdb/current/onlinedocs/gdb/Context-management.html#Context-management

Trass3r · 2021-05-05T06:21:22Z

Interesting. But it doesn't make a difference here as it still runs all threads:

*running,thread-id="all"

So it could still theoretically break in another thread first.
But if Eclipse is using the same temp bp method it probably doesn't happen in practice.

FabianoGK · 2021-05-05T16:01:38Z

Interesting. But it doesn't make a difference here as it still runs all threads:
*running,thread-id="all"
So it could still theoretically break in another thread first.
But if Eclipse is using the same temp bp method it probably doesn't happen in practice.

Interesting observation indeed. I had to turn on non-stop mode in order to behave as expected:

1: (53481) <-1151-exec-jump --thread 2 test.cpp:24
1: (53483) ->1151^running
1: (53483) ->*running,thread-id="2"

Trass3r · 2021-05-06T21:02:08Z

And there's also scheduler-locking mode, very convoluted.

aallrd · 2021-11-05T17:39:33Z

Hello,
What is still required to merge this development?

Trass3r · 2021-11-05T18:08:04Z

See all the open discussions. There were some points without final resolution.

Trass3r force-pushed the goto branch from b2e7965 to fd57886 Compare September 6, 2020 16:49

WardenGnaw requested changes Sep 6, 2020

View reviewed changes

Trass3r force-pushed the goto branch from 3c484aa to a4cdd47 Compare September 6, 2020 19:16