Skip to content
Prev Previous commit
Next Next commit
Remove Jump for only GoToTarget and add memory ref
  • Loading branch information
WardenGnaw committed Jan 29, 2021
commit f37fe48f6417c1525b29e30b8c4f238616bee8e6
6 changes: 0 additions & 6 deletions src/MICore/CommandFactories/MICommandFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -242,12 +242,6 @@ 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);
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
26 changes: 2 additions & 24 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 -s " + file + " -li " + line;
string cmd = "info line " + file + ":" + 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 (SpanNextAddr(addrStr, out address) != null)
if (MICommandFactory.SpanNextAddr(addrStr, out address) != null)
{
addresses.Add(address);
}
Expand All @@ -183,28 +183,6 @@ 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);
await _debugger.CmdAsync("-exec-jump " + target, ResultClass.running);
}

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)
{
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: 0 additions & 13 deletions src/MICore/CommandFactories/lldb.cs
Original file line number Diff line number Diff line change
Expand Up @@ -177,19 +177,6 @@ public override Task Catch(string name, bool onlyOnce = false, ResultClass resul
throw new NotImplementedException("lldb catch command");
}

// 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
40 changes: 1 addition & 39 deletions src/MIDebugEngine/AD7.Impl/AD7Engine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -182,44 +182,6 @@ 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;
}

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;
}

return Constants.S_OK;
}

#region IDebugEngine2 Members

// Attach the debug engine to a program.
Expand Down Expand Up @@ -877,7 +839,7 @@ public int EnumCodeContexts(IDebugDocumentPosition2 docPosition, out IEnumDebugC
{
var codeCxt = new AD7MemoryAddress(this, a, null);
TEXT_POSITION pos;
pos.dwLine = line;
pos.dwLine = line - 1;
pos.dwColumn = 0;
MITextPosition textPosition = new MITextPosition(documentName, pos, pos);
codeCxt.SetDocumentContext(new AD7DocumentContext(textPosition, codeCxt));
Expand Down
4 changes: 0 additions & 4 deletions src/MIDebugEngine/AD7.Impl/AD7MemoryAddress.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,6 @@ 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: 14 additions & 10 deletions src/MIDebugEngine/AD7.Impl/AD7Thread.cs
Original file line number Diff line number Diff line change
Expand Up @@ -282,23 +282,27 @@ 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)
{
// 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;

try
ulong addr = ((AD7MemoryAddress)codeContext).Address;
AD7StackFrame frame = ((AD7StackFrame)stackFrame);
if (frame.ThreadContext.Level != 0 || frame.Thread != this || !frame.ThreadContext.pc.HasValue)
{
ulong addr = ((AD7MemoryAddress)codeContext).Address;
return _engine.Jump(addr);
return Constants.S_FALSE;
}
catch (Exception)
string toFunc = EngineUtils.GetAddressDescription(_engine.DebuggedProcess, addr);
string fromFunc = EngineUtils.GetAddressDescription(_engine.DebuggedProcess, frame.ThreadContext.pc.Value);
if (toFunc != fromFunc)
{
return Constants.S_FALSE;
}
string result = frame.EvaluateExpression("$pc=" + EngineUtils.AsAddr(addr, _engine.DebuggedProcess.Is64BitArch));
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is now only used by VS, right?
It can handle this expression?

Copy link
Member

Choose a reason for hiding this comment

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

This function is now only used by VS, right?

Correct. Currently this code isn't reachable in DAP scenarios.

It can handle this expression?

This is just an internal call within MIEngine, so it will not matter what IDE is used. It worked with whatever scenario Set Next Statement was originally tested (Android?). I don't know if it has been tested more broadly. Certainly using the actual GDB command is better. But this PR just seeks to finish off disassembly support. Actually implementing set next statement is still left for your PR. Someone needs to finish off handling the work of handling that stopping event...

Copy link
Contributor

@Trass3r Trass3r Jan 29, 2021

Choose a reason for hiding this comment

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

Ah yeah my bad, of course this goes down to gdb and not up.
Setting $pc is actually different from the jump command in that it does not start your program running; it only changes the address of where it will run when you continue: https://sourceware.org/gdb/current/onlinedocs/gdb/Jumping.html
So I'm not sure how this worked out regarding UI/UX.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, we would need to hide this difference -- the new set next statement would need to save some state in the process object to indicate that it is waiting for that stop event, and then SetNextStatement will need to wait for the stop event to be raised (or for the target process to exit).

if (result != null)
{
_engine.DebuggedProcess.ThreadCache.MarkDirty();
return Constants.S_OK;
}
return Constants.S_FALSE;
}

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

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

public async Task Jump(ulong address)
{
await MICommandFactory.ExecJump(address);
}

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

protected override void HandleGotoRequestAsync(IRequestResponder<GotoArguments> responder)
{
var response = new GotoResponse();
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));
}
}
catch (AD7Exception e)
{
m_isStopped = true;
responder.SetError(new ProtocolException(e.Message));
return;
}

responder.SetResponse(response);
FireStoppedEvent(thread, StoppedEvent.ReasonValue.Goto);
responder.SetError(new ProtocolException("Not implemented exception."));
}

protected override void HandleGotoTargetsRequestAsync(IRequestResponder<GotoTargetsArguments, GotoTargetsResponse> responder)
Expand Down Expand Up @@ -1387,8 +1357,10 @@ 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 All @@ -1399,10 +1371,23 @@ protected override void HandleGotoTargetsRequestAsync(IRequestResponder<GotoTarg
line = m_pathConverter.ConvertDebuggerLineToClient((int)startPos[0].dwLine);
}

string instructionPointerReference = null;
CONTEXT_INFO[] contextInfo = new CONTEXT_INFO[1];
if (codeContext.GetInfo(enum_CONTEXT_INFO_FIELDS.CIF_ADDRESS, contextInfo) == HRConstants.S_OK)
{
instructionPointerReference = contextInfo[0].bstrAddress;
}

int codeContextId = m_nextContextId++;
m_gotoCodeContexts.TryAdd(codeContextId, codeContext);

targets.Add(new GotoTarget(codeContextId, contextName, line));
targets.Add(new GotoTarget()
{
Id = codeContextId,
Label = contextName,
Line = line,
InstructionPointerReference = instructionPointerReference
});
}
}

Expand Down
11 changes: 0 additions & 11 deletions src/OpenDebugAD7/AD7Resources.Designer.cs

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

3 changes: 0 additions & 3 deletions src/OpenDebugAD7/AD7Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,6 @@
<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