Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement Jump to Line #1042

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/MICore/CommandFactories/MICommandFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,12 @@ public async Task ExecNextInstruction(int threadId, ResultClass resultClass = Re
await ThreadFrameCmdAsync(command, resultClass, threadId, 0);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Same

abstract public Task ExecJump(ulong address);

/// <summary>
/// Tells GDB to spawn a target process previous setup with -file-exec-and-symbols or similar
/// </summary>
Expand Down
28 changes: 26 additions & 2 deletions src/MICore/CommandFactories/gdb.cs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ public override async Task<Results> ThreadInfo(uint? threadId = null)

public override async Task<List<ulong>> StartAddressesForLine(string file, uint line)
{
string cmd = "info line " + file + ":" + line;
string cmd = "info line -s " + file + " -li " + line;
var result = await _debugger.ConsoleCmdAsync(cmd, allowWhileRunning: false);
List<ulong> addresses = new List<ulong>();
using (StringReader stringReader = new StringReader(result))
Expand All @@ -173,7 +173,7 @@ public override async Task<List<ulong>> StartAddressesForLine(string file, uint
{
ulong address;
string addrStr = resultLine.Substring(pos + 18);
if (MICommandFactory.SpanNextAddr(addrStr, out address) != null)
if (SpanNextAddr(addrStr, out address) != null)
{
addresses.Add(address);
}
Expand All @@ -183,6 +183,30 @@ public override async Task<List<ulong>> StartAddressesForLine(string file, uint
return addresses;
}

private async Task JumpInternal(string target)
{
// temporary breakpoint + jump
// NB: the gdb docs state: "Resume execution at line linespec. Execution stops again immediately if there is a breakpoint there."
// We rely on this. If another thread hits a breakpoint before that we have a UX problem
// and would need to handle this via scheduler-locking for all-stop mode and ??? for non-stop mode.
await _debugger.CmdAsync("-break-insert -t " + target, ResultClass.done);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by not registered?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

await _debugger.CmdAsync("-exec-jump " + target, ResultClass.running);
Trass3r marked this conversation as resolved.
Show resolved Hide resolved
}

public override Task ExecJump(string filename, int line)
{
string target = "--source " + filename + " --line " + line.ToString(CultureInfo.InvariantCulture);
return JumpInternal(target);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


string target = "*" + string.Format(CultureInfo.InvariantCulture, "0x{0:X}", address);
return JumpInternal(target);
}

public override Task EnableTargetAsyncOption()
{
// Linux attach TODO: GDB will fail this command when attaching. This is worked around
Expand Down
13 changes: 13 additions & 0 deletions src/MICore/CommandFactories/lldb.cs
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,19 @@ public override Task Catch(string name, bool onlyOnce = false, ResultClass resul
throw new NotImplementedException("lldb catch command");
}

Trass3r marked this conversation as resolved.
Show resolved Hide resolved
// TODO: update these if they become available in lldb-mi
public override async Task ExecJump(string filename, int line)
{
string command = "jump " + filename + ":" + line;
await _debugger.CmdAsync(command, ResultClass.running);
}

public override async Task ExecJump(ulong address)
{
string command = "jump *" + string.Format(CultureInfo.InvariantCulture, "0x{0:X}", address);
await _debugger.CmdAsync(command, ResultClass.running);
}

/// <summary>
/// Assigns the value of an expression to a variable.
/// Since LLDB only accepts assigning values to variables, the expression may need to be evaluated.
Expand Down
39 changes: 39 additions & 0 deletions src/MIDebugEngine/AD7.Impl/AD7Engine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,45 @@ public object GetMetric(string metric)
return _configStore.GetEngineMetric(metric);
}

public int Jump(string filename, int line)
{
try
{
_debuggedProcess.WorkerThread.RunOperation(() => _debuggedProcess.Jump(filename, line));
}
catch (InvalidCoreDumpOperationException)
{
return AD7_HRESULT.E_CRASHDUMP_UNSUPPORTED;
}
catch (Exception e)
{
_engineCallback.OnError(EngineUtils.GetExceptionDescription(e));
return Constants.E_ABORT;
}

return Constants.S_OK;
}

Trass3r marked this conversation as resolved.
Show resolved Hide resolved
public int Jump(ulong address)
{
try
{
_debuggedProcess.WorkerThread.RunOperation(() => _debuggedProcess.Jump(address));
}
catch (InvalidCoreDumpOperationException)
{
return AD7_HRESULT.E_CRASHDUMP_UNSUPPORTED;
}
catch (Exception e)
{
_engineCallback.OnError(EngineUtils.GetExceptionDescription(e));
return Constants.E_ABORT;
}
DebuggedProcess.ThreadCache.MarkDirty();
Copy link
Contributor Author

@Trass3r Trass3r Jan 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return Constants.S_OK;
}

#region IDebugEngine2 Members

// Attach the debug engine to a program.
Expand Down
4 changes: 4 additions & 0 deletions src/MIDebugEngine/AD7.Impl/AD7MemoryAddress.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,10 @@ public int GetInfo(enum_CONTEXT_INFO_FIELDS dwFields, CONTEXT_INFO[] pinfo)
pinfo[0].dwFields |= enum_CONTEXT_INFO_FIELDS.CIF_FUNCTION;
}
}
if ((dwFields & enum_CONTEXT_INFO_FIELDS.CIF_FUNCTIONOFFSET) != 0)
{
// TODO:
}

return Constants.S_OK;
}
Expand Down
24 changes: 10 additions & 14 deletions src/MIDebugEngine/AD7.Impl/AD7Thread.cs
Original file line number Diff line number Diff line change
Expand Up @@ -282,27 +282,23 @@ int IDebugThread2.Resume(out uint suspendCount)
}

