Skip to content

Commit daeea95

Browse files
asimgunesasimgunes
committed
Code improvement based on feedback
1 parent dc41f19 commit daeea95

File tree

4 files changed

+99
-78
lines changed

4 files changed

+99
-78
lines changed

‎src/gdb/GDBDebugSessionBase.ts

+3
Original file line numberDiff line numberDiff line change
@@ -1346,6 +1346,9 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
13461346
args: CDTDisassembleArguments
13471347
) {
13481348
try {
1349+
if (!args.memoryReference) {
1350+
throw new Error('Target memory reference is not specified!');
1351+
}
13491352
const instructionStartOffset = args.instructionOffset ?? 0;
13501353
const instructionEndOffset =
13511354
args.instructionCount + instructionStartOffset;

‎src/integration-tests/diassemble.spec.ts

+19-4
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import { DebugProtocol } from '@vscode/debugprotocol/lib/debugProtocol';
1414
import { CdtDebugClient } from './debugClient';
1515
import { fillDefaults, standardBeforeEach, testProgramsDir } from './utils';
1616
import { calculateMemoryOffset } from '../util/calculateMemoryOffset';
17+
import { assert } from 'sinon';
1718

1819
describe('Disassembly Test Suite', function () {
1920
let dc: CdtDebugClient;
@@ -99,8 +100,6 @@ describe('Disassembly Test Suite', function () {
99100
});
100101

101102
it('can disassemble with negative offsets', async function () {
102-
// In this case we attempt to read from where there is no source,
103-
// GDB returns data in a different format in that case
104103
const disassemble = (await dc.send('disassemble', {
105104
memoryReference: 'main',
106105
instructionOffset: -20,
@@ -110,6 +109,21 @@ describe('Disassembly Test Suite', function () {
110109
expectsGeneralDisassemble(disassemble, 20, true);
111110
});
112111

112+
it('send error response handle on empty memory reference', async function () {
113+
try {
114+
await dc.send('disassemble', {
115+
memoryReference: '',
116+
instructionOffset: -20,
117+
instructionCount: 20,
118+
} as DebugProtocol.DisassembleArguments);
119+
assert.fail('Should throw error!');
120+
} catch (e) {
121+
expect(e).to.be.deep.equal(
122+
new Error('Target memory reference is not specified!')
123+
);
124+
}
125+
});
126+
113127
it('can disassemble with correct boundries', async function () {
114128
const get = (
115129
disassemble: DebugProtocol.DisassembleResponse,
@@ -130,8 +144,6 @@ describe('Disassembly Test Suite', function () {
130144
expect(instruction1.address).to.eq(instruction2.address, message);
131145
};
132146

133-
// In this case we attempt to read from where there is no source,
134-
// GDB returns data in a different format in that case
135147
const disassembleLower = (await dc.send('disassemble', {
136148
memoryReference: 'main',
137149
instructionOffset: -20,
@@ -152,6 +164,9 @@ describe('Disassembly Test Suite', function () {
152164
expectsGeneralDisassemble(disassembleMiddle, 20, true);
153165
expectsGeneralDisassemble(disassembleHigher, 20, true);
154166

167+
// Current implementation have known edge cases, possibly instruction misaligning while
168+
// handling the negative offsets, please refer to the discussion at the following link:
169+
// https://github.com/eclipse-cdt-cloud/cdt-gdb-adapter/pull/341#discussion_r1857422980
155170
expectsInstructionEquals(
156171
get(disassembleLower, 15),
157172
get(disassembleMiddle, 5),

‎src/integration-tests/util.spec.ts

+6
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,12 @@ describe('calculateMemoryOffset', () => {
182182
expect(calculateMemoryOffset('0xffeeddcc0000ff00', '0x0100')).to.eq(
183183
'0xffeeddcc00010000'
184184
);
185+
expect(
186+
calculateMemoryOffset('0xefeeddcc0000ff00', '0x10000000000000ff')
187+
).to.eq('0xffeeddcc0000ffff');
188+
expect(
189+
calculateMemoryOffset('0xefeeddcc0000ff00', '0x1000000000000100')
190+
).to.eq('0xffeeddcc00010000');
185191
});
186192
});
187193

‎src/util/disassembly.ts

+71-74
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,7 @@
88
* SPDX-License-Identifier: EPL-2.0
99
*********************************************************************/
1010
import { DebugProtocol } from '@vscode/debugprotocol';
11-
import {
12-
MIDataDisassembleAsmInsn,
13-
MIDataDisassembleResponse,
14-
sendDataDisassemble,
15-
} from '../mi';
11+
import { MIDataDisassembleAsmInsn, sendDataDisassemble } from '../mi';
1612
import { IGDBBackend } from '../types/gdb';
1713
import { calculateMemoryOffset } from './calculateMemoryOffset';
1814

@@ -115,85 +111,86 @@ export const getInstructions = async (
115111
) => {
116112
const list: DebugProtocol.DisassembledInstruction[] = [];
117113
const meanSizeOfInstruction = 4;
114+
const isReverseFetch = length < 0;
118115
const absLength = Math.abs(length);
119116

120-
let result: MIDataDisassembleResponse | undefined = undefined;
121-
try {
122-
result =
123-
length < 0
124-
? await sendDataDisassemble(
125-
gdb,
126-
`(${memoryReference})-${
127-
absLength * meanSizeOfInstruction
128-
}`,
129-
`(${memoryReference})+0`
130-
)
131-
: await sendDataDisassemble(
132-
gdb,
133-
`(${memoryReference})+0`,
134-
`(${memoryReference})+${
135-
absLength * meanSizeOfInstruction
136-
}`
137-
);
138-
} catch (e) {
139-
// Do nothing in case of memory error.
140-
result = undefined;
141-
}
117+
const formatMemoryAddress = (offset: number) => {
118+
return `(${memoryReference})${offset < 0 ? '-' : '+'}${Math.abs(
119+
offset
120+
)}`;
121+
};
142122

143-
if (result === undefined) {
144-
// result is undefined in case of error,
145-
// then return empty instructions list
146-
return getEmptyInstructions(memoryReference, length, 2);
147-
}
123+
const sendDataDisassembleWrapper = async (lower: number, upper: number) => {
124+
const list: DebugProtocol.DisassembledInstruction[] = [];
148125

149-
for (const asmInsn of result.asm_insns) {
150-
const line: number | undefined = asmInsn.line
151-
? parseInt(asmInsn.line, 10)
152-
: undefined;
153-
const location = {
154-
name: asmInsn.file,
155-
path: asmInsn.fullname,
156-
} as DebugProtocol.Source;
157-
for (const asmLine of asmInsn.line_asm_insn) {
158-
list.push({
159-
...getDisassembledInstruction(asmLine),
160-
location,
161-
line,
162-
});
126+
const result = await sendDataDisassemble(
127+
gdb,
128+
formatMemoryAddress(lower),
129+
formatMemoryAddress(upper)
130+
);
131+
for (const asmInsn of result.asm_insns) {
132+
const line: number | undefined = asmInsn.line
133+
? parseInt(asmInsn.line, 10)
134+
: undefined;
135+
const location = {
136+
name: asmInsn.file,
137+
path: asmInsn.fullname,
138+
} as DebugProtocol.Source;
139+
for (const asmLine of asmInsn.line_asm_insn) {
140+
list.push({
141+
...getDisassembledInstruction(asmLine),
142+
location,
143+
line,
144+
});
145+
}
163146
}
164-
}
147+
return list;
148+
};
165149

166-
if (length < 0) {
167-
// Remove the heading, if necessary
168-
if (absLength < list.length) {
169-
list.splice(0, list.length - absLength);
150+
const target = { lower: 0, higher: 0 };
151+
const recalculateTargetBounds = (length: number) => {
152+
if (isReverseFetch) {
153+
target.higher = target.lower;
154+
target.lower += length * meanSizeOfInstruction;
155+
} else {
156+
target.lower = target.higher;
157+
target.higher += length * meanSizeOfInstruction;
170158
}
171-
172-
// Add empty instructions, if necessary
173-
if (list.length < absLength) {
174-
const startAddress = list[0].address;
175-
const filling = getEmptyInstructions(
176-
startAddress,
177-
absLength - list.length,
178-
-2
159+
};
160+
const remainingLength = () =>
161+
Math.sign(length) * Math.max(absLength - list.length, 0);
162+
try {
163+
// Do we need an explicit guard to prevent infinite loop
164+
while (absLength > list.length) {
165+
recalculateTargetBounds(remainingLength());
166+
const result = await sendDataDisassembleWrapper(
167+
target.lower,
168+
target.higher
179169
);
180-
list.unshift(...filling);
181-
}
182-
} else {
183-
// Remove the tail, if necessary
184-
if (absLength < list.length) {
185-
list.splice(absLength, list.length - absLength);
170+
if (isReverseFetch) {
171+
list.unshift(...result);
172+
} else {
173+
list.push(...result);
174+
}
186175
}
176+
} catch (e) {
177+
// Fill with empty instructions in case of memory error.
178+
const lastMemoryAddress =
179+
list.length === 0
180+
? memoryReference
181+
: list[isReverseFetch ? 0 : list.length - 1].address;
182+
list.push(
183+
...getEmptyInstructions(lastMemoryAddress, remainingLength(), 2)
184+
);
185+
}
187186

188-
// Add empty instructions, if necessary
189-
if (list.length < absLength) {
190-
const startAddress = list[list.length - 1].address;
191-
const filling = getEmptyInstructions(
192-
startAddress,
193-
absLength - list.length,
194-
2
195-
);
196-
list.push(...filling);
187+
if (absLength < list.length) {
188+
if (length < 0) {
189+
// Remove the heading, if necessary
190+
list.splice(0, list.length - absLength);
191+
} else {
192+
// Remove the tail, if necessary
193+
list.splice(absLength, list.length - absLength);
197194
}
198195
}
199196

0 commit comments

Comments
 (0)