Code improvement based on feedback

src/gdb/GDBDebugSessionBase.ts

@@ -1346,6 +1346,9 @@ export abstract class GDBDebugSessionBase extends LoggingDebugSession {
         args: CDTDisassembleArguments
     ) {
         try {
+            if (!args.memoryReference) {
+                throw new Error('Target memory reference is not specified!');
+            }
             const instructionStartOffset = args.instructionOffset ?? 0;
             const instructionEndOffset =
                 args.instructionCount + instructionStartOffset;

src/integration-tests/diassemble.spec.ts

@@ -14,6 +14,7 @@ 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;
@@ -99,8 +100,6 @@ describe('Disassembly Test Suite', function () {
     });
 
     it('can disassemble with negative offsets', async function () {
-        // In this case we attempt to read from where there is no source,
-        // GDB returns data in a different format in that case
         const disassemble = (await dc.send('disassemble', {
             memoryReference: 'main',
             instructionOffset: -20,
@@ -110,6 +109,21 @@ describe('Disassembly Test Suite', function () {
         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,
@@ -130,8 +144,6 @@ describe('Disassembly Test Suite', function () {
             expect(instruction1.address).to.eq(instruction2.address, message);
         };
 
-        // In this case we attempt to read from where there is no source,
-        // GDB returns data in a different format in that case
         const disassembleLower = (await dc.send('disassemble', {
             memoryReference: 'main',
             instructionOffset: -20,
@@ -152,6 +164,9 @@ describe('Disassembly Test Suite', function () {
         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),

src/integration-tests/util.spec.ts

@@ -182,6 +182,12 @@ describe('calculateMemoryOffset', () => {
         expect(calculateMemoryOffset('0xffeeddcc0000ff00', '0x0100')).to.eq(
             '0xffeeddcc00010000'
         );
+        expect(
+            calculateMemoryOffset('0xefeeddcc0000ff00', '0x10000000000000ff')
+        ).to.eq('0xffeeddcc0000ffff');
+        expect(
+            calculateMemoryOffset('0xefeeddcc0000ff00', '0x1000000000000100')
+        ).to.eq('0xffeeddcc00010000');
     });
 });
 

src/util/disassembly.ts

@@ -8,11 +8,7 @@
  * SPDX-License-Identifier: EPL-2.0
  *********************************************************************/
 import { DebugProtocol } from '@vscode/debugprotocol';
-import {
-    MIDataDisassembleAsmInsn,
-    MIDataDisassembleResponse,
-    sendDataDisassemble,
-} from '../mi';
+import { MIDataDisassembleAsmInsn, sendDataDisassemble } from '../mi';
 import { IGDBBackend } from '../types/gdb';
 import { calculateMemoryOffset } from './calculateMemoryOffset';
 
@@ -115,85 +111,90 @@ export const getInstructions = async (
 ) => {
     const list: DebugProtocol.DisassembledInstruction[] = [];
     const meanSizeOfInstruction = 4;
+    const isReverseFetch = length < 0;
     const absLength = Math.abs(length);
 
-    let result: MIDataDisassembleResponse | undefined = undefined;
-    try {
-        result =
-            length < 0
-                ? await sendDataDisassemble(
-                      gdb,
-                      `(${memoryReference})-${
-                          absLength * meanSizeOfInstruction
-                      }`,
-                      `(${memoryReference})+0`
-                  )
-                : await sendDataDisassemble(
-                      gdb,
-                      `(${memoryReference})+0`,
-                      `(${memoryReference})+${
-                          absLength * meanSizeOfInstruction
-                      }`
-                  );
-    } catch (e) {
-        // Do nothing in case of memory error.
-        result = undefined;
-    }
+    const formatMemoryAddress = (offset: number) => {
+        return `(${memoryReference})${offset < 0 ? '-' : '+'}${Math.abs(
+            offset
+        )}`;
+    };
 
-    if (result === undefined) {
-        // result is undefined in case of error,
-        // then return empty instructions list
-        return getEmptyInstructions(memoryReference, length, 2);
-    }
+    const sendDataDisassembleWrapper = async (lower: number, upper: number) => {
+        const list: DebugProtocol.DisassembledInstruction[] = [];
 
-    for (const asmInsn of result.asm_insns) {
-        const line: number | undefined = asmInsn.line
-            ? parseInt(asmInsn.line, 10)
-            : undefined;
-        const location = {
-            name: asmInsn.file,
-            path: asmInsn.fullname,
-        } as DebugProtocol.Source;
-        for (const asmLine of asmInsn.line_asm_insn) {
-            list.push({
-                ...getDisassembledInstruction(asmLine),
-                location,
-                line,
-            });
+        const result = await sendDataDisassemble(
+            gdb,
+            formatMemoryAddress(lower),
+            formatMemoryAddress(upper)
+        );
+        for (const asmInsn of result.asm_insns) {
+            const line: number | undefined = asmInsn.line
+                ? parseInt(asmInsn.line, 10)
+                : undefined;
+            const location = {
+                name: asmInsn.file,
+                path: asmInsn.fullname,
+            } as DebugProtocol.Source;
+            for (const asmLine of asmInsn.line_asm_insn) {
+                list.push({
+                    ...getDisassembledInstruction(asmLine),
+                    location,
+                    line,
+                });
+            }
         }
-    }
+        return list;
+    };
 
-    if (length < 0) {
-        // Remove the heading, if necessary
-        if (absLength < list.length) {
-            list.splice(0, list.length - absLength);
+    const target = { lower: 0, higher: 0 };
+    const recalculateTargetBounds = (length: number) => {
+        if (isReverseFetch) {
+            target.higher = target.lower;
+            target.lower += length * meanSizeOfInstruction;
+        } else {
+            target.lower = target.higher;
+            target.higher += length * meanSizeOfInstruction;
         }
-
-        // Add empty instructions, if necessary
-        if (list.length < absLength) {
-            const startAddress = list[0].address;
-            const filling = getEmptyInstructions(
-                startAddress,
-                absLength - list.length,
-                -2
+    };
+    const remainingLength = () =>
+        Math.sign(length) * Math.max(absLength - list.length, 0);
+    try {
+        while (absLength > list.length) {
+            recalculateTargetBounds(remainingLength());
+            const result = await sendDataDisassembleWrapper(
+                target.lower,
+                target.higher
             );
-            list.unshift(...filling);
-        }
-    } else {
-        // Remove the tail, if necessary
-        if (absLength < list.length) {
-            list.splice(absLength, list.length - absLength);
+            if (result.length === 0) {
+                // If cannot retrieve more instructions, break the loop, go to catch
+                // and fill the remaining instructions with empty instruction information
+                throw new Error('Cannot retrieve more instructions!');
+            }
+            if (isReverseFetch) {
+                list.unshift(...result);
+            } else {
+                list.push(...result);
+            }
         }
+    } catch (e) {
+        // Fill with empty instructions in case of memory error.
+        const lastMemoryAddress =
+            list.length === 0
+                ? memoryReference
+                : list[isReverseFetch ? 0 : list.length - 1].address;
+        list.push(
+            ...getEmptyInstructions(lastMemoryAddress, remainingLength(), 2)
+        );
+    }
 
-        // Add empty instructions, if necessary
-        if (list.length < absLength) {
-            const startAddress = list[list.length - 1].address;
-            const filling = getEmptyInstructions(
-                startAddress,
-                absLength - list.length,
-                2
-            );
-            list.push(...filling);
+    if (absLength < list.length) {
+        if (length < 0) {
+            // Remove the heading, if necessary
+            list.splice(0, list.length - absLength);
+        } else {
+            // Remove the tail, if necessary
+            list.splice(absLength, list.length - absLength);
         }
     }