// Sets the next statement to the given stack frame and code context.
// https://docs.microsoft.com/en-us/visualstudio/extensibility/debugger/reference/idebugthread2-setnextstatement
int IDebugThread2.SetNextStatement(IDebugStackFrame2 stackFrame, IDebugCodeContext2 codeContext)
{
ulong addr = ((AD7MemoryAddress)codeContext).Address;
AD7StackFrame frame = ((AD7StackFrame)stackFrame);
if (frame.ThreadContext.Level != 0 || frame.Thread != this || !frame.ThreadContext.pc.HasValue)
{
// VS does provide a frame so at least do some sanity checks
AD7StackFrame frame = stackFrame as AD7StackFrame;
if (frame != null && (frame.ThreadContext.Level != 0 || frame.Thread != this))
return Constants.S_FALSE;
Trass3r marked this conversation as resolved.
Show resolved Hide resolved
}
string toFunc = EngineUtils.GetAddressDescription(_engine.DebuggedProcess, addr);
string fromFunc = EngineUtils.GetAddressDescription(_engine.DebuggedProcess, frame.ThreadContext.pc.Value);
if (toFunc != fromFunc)

try
{
return Constants.S_FALSE;
ulong addr = ((AD7MemoryAddress)codeContext).Address;
return _engine.Jump(addr);
}
string result = frame.EvaluateExpression("$pc=" + EngineUtils.AsAddr(addr, _engine.DebuggedProcess.Is64BitArch));
if (result != null)
catch (Exception)
{
_engine.DebuggedProcess.ThreadCache.MarkDirty();
return Constants.S_OK;
return Constants.S_FALSE;
}
return Constants.S_FALSE;
}

// suspend a thread.
Expand Down
10 changes: 10 additions & 0 deletions src/MIDebugEngine/Engine.Impl/DebuggedProcess.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1631,6 +1631,16 @@ public Task Continue(DebuggedThread thread)
return Execute(thread);
}

public async Task Jump(string filename, int line)
{
await MICommandFactory.ExecJump(filename, line);
}
Trass3r marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public async Task Jump(ulong address)
{
Trass3r marked this conversation as resolved.
Show resolved Hide resolved
await MICommandFactory.ExecJump(address);
}

public async Task Step(int threadId, enum_STEPKIND kind, enum_STEPUNIT unit)
{
this.VerifyNotDebuggingCoreDump();
Expand Down
35 changes: 32 additions & 3 deletions src/OpenDebugAD7/AD7DebugSession.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1337,7 +1337,37 @@ protected override void HandlePauseRequestAsync(IRequestResponder<PauseArguments

protected override void HandleGotoRequestAsync(IRequestResponder<GotoArguments> responder)
{
responder.SetError(new ProtocolException(AD7Resources.Error_NotImplementedSetNextStatement));
var response = new GotoResponse();
Trass3r marked this conversation as resolved.
Show resolved Hide resolved
if (!m_isStopped)
{
responder.SetResponse(response);
return;
}

var builder = new ErrorBuilder(() => AD7Resources.Error_UnableToSetNextStatement);
IDebugThread2 thread = null;
try
{
if (m_gotoCodeContexts.TryGetValue(responder.Arguments.TargetId, out IDebugCodeContext2 gotoTarget))
{
lock (m_threads)
{
if (!m_threads.TryGetValue(responder.Arguments.ThreadId, out thread))
throw new AD7Exception("Unknown thread id: " + responder.Arguments.ThreadId.ToString(CultureInfo.InvariantCulture));
}
//BeforeContinue();
builder.CheckHR(thread.SetNextStatement(null, gotoTarget));
Copy link
Contributor Author

@Trass3r Trass3r Jan 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to remove?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
gregg-miskelly marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

We need to throw if this if is false.

}
catch (AD7Exception e)
{
m_isStopped = true;
responder.SetError(new ProtocolException(e.Message));
return;
}

responder.SetResponse(response);
FireStoppedEvent(thread, StoppedEvent.ReasonValue.Goto);
}

protected override void HandleGotoTargetsRequestAsync(IRequestResponder<GotoTargetsArguments, GotoTargetsResponse> responder)
Expand All @@ -1347,6 +1377,7 @@ protected override void HandleGotoTargetsRequestAsync(IRequestResponder<GotoTarg
var source = responder.Arguments.Source;

// Virtual documents don't have paths
// TODO: handle this for disassembly debugging
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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")

if (source.Path == null)
{
responder.SetResponse(response);
Expand All @@ -1369,10 +1400,8 @@ protected override void HandleGotoTargetsRequestAsync(IRequestResponder<GotoTarg
while (codeContextsEnum.Next(1, codeContexts, ref nProps) == HRConstants.S_OK)
{
var codeContext = codeContexts[0];

string contextName;
codeContext.GetName(out contextName);

line = responder.Arguments.Line;
IDebugDocumentContext2 documentContext;
if (codeContext.GetDocumentContext(out documentContext) == HRConstants.S_OK)
Expand Down
20 changes: 11 additions & 9 deletions src/OpenDebugAD7/AD7Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions src/OpenDebugAD7/AD7Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@
<data name="Error_UnableToSetBreakpoint" xml:space="preserve">
<value>Error setting breakpoint. {0}</value>
</data>
<data name="Error_UnableToSetNextStatement" xml:space="preserve">
<value>Error setting next statement. {0}</value>
</data>
<data name="Error_UnableToParseLogMessage" xml:space="preserve">
<value>Unable to parse 'logMessage'.</value>
</data>
Expand Down Expand Up @@ -298,7 +301,4 @@
<data name="Error_Scenario_StackTrace" xml:space="preserve">
<value>Unable to retrieve stack trace. {0}</value>
</data>
<data name="Error_NotImplementedSetNextStatement" xml:space="preserve">
<value>Set next statement is not supported by the current debugger.</value>
</data>
</root>