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.

Sign up for GitHub

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

Improving the DisassembleRequest logic #341

Merged
merged 4 commits into from
Dec 10, 2024
Merged

Improving the DisassembleRequest logic #341

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cf5b32
Improving the DisassembleRequest logic
asimgunes Nov 19, 2024
8cd7292
Code improvement based on feedback
asimgunes Dec 5, 2024
83f2822
Negative offset read optimisation
asimgunes Dec 9, 2024
2d6fad1
Merge branch 'main' into improvement/updating-disassemble-request-imp…
jonahgraham Dec 10, 2024
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
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@
},
"homepage": "https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter#readme",
"dependencies": {
"@vscode/debugadapter": "^1.59.0",
"@vscode/debugprotocol": "^1.59.0",
"@vscode/debugadapter": "^1.68.0",
"@vscode/debugprotocol": "^1.68.0",
"node-addon-api": "^4.3.0",
"serialport": "11.0.0",
"utf8": "^3.0.0"
Expand Down
131 changes: 35 additions & 96 deletions src/gdb/GDBDebugSessionBase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ import {
CDTDisassembleArguments,
} from '../types/session';
import { IGDBBackend, IGDBBackendFactory } from '../types/gdb';
import { getInstructions } from '../util/disassembly';
import { calculateMemoryOffset } from '../util/calculateMemoryOffset';

class ThreadWithStatus implements DebugProtocol.Thread {
id: number;
Expand Down Expand Up @@ -1344,108 +1346,45 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
args: CDTDisassembleArguments
) {
try {
const meanSizeOfInstruction = 4;
let startOffset = 0;
let lastStartOffset = -1;
if (!args.memoryReference) {
throw new Error('Target memory reference is not specified!');
}
const instructionStartOffset = args.instructionOffset ?? 0;
const instructionEndOffset =
args.instructionCount + instructionStartOffset;
const instructions: DebugProtocol.DisassembledInstruction[] = [];
let oneIterationOnly = false;
outer_loop: while (
instructions.length < args.instructionCount &&
!oneIterationOnly
) {
if (startOffset === lastStartOffset) {
// We have stopped getting new instructions, give up
break outer_loop;
}
lastStartOffset = startOffset;

const fetchSize =
(args.instructionCount - instructions.length) *
meanSizeOfInstruction;

// args.memoryReference is an arbitrary expression, so let GDB do the
// math on resolving value rather than doing the addition in the adapter
try {
const stepStartAddress = `(${args.memoryReference})+${startOffset}`;
let stepEndAddress = `(${args.memoryReference})+${startOffset}+${fetchSize}`;
if (args.endMemoryReference && instructions.length === 0) {
// On the first call, if we have an end memory address use it instead of
// the approx size
stepEndAddress = args.endMemoryReference;
oneIterationOnly = true;
}
const result = await mi.sendDataDisassemble(
this.gdb,
stepStartAddress,
stepEndAddress
);
for (const asmInsn of result.asm_insns) {
const line: number | undefined = asmInsn.line
? parseInt(asmInsn.line, 10)
: undefined;
const source = {
name: asmInsn.file,
path: asmInsn.fullname,
} as DebugProtocol.Source;
for (const asmLine of asmInsn.line_asm_insn) {
let funcAndOffset: string | undefined;
if (asmLine['func-name'] && asmLine.offset) {
funcAndOffset = `${asmLine['func-name']}+${asmLine.offset}`;
} else if (asmLine['func-name']) {
funcAndOffset = asmLine['func-name'];
} else {
funcAndOffset = undefined;
}
const disInsn = {
address: asmLine.address,
instructionBytes: asmLine.opcodes,
instruction: asmLine.inst,
symbol: funcAndOffset,
location: source,
line,
} as DebugProtocol.DisassembledInstruction;
instructions.push(disInsn);
if (instructions.length === args.instructionCount) {
break outer_loop;
}
const memoryReference =
args.offset === undefined
? args.memoryReference
: calculateMemoryOffset(args.memoryReference, args.offset);

if (instructionStartOffset < 0) {
// Getting lower memory area
const list = await getInstructions(
this.gdb,
memoryReference,
instructionStartOffset
);

const bytes = asmLine.opcodes.replace(/\s/g, '');
startOffset += bytes.length;
}
}
} catch (err) {
// Failed to read instruction -- what best to do here?
// in other words, whose responsibility (adapter or client)
// to reissue reads in smaller chunks to find good memory
while (instructions.length < args.instructionCount) {
const badDisInsn = {
// TODO this should start at byte after last retrieved address
address: `0x${startOffset.toString(16)}`,
instruction:
err instanceof Error
? err.message
: String(err),
} as DebugProtocol.DisassembledInstruction;
instructions.push(badDisInsn);
startOffset += 2;
}
break outer_loop;
}
// Add them to instruction list
instructions.push(...list);
}

if (!args.endMemoryReference) {
while (instructions.length < args.instructionCount) {
const badDisInsn = {
// TODO this should start at byte after last retrieved address
address: `0x${startOffset.toString(16)}`,
instruction: 'failed to retrieve instruction',
} as DebugProtocol.DisassembledInstruction;
instructions.push(badDisInsn);
startOffset += 2;
}
if (instructionEndOffset > 0) {
// Getting higher memory area
const list = await getInstructions(
this.gdb,
memoryReference,
instructionEndOffset
);

// Add them to instruction list
instructions.push(...list);
}

response.body = { instructions };
response.body = {
instructions,
};
this.sendResponse(response);
} catch (err) {
this.sendErrorResponse(
Expand Down
152 changes: 120 additions & 32 deletions src/integration-tests/diassemble.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,37 @@ import * as path from 'path';
import { DebugProtocol } from '@vscode/debugprotocol/lib/debugProtocol';
import { CdtDebugClient } from './debugClient';
import { fillDefaults, standardBeforeEach, testProgramsDir } from './utils';
import { calculateMemoryOffset } from '../util/calculateMemoryOffset';
import { assert } from 'sinon';

describe('Disassembly Test Suite', function () {
let dc: CdtDebugClient;
let frame: DebugProtocol.StackFrame;
const disProgram = path.join(testProgramsDir, 'disassemble');
const disSrc = path.join(testProgramsDir, 'disassemble.c');

const expectsGeneralDisassemble = (
disassemble: DebugProtocol.DisassembleResponse,
length: number,
ignoreEmptyInstructions?: boolean
) => {
expect(disassemble).not.eq(undefined);
expect(disassemble.body).not.eq(undefined);
if (disassemble.body) {
const instructions = disassemble.body.instructions;
expect(instructions).to.have.lengthOf(length);
// the contents of the instructions are platform dependent, so instead
// make sure we have read fully
for (const i of instructions) {
expect(i.address).to.have.lengthOf.greaterThan(0);
expect(i.instruction).to.have.lengthOf.greaterThan(0);
if (!ignoreEmptyInstructions) {
expect(i.instructionBytes).to.have.lengthOf.greaterThan(0);
}
}
}
};

beforeEach(async function () {
dc = await standardBeforeEach();

Expand Down Expand Up @@ -60,19 +84,8 @@ describe('Disassembly Test Suite', function () {
memoryReference: 'main',
instructionCount: 100,
})) as DebugProtocol.DisassembleResponse;
expect(disassemble).not.eq(undefined);
expect(disassemble.body).not.eq(undefined);
if (disassemble.body) {
const instructions = disassemble.body.instructions;
expect(instructions).to.have.lengthOf(100);
// the contents of the instructions are platform dependent, so instead
// make sure we have read fully
for (const i of instructions) {
expect(i.address).to.have.lengthOf.greaterThan(0);
expect(i.instructionBytes).to.have.lengthOf.greaterThan(0);
expect(i.instruction).to.have.lengthOf.greaterThan(0);
}
}

expectsGeneralDisassemble(disassemble, 100);
});

it('can disassemble with no source references', async function () {
Expand All @@ -82,38 +95,113 @@ describe('Disassembly Test Suite', function () {
memoryReference: 'main+1000',
instructionCount: 100,
})) as DebugProtocol.DisassembleResponse;
expect(disassemble).not.eq(undefined);
expect(disassemble.body).not.eq(undefined);
if (disassemble.body) {
const instructions = disassemble.body.instructions;
expect(instructions).to.have.lengthOf(100);
// the contents of the instructions are platform dependent, so instead
// make sure we have read fully
for (const i of instructions) {
expect(i.address).to.have.lengthOf.greaterThan(0);
expect(i.instructionBytes).to.have.lengthOf.greaterThan(0);
expect(i.instruction).to.have.lengthOf.greaterThan(0);
}

expectsGeneralDisassemble(disassemble, 100);
});

it('can disassemble with negative offsets', async function () {
const disassemble = (await dc.send('disassemble', {
memoryReference: 'main',
instructionOffset: -20,
instructionCount: 20,
} as DebugProtocol.DisassembleArguments)) as DebugProtocol.DisassembleResponse;

expectsGeneralDisassemble(disassemble, 20, true);
});

it('send error response handle on empty memory reference', async function () {
try {
await dc.send('disassemble', {
memoryReference: '',
instructionOffset: -20,
instructionCount: 20,
} as DebugProtocol.DisassembleArguments);
assert.fail('Should throw error!');
} catch (e) {
expect(e).to.be.deep.equal(
new Error('Target memory reference is not specified!')
);
}
});

it('can disassemble with correct boundries', async function () {
const get = (
disassemble: DebugProtocol.DisassembleResponse,
offset: number
) => {
const instruction = disassemble.body?.instructions[offset];
expect(instruction).not.eq(undefined);
// Instruction undefined already checked.
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return instruction!;
};

const expectsInstructionEquals = (
instruction1: DebugProtocol.DisassembledInstruction,
instruction2: DebugProtocol.DisassembledInstruction,
message?: string
) => {
expect(instruction1.address).to.eq(instruction2.address, message);
};

const disassembleLower = (await dc.send('disassemble', {
memoryReference: 'main',
instructionOffset: -20,
instructionCount: 20,
} as DebugProtocol.DisassembleArguments)) as DebugProtocol.DisassembleResponse;
const disassembleMiddle = (await dc.send('disassemble', {
memoryReference: 'main',
instructionOffset: -10,
instructionCount: 20,
} as DebugProtocol.DisassembleArguments)) as DebugProtocol.DisassembleResponse;
const disassembleHigher = (await dc.send('disassemble', {
memoryReference: 'main',
instructionOffset: 0,
instructionCount: 20,
} as DebugProtocol.DisassembleArguments)) as DebugProtocol.DisassembleResponse;

expectsGeneralDisassemble(disassembleLower, 20, true);
expectsGeneralDisassemble(disassembleMiddle, 20, true);
expectsGeneralDisassemble(disassembleHigher, 20, true);

// Current implementation have known edge cases, possibly instruction misaligning while
// handling the negative offsets, please refer to the discussion at the following link:
// https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/341#discussion_r1857422980
expectsInstructionEquals(
get(disassembleLower, 15),
get(disassembleMiddle, 5),
'lower[15] should be same with middle[5]'
);

expectsInstructionEquals(
get(disassembleMiddle, 15),
get(disassembleHigher, 5),
'middle[15] should be same with higher[5]'
);
});

it('can handle disassemble at bad address', async function () {
const disassemble = (await dc.send('disassemble', {
memoryReference: '0x0',
instructionCount: 10,
})) as DebugProtocol.DisassembleResponse;

expect(disassemble).not.eq(undefined);
expect(disassemble.body).not.eq(undefined);
if (disassemble.body) {
const instructions = disassemble.body.instructions;
expect(instructions).to.have.lengthOf(10);
// the contens of the instructions are platform dependent, so instead
// make sure we have read fully
for (const i of instructions) {
expect(i.address).to.have.lengthOf.greaterThan(0);
expect(i.instruction).to.have.lengthOf.greaterThan(0);
expect(i.instructionBytes).eq(undefined);
}
// Checking the invalid instructions content
instructions.forEach((inst, ix) => {
expect(inst.address).to.eq(
calculateMemoryOffset('0x0', ix * 2)
);
expect(inst.address).to.have.lengthOf.greaterThan(0);
expect(inst.instruction).to.eq(
'failed to retrieve instruction'
);
expect(inst.presentationHint).to.eq('invalid');
});
}
});
});
Loading
Loading