Code improvement based on feedback

Author: asimgunes

src/integration-tests/diassemble.spec.ts

@@ -99,8 +99,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,
@@ -130,8 +128,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 +148,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,86 @@ 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 {
+        // Do we need an explicit guard to prevent infinite loop
+        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 (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);
         }
     }