From 25093fe5d1e7e3bddbee037bab7eb178923e8011 Mon Sep 17 00:00:00 2001 From: Derek Parker Date: Wed, 17 Jun 2026 17:03:26 -0700 Subject: [PATCH 1/3] Add -dap-log flag for DAP protocol/tool logging in tests Add TestMain with a -dap-log flag that enables DAP protocol and tool-level logging during test runs. When set to a directory path, each test gets its own .protocol.log and .tool.log files. Helper functions dapLogArgs() and dumpDAPLogs() wire the logging into test debug sessions and dump logs on test failure. --- tools_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tools_test.go b/tools_test.go index e92b830..c730cc4 100644 --- a/tools_test.go +++ b/tools_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "flag" "fmt" "io" "net/http" @@ -18,6 +19,46 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" ) +var dapLogDir string + +func TestMain(m *testing.M) { + flag.StringVar(&dapLogDir, "dap-log", "", "directory for DAP protocol and tool logs (enables verbose DAP logging)") + flag.Parse() + if dapLogDir != "" { + if err := os.MkdirAll(dapLogDir, 0o755); err != nil { + fmt.Fprintf(os.Stderr, "failed to create dap-log dir: %v\n", err) + os.Exit(1) + } + } + os.Exit(m.Run()) +} + +func dapLogArgs(t *testing.T) map[string]any { + if dapLogDir == "" { + return nil + } + name := strings.ReplaceAll(t.Name(), "/", "_") + return map[string]any{ + "protocolLog": filepath.Join(dapLogDir, name+".protocol.log"), + "toolLog": filepath.Join(dapLogDir, name+".tool.log"), + } +} + +func dumpDAPLogs(t *testing.T) { + if dapLogDir == "" { + return + } + name := strings.ReplaceAll(t.Name(), "/", "_") + for _, suffix := range []string{".protocol.log", ".tool.log"} { + path := filepath.Join(dapLogDir, name+suffix) + data, err := os.ReadFile(path) + if err != nil || len(data) == 0 { + continue + } + t.Logf("\n=== %s ===\n%s", filepath.Base(path), string(data)) + } +} + // testSetup holds the common test infrastructure type testSetup struct { cwd string @@ -129,6 +170,9 @@ func (ts *testSetup) startDebugSession(t *testing.T, port string, binaryPath str if len(programArgs) > 0 { args["args"] = programArgs } + for k, v := range dapLogArgs(t) { + args[k] = v + } result, err := ts.session.CallTool(ts.ctx, &mcp.CallToolParams{ Name: "debug", From 875e4c1db92622971c055ae284d9fb9bcdd1cc70 Mon Sep 17 00:00:00 2001 From: Derek Parker Date: Wed, 17 Jun 2026 17:05:45 -0700 Subject: [PATCH 2/3] Fix GDB core dump test hang caused by deferred error response GDB's native DAP defers launch/attach processing until after configurationDone. When GDB rejects an attach request (e.g. core dump on GDB < 18 which lacks coreFile support), the error response arrives in the post-configurationDone wait loop where it was silently skipped, causing an infinite hang. Track the launch/attach request sequence number and check for matching error responses in the StoppedEvent and breakpoint wait loops. This turns a 120s timeout into a fast error (~0.3s). Also: - Revert incorrect core dump protocol (attach + coreFile is correct, not launch + core) - Remove mandatory path requirement for GDB core mode (GDB 18+ can auto-detect executable from core file) - Add os.Chmod guard in TestGDBCoreDump to prevent false success - Skip GDB core dump tests on GDB < 18 with informative message --- backend.go | 3 ++- backend_test.go | 3 +++ tools.go | 42 ++++++++++++++++++++++----------- tools_test.go | 62 +++++++++++++++++++++++++++++++++---------------- 4 files changed, 75 insertions(+), 35 deletions(-) diff --git a/backend.go b/backend.go index 3daa887..eee94d0 100644 --- a/backend.go +++ b/backend.go @@ -231,7 +231,8 @@ func (g *gdbBackend) LaunchArgs(mode, programPath string, stopOnEntry bool, prog return args, nil } -// CoreRequestType returns "attach" because GDB handles core dumps via the attach request. +// CoreRequestType returns "attach" because GDB native DAP handles core dumps +// via the attach request with a "coreFile" argument. func (g *gdbBackend) CoreRequestType() string { return "attach" } diff --git a/backend_test.go b/backend_test.go index f376377..e912e18 100644 --- a/backend_test.go +++ b/backend_test.go @@ -169,6 +169,9 @@ func TestGDBBackendTransportMode(t *testing.T) { func TestGDBBackendCoreArgs(t *testing.T) { backend := &gdbBackend{gdbPath: "gdb"} + if backend.CoreRequestType() != "attach" { + t.Errorf("expected CoreRequestType 'attach', got: %s", backend.CoreRequestType()) + } args, err := backend.CoreArgs("/path/to/program", "/path/to/core") if err != nil { t.Fatalf("unexpected error: %v", err) diff --git a/tools.go b/tools.go index 32227c2..56b4b59 100644 --- a/tools.go +++ b/tools.go @@ -1070,9 +1070,8 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa log.Printf("warning: tool-level logging is not supported for Delve; Delve DAP logs go to the server log") } - if mode == "core" && params.Path == "" && debugger != "gdb" { - return nil, nil, fmt.Errorf("path is required for core mode with %s (only GDB can auto-detect the executable from a core file)", debugger) - } + // Note: GDB can auto-detect the executable from the core file, + // so params.Path is optional for GDB core mode. // Spawn DAP server via backend cmd, listenAddr, err := ds.backend.Spawn(port, ds.logWriter) @@ -1124,6 +1123,7 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa // Launch or attach using backend-specific args stopOnEntry := params.StopOnEntry || len(params.Breakpoints) == 0 + var launchSeq int switch mode { case "source", "binary": launchArgs, err := ds.backend.LaunchArgs(mode, params.Path, stopOnEntry, params.Args) @@ -1131,6 +1131,7 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa return nil, nil, err } req := ds.client.newRequest("launch") + launchSeq = req.Seq request := &dap.LaunchRequest{Request: *req} request.Arguments = toRawMessage(launchArgs) if err := ds.client.send(request); err != nil { @@ -1145,9 +1146,11 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa var request dap.Message if ds.backend.CoreRequestType() == "attach" { req := ds.client.newRequest("attach") + launchSeq = req.Seq request = &dap.AttachRequest{Request: *req, Arguments: rawArgs} } else if ds.backend.CoreRequestType() == "launch" { req := ds.client.newRequest("launch") + launchSeq = req.Seq request = &dap.LaunchRequest{Request: *req, Arguments: rawArgs} } else { return nil, nil, fmt.Errorf("unsupported core request type: %s", ds.backend.CoreRequestType()) @@ -1161,6 +1164,7 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa return nil, nil, err } req := ds.client.newRequest("attach") + launchSeq = req.Seq request := &dap.AttachRequest{Request: *req} request.Arguments = toRawMessage(attachArgs) if err := ds.client.send(request); err != nil { @@ -1171,13 +1175,13 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa // // Delve: launch response arrives immediately, then initialized event. // - // GDB native DAP: may send an "initialized" event before or after the - // launch response. + // GDB native DAP: sends initialized event first, then DEFERS the + // launch/attach response until after configurationDone is processed. // - // We unify both by reading messages until we see the initialized event. + // We read messages until we see the initialized event. // The launch response may arrive before or after — if it arrives here, - // we consume it. If it arrives later, it will be automatically skipped - // as an out-of-order response by subsequent seq-based readers. + // we consume it and check for errors. If it arrives later (GDB's deferred + // pattern), it will be checked in subsequent message-reading loops. for { msg, err := ds.client.ReadMessage() if err != nil { @@ -1188,7 +1192,6 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa if !resp.GetResponse().Success { return nil, nil, fmt.Errorf("unable to start debug session: %s", resp.GetResponse().Message) } - // Launch response consumed; continue reading for initialized event case *dap.InitializedEvent: _ = resp goto initialized @@ -1230,16 +1233,17 @@ initialized: return nil, nil, err } - // If the launch response was deferred (arrived after the initialized event), - // it will be automatically consumed and skipped as an out-of-order response by - // subsequent readAndValidateResponse/readTypedResponse calls, which match - // by request_seq. - // Register session-specific tools based on capabilities ds.registerSessionTools() // For core dump mode, the program is already stopped at the crash point. // Wait for the StoppedEvent from the adapter before returning context. + // + // GDB native DAP defers the launch/attach response until after + // configurationDone is processed. If the attach fails (e.g. unsupported + // coreFile parameter), the error response arrives here. We must check + // ResponseMessages to avoid hanging forever waiting for a StoppedEvent + // that will never come. if mode == "core" { for { msg, err := ds.client.ReadMessage() @@ -1257,6 +1261,11 @@ initialized: return result, nil, err } return stopSummary(result, ev.Body.Reason), nil, nil + case dap.ResponseMessage: + r := ev.GetResponse() + if r.RequestSeq == launchSeq && !r.Success { + return nil, nil, fmt.Errorf("unable to start debug session: %s", r.Message) + } case dap.EventMessage: continue } @@ -1295,6 +1304,11 @@ initialized: goto stopped case *dap.TerminatedEvent: goto stopped + case dap.ResponseMessage: + r := ev.GetResponse() + if r.RequestSeq == launchSeq && !r.Success { + return nil, nil, fmt.Errorf("unable to start debug session: %s", r.Message) + } } } stopped: diff --git a/tools_test.go b/tools_test.go index c730cc4..0e17e5d 100644 --- a/tools_test.go +++ b/tools_test.go @@ -1149,14 +1149,27 @@ func TestGDBCoreDump(t *testing.T) { corePath := generateCoreDump(t, binaryPath) defer os.Remove(corePath) + // Remove execute permission so GDB cannot re-run the binary. + // Core dump analysis only needs to read the binary for symbols; + // if GDB tries to execute it instead, the session will fail. + if err := os.Chmod(binaryPath, 0o444); err != nil { + t.Fatalf("Failed to chmod binary: %v", err) + } + + debugArgs := map[string]any{ + "debugger": "gdb", + "mode": "core", + "path": binaryPath, + "coreFilePath": corePath, + } + for k, v := range dapLogArgs(t) { + debugArgs[k] = v + } + defer dumpDAPLogs(t) + result, err := ts.session.CallTool(ts.ctx, &mcp.CallToolParams{ - Name: "debug", - Arguments: map[string]any{ - "debugger": "gdb", - "mode": "core", - "path": binaryPath, - "coreFilePath": corePath, - }, + Name: "debug", + Arguments: debugArgs, }) if err != nil { t.Fatalf("Failed to start GDB core debug session: %v", err) @@ -1168,6 +1181,11 @@ func TestGDBCoreDump(t *testing.T) { errorMsg = tc.Text } } + // GDB added DAP core file support after 17.2 (via attach + coreFile). + // Older versions reject the attach with this specific error. + if strings.Contains(errorMsg, "attach requires either") { + t.Skipf("GDB does not support DAP core file loading (requires GDB 18+): %s", errorMsg) + } t.Fatalf("GDB core debug session returned error: %s", errorMsg) } @@ -1184,9 +1202,8 @@ func TestGDBCoreDump(t *testing.T) { ts.stopDebugger(t) } -// Start a 'core' session for GDB passing a core file but no -// executable. GDB should be able to figure out the executable -// from the core file. +// GDB can auto-detect the executable from the core file, +// so the program path should be optional. func TestGDBCoreDumpWithoutPath(t *testing.T) { requireGDBDeps(t) @@ -1199,14 +1216,19 @@ func TestGDBCoreDumpWithoutPath(t *testing.T) { corePath := generateCoreDump(t, binaryPath) defer os.Remove(corePath) - // Do not pass "path" — GDB should auto-detect the executable from the core file. + debugArgs := map[string]any{ + "debugger": "gdb", + "mode": "core", + "coreFilePath": corePath, + } + for k, v := range dapLogArgs(t) { + debugArgs[k] = v + } + defer dumpDAPLogs(t) + result, err := ts.session.CallTool(ts.ctx, &mcp.CallToolParams{ - Name: "debug", - Arguments: map[string]any{ - "debugger": "gdb", - "mode": "core", - "coreFilePath": corePath, - }, + Name: "debug", + Arguments: debugArgs, }) if err != nil { t.Fatalf("Failed to start GDB core debug session without path: %v", err) @@ -1218,6 +1240,9 @@ func TestGDBCoreDumpWithoutPath(t *testing.T) { errorMsg = tc.Text } } + if strings.Contains(errorMsg, "attach requires either") { + t.Skipf("GDB does not support DAP core file loading (requires GDB 18+): %s", errorMsg) + } t.Fatalf("GDB core debug session without path returned error: %s", errorMsg) } @@ -1227,9 +1252,6 @@ func TestGDBCoreDumpWithoutPath(t *testing.T) { if !strings.Contains(contextStr, "crash") { t.Errorf("Expected stack trace to contain 'crash', got:\n%s", contextStr) } - if !strings.Contains(contextStr, "main") { - t.Errorf("Expected stack trace to contain 'main', got:\n%s", contextStr) - } ts.stopDebugger(t) } From 49d4111fc71f6d9914ada2c31f2827fd4b1d6c0d Mon Sep 17 00:00:00 2001 From: Derek Parker Date: Thu, 18 Jun 2026 10:22:04 -0700 Subject: [PATCH 3/3] Harden deferred response handling and restore review-suggested guards - Restore early validation for non-GDB core mode requiring path param - Use -1 sentinel for launchSeq so unset value can't match a real response - Add comments clarifying that successful deferred responses are intentional no-ops - Restore "main" assertion in TestGDBCoreDumpWithoutPath - Extract skipIfGDBLacksCoreFileSupport helper to reduce duplication --- tools.go | 9 ++++++--- tools_test.go | 20 ++++++++++++-------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/tools.go b/tools.go index 56b4b59..867a8c2 100644 --- a/tools.go +++ b/tools.go @@ -1070,8 +1070,9 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa log.Printf("warning: tool-level logging is not supported for Delve; Delve DAP logs go to the server log") } - // Note: GDB can auto-detect the executable from the core file, - // so params.Path is optional for GDB core mode. + if mode == "core" && params.Path == "" && debugger != "gdb" { + return nil, nil, fmt.Errorf("path is required for core mode with %s (only GDB can auto-detect the executable from a core file)", debugger) + } // Spawn DAP server via backend cmd, listenAddr, err := ds.backend.Spawn(port, ds.logWriter) @@ -1123,7 +1124,7 @@ func (ds *debuggerSession) debug(ctx context.Context, _ *mcp.CallToolRequest, pa // Launch or attach using backend-specific args stopOnEntry := params.StopOnEntry || len(params.Breakpoints) == 0 - var launchSeq int + launchSeq := -1 switch mode { case "source", "binary": launchArgs, err := ds.backend.LaunchArgs(mode, params.Path, stopOnEntry, params.Args) @@ -1266,6 +1267,7 @@ initialized: if r.RequestSeq == launchSeq && !r.Success { return nil, nil, fmt.Errorf("unable to start debug session: %s", r.Message) } + // Successful deferred response; keep waiting for StoppedEvent case dap.EventMessage: continue } @@ -1309,6 +1311,7 @@ initialized: if r.RequestSeq == launchSeq && !r.Success { return nil, nil, fmt.Errorf("unable to start debug session: %s", r.Message) } + // Successful deferred response; keep waiting for StoppedEvent } } stopped: diff --git a/tools_test.go b/tools_test.go index 0e17e5d..5d4b57a 100644 --- a/tools_test.go +++ b/tools_test.go @@ -283,6 +283,13 @@ func requireGDBDeps(t *testing.T) { } } +func skipIfGDBLacksCoreFileSupport(t *testing.T, errorMsg string) { + t.Helper() + if strings.Contains(errorMsg, "attach requires either") { + t.Skipf("GDB does not support DAP core file loading (requires GDB 18+): %s", errorMsg) + } +} + // compileTestCProgram compiles a C test program with debug symbols and returns the binary path. func compileTestCProgram(t *testing.T, cwd, name string) (binaryPath string, cleanup func()) { t.Helper() @@ -1181,11 +1188,7 @@ func TestGDBCoreDump(t *testing.T) { errorMsg = tc.Text } } - // GDB added DAP core file support after 17.2 (via attach + coreFile). - // Older versions reject the attach with this specific error. - if strings.Contains(errorMsg, "attach requires either") { - t.Skipf("GDB does not support DAP core file loading (requires GDB 18+): %s", errorMsg) - } + skipIfGDBLacksCoreFileSupport(t, errorMsg) t.Fatalf("GDB core debug session returned error: %s", errorMsg) } @@ -1240,9 +1243,7 @@ func TestGDBCoreDumpWithoutPath(t *testing.T) { errorMsg = tc.Text } } - if strings.Contains(errorMsg, "attach requires either") { - t.Skipf("GDB does not support DAP core file loading (requires GDB 18+): %s", errorMsg) - } + skipIfGDBLacksCoreFileSupport(t, errorMsg) t.Fatalf("GDB core debug session without path returned error: %s", errorMsg) } @@ -1252,6 +1253,9 @@ func TestGDBCoreDumpWithoutPath(t *testing.T) { if !strings.Contains(contextStr, "crash") { t.Errorf("Expected stack trace to contain 'crash', got:\n%s", contextStr) } + if !strings.Contains(contextStr, "main") { + t.Errorf("Expected stack trace to contain 'main', got:\n%s", contextStr) + } ts.stopDebugger(t) }