Improving the DisassembleRequest logic (#341)

This is an update for the disassembleRequest logic in the debug
adapter. This implementation is covering the scenarios discussed
at the #340 .

Notes:

- The previous implementation was covering a scenario for the
  endMemoryReference argument. I couldn't find the argument in
  the DAP specifications. Since this argument doesn't exists on
  the current DAP protocol and only existed for the now deleted
  Eclipse CDT front end, this field support has been removed.
- The main thing this code changes is handling negative offsets
  from the memory reference, something the original code did not
  do and wasn't used by vscode when the original code was written.

Author: asimgunes

package.json

@@ -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"

src/gdb/GDBDebugSessionBase.ts

@@ -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;
@@ -1344,115 +1346,50 @@ 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(
-                response,
-                1,
-                err instanceof Error ? err.message : String(err)
-            );
+            const message = err instanceof Error ? err.message : String(err);
+            this.sendEvent(new OutputEvent(`Error: ${message}`));
+            this.sendErrorResponse(response, 1, message);
         }
     }
 

src/integration-tests/diassemble.spec.ts

@@ -13,13 +13,36 @@ import * as path from 'path';
 import { DebugProtocol } from '@vscode/debugprotocol/lib/debugProtocol';
 import { CdtDebugClient } from './debugClient';
 import { fillDefaults, standardBeforeEach, testProgramsDir } from './utils';
+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();
 
@@ -60,19 +83,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 () {
@@ -82,38 +94,102 @@ 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 handle disassemble at bad address', async function () {
+    it('can disassemble with negative offsets', 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);
-            }
+            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('return error at bad address', async function () {
+        try {
+            await dc.send('disassemble', {
+                memoryReference: '0x0',
+                instructionCount: 10,
+            } as DebugProtocol.DisassembleArguments);
+            assert.fail('Should throw error!');
+        } catch (e) {
+            expect(e).to.be.deep.equal(
+                new Error('Cannot access memory at address 0x0')
+            );
         }
     });
 });