From 862d0c07ca434315555fd086bc1327d3d0ac502d Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Mon, 7 Apr 2025 18:02:13 -0400 Subject: [PATCH] debugger: Fix gdb adapter and logger (#28280) There were two bugs that caused the gdb adapter not to work properly, one on our end and one their end. The bug on our end was sending `stopOnEntry: null` in our launch request when stop on entry had no value. I fixed that bug across all dap adapters The other bug had to do with python's "great" type system and how we serialized our unit structs to json; mainly, `ConfigurationDoneArguments` and `ThreadsArguments`. Gdb seems to follow a pattern for handling requests where they pass `**args` to a function, this errors out when the equivalent json is `"arguments": null`. ```py @capability("supportsConfigurationDoneRequest") @request("configurationDone", on_dap_thread=True) def config_done(**args): ### BUG!! ... ``` Release Notes: - N/A --- Cargo.lock | 2 +- Cargo.toml | 2 +- crates/dap_adapters/src/gdb.rs | 34 ++++++++++-- crates/dap_adapters/src/go.rs | 12 +++-- crates/dap_adapters/src/javascript.rs | 21 ++++---- crates/dap_adapters/src/lldb.rs | 21 ++++---- crates/dap_adapters/src/php.rs | 2 +- crates/debugger_ui/src/debugger_panel.rs | 6 +-- crates/debugger_ui/src/session.rs | 4 +- .../debugger_ui/src/tests/debugger_panel.rs | 4 +- crates/project/src/debugger/dap_command.rs | 6 +-- crates/project/src/debugger/dap_store.rs | 17 +++--- crates/project/src/debugger/session.rs | 53 +++++++++++++------ 13 files changed, 120 insertions(+), 64 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 479dfa500f..482359f8e3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4050,7 +4050,7 @@ dependencies = [ [[package]] name = "dap-types" version = "0.0.1" -source = "git+https://github.com/zed-industries/dap-types?rev=bfd4af0#bfd4af084bbaa5f344e6925370d7642e41d0b5b8" +source = "git+https://github.com/zed-industries/dap-types?rev=be69a016ba710191b9fdded28c8b042af4b617f7#be69a016ba710191b9fdded28c8b042af4b617f7" dependencies = [ "schemars", "serde", diff --git a/Cargo.toml b/Cargo.toml index 26ac487a07..9127a8c827 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -425,7 +425,7 @@ core-foundation = "0.10.0" core-foundation-sys = "0.8.6" ctor = "0.4.0" dashmap = "6.0" -dap-types = { git = "https://github.com/zed-industries/dap-types", rev = "bfd4af0" } +dap-types = { git = "https://github.com/zed-industries/dap-types", rev = "be69a016ba710191b9fdded28c8b042af4b617f7" } derive_more = "0.99.17" dirs = "4.0" ec4rs = "1.1" diff --git a/crates/dap_adapters/src/gdb.rs b/crates/dap_adapters/src/gdb.rs index 809e2ba96a..e67556b6ae 100644 --- a/crates/dap_adapters/src/gdb.rs +++ b/crates/dap_adapters/src/gdb.rs @@ -3,7 +3,7 @@ use std::ffi::OsStr; use anyhow::{Result, bail}; use async_trait::async_trait; use gpui::AsyncApp; -use task::{DebugAdapterConfig, DebugTaskDefinition}; +use task::{DebugAdapterConfig, DebugRequestType, DebugTaskDefinition}; use crate::*; @@ -74,13 +74,37 @@ impl DebugAdapter for GdbDebugAdapter { } fn request_args(&self, config: &DebugTaskDefinition) -> Value { + let mut args = json!({ + "request": match config.request { + DebugRequestType::Launch(_) => "launch", + DebugRequestType::Attach(_) => "attach", + }, + }); + + let map = args.as_object_mut().unwrap(); match &config.request { - dap::DebugRequestType::Attach(attach_config) => { - json!({"pid": attach_config.process_id}) + DebugRequestType::Attach(attach) => { + map.insert("pid".into(), attach.process_id.into()); } - dap::DebugRequestType::Launch(launch_config) => { - json!({"program": launch_config.program, "cwd": launch_config.cwd, "stopOnEntry": config.stop_on_entry, "args": launch_config.args.clone()}) + + DebugRequestType::Launch(launch) => { + map.insert("program".into(), launch.program.clone().into()); + + if !launch.args.is_empty() { + map.insert("args".into(), launch.args.clone().into()); + } + + if let Some(stop_on_entry) = config.stop_on_entry { + map.insert( + "stopAtBeginningOfMainSubprogram".into(), + stop_on_entry.into(), + ); + } + if let Some(cwd) = launch.cwd.as_ref() { + map.insert("cwd".into(), cwd.to_string_lossy().into_owned().into()); + } } } + args } } diff --git a/crates/dap_adapters/src/go.rs b/crates/dap_adapters/src/go.rs index e4f6bb097c..ecb1408ca0 100644 --- a/crates/dap_adapters/src/go.rs +++ b/crates/dap_adapters/src/go.rs @@ -83,19 +83,25 @@ impl DebugAdapter for GoDebugAdapter { } fn request_args(&self, config: &DebugTaskDefinition) -> Value { - match &config.request { + let mut args = match &config.request { dap::DebugRequestType::Attach(attach_config) => { json!({ "processId": attach_config.process_id, - "stopOnEntry": config.stop_on_entry, }) } dap::DebugRequestType::Launch(launch_config) => json!({ "program": launch_config.program, "cwd": launch_config.cwd, - "stopOnEntry": config.stop_on_entry, "args": launch_config.args }), + }; + + let map = args.as_object_mut().unwrap(); + + if let Some(stop_on_entry) = config.stop_on_entry { + map.insert("stopOnEntry".into(), stop_on_entry.into()); } + + args } } diff --git a/crates/dap_adapters/src/javascript.rs b/crates/dap_adapters/src/javascript.rs index 485014749f..5022f0ac76 100644 --- a/crates/dap_adapters/src/javascript.rs +++ b/crates/dap_adapters/src/javascript.rs @@ -131,19 +131,20 @@ impl DebugAdapter for JsDebugAdapter { match &config.request { DebugRequestType::Attach(attach) => { map.insert("processId".into(), attach.process_id.into()); - map.insert("stopOnEntry".into(), config.stop_on_entry.into()); } DebugRequestType::Launch(launch) => { map.insert("program".into(), launch.program.clone().into()); - map.insert("args".into(), launch.args.clone().into()); - map.insert( - "cwd".into(), - launch - .cwd - .as_ref() - .map(|s| s.to_string_lossy().into_owned()) - .into(), - ); + + if !launch.args.is_empty() { + map.insert("args".into(), launch.args.clone().into()); + } + + if let Some(stop_on_entry) = config.stop_on_entry { + map.insert("stopOnEntry".into(), stop_on_entry.into()); + } + if let Some(cwd) = launch.cwd.as_ref() { + map.insert("cwd".into(), cwd.to_string_lossy().into_owned().into()); + } } } args diff --git a/crates/dap_adapters/src/lldb.rs b/crates/dap_adapters/src/lldb.rs index d319f5e3e0..bb3f28c210 100644 --- a/crates/dap_adapters/src/lldb.rs +++ b/crates/dap_adapters/src/lldb.rs @@ -81,16 +81,17 @@ impl DebugAdapter for LldbDebugAdapter { } DebugRequestType::Launch(launch) => { map.insert("program".into(), launch.program.clone().into()); - map.insert("stopOnEntry".into(), config.stop_on_entry.into()); - map.insert("args".into(), launch.args.clone().into()); - map.insert( - "cwd".into(), - launch - .cwd - .as_ref() - .map(|s| s.to_string_lossy().into_owned()) - .into(), - ); + + if !launch.args.is_empty() { + map.insert("args".into(), launch.args.clone().into()); + } + + if let Some(stop_on_entry) = config.stop_on_entry { + map.insert("stopOnEntry".into(), stop_on_entry.into()); + } + if let Some(cwd) = launch.cwd.as_ref() { + map.insert("cwd".into(), cwd.to_string_lossy().into_owned().into()); + } } } args diff --git a/crates/dap_adapters/src/php.rs b/crates/dap_adapters/src/php.rs index 9dbdf5bee4..76134a818f 100644 --- a/crates/dap_adapters/src/php.rs +++ b/crates/dap_adapters/src/php.rs @@ -119,7 +119,7 @@ impl DebugAdapter for PhpDebugAdapter { "program": launch_config.program, "cwd": launch_config.cwd, "args": launch_config.args, - "stopOnEntry": config.stop_on_entry, + "stopOnEntry": config.stop_on_entry.unwrap_or_default(), }) } } diff --git a/crates/debugger_ui/src/debugger_panel.rs b/crates/debugger_ui/src/debugger_panel.rs index a671b6ebee..b1cc860663 100644 --- a/crates/debugger_ui/src/debugger_panel.rs +++ b/crates/debugger_ui/src/debugger_panel.rs @@ -185,7 +185,7 @@ impl DebugPanel { ) -> Vec> { self.sessions .iter() - .filter(|item| item.read(cx).session_id(cx) == Some(*client_id)) + .filter(|item| item.read(cx).session_id(cx) == *client_id) .map(|item| item.clone()) .collect() } @@ -200,7 +200,7 @@ impl DebugPanel { .find(|item| { let item = item.read(cx); - item.session_id(cx) == Some(client_id) + item.session_id(cx) == client_id }) .cloned() } @@ -227,7 +227,7 @@ impl DebugPanel { if self .sessions .iter() - .any(|item| item.read(cx).session_id(cx) == Some(*session_id)) + .any(|item| item.read(cx).session_id(cx) == *session_id) { // We already have an item for this session. return; diff --git a/crates/debugger_ui/src/session.rs b/crates/debugger_ui/src/session.rs index 22768538a9..ddda3a2c69 100644 --- a/crates/debugger_ui/src/session.rs +++ b/crates/debugger_ui/src/session.rs @@ -75,9 +75,9 @@ impl DebugSession { }) } - pub(crate) fn session_id(&self, cx: &App) -> Option { + pub(crate) fn session_id(&self, cx: &App) -> SessionId { match &self.mode { - DebugSessionState::Running(entity) => Some(entity.read(cx).session_id()), + DebugSessionState::Running(entity) => entity.read(cx).session_id(), } } diff --git a/crates/debugger_ui/src/tests/debugger_panel.rs b/crates/debugger_ui/src/tests/debugger_panel.rs index e685de7a14..2ba9d12154 100644 --- a/crates/debugger_ui/src/tests/debugger_panel.rs +++ b/crates/debugger_ui/src/tests/debugger_panel.rs @@ -270,7 +270,7 @@ async fn test_we_can_only_have_one_panel_per_debug_session( .clone() }); - assert_eq!(client.id(), active_session.read(cx).session_id(cx).unwrap()); + assert_eq!(client.id(), active_session.read(cx).session_id(cx)); assert_eq!( ThreadId(1), running_state.read(cx).selected_thread_id().unwrap() @@ -307,7 +307,7 @@ async fn test_we_can_only_have_one_panel_per_debug_session( .clone() }); - assert_eq!(client.id(), active_session.read(cx).session_id(cx).unwrap()); + assert_eq!(client.id(), active_session.read(cx).session_id(cx)); assert_eq!( ThreadId(1), running_state.read(cx).selected_thread_id().unwrap() diff --git a/crates/project/src/debugger/dap_command.rs b/crates/project/src/debugger/dap_command.rs index 0685e513d1..60fade53eb 100644 --- a/crates/project/src/debugger/dap_command.rs +++ b/crates/project/src/debugger/dap_command.rs @@ -1479,7 +1479,7 @@ impl LocalDapCommand for ThreadsCommand { type DapRequest = dap::requests::Threads; fn to_dap(&self) -> ::Arguments { - () + dap::ThreadsArgument {} } fn response_from_dap( @@ -1568,7 +1568,7 @@ impl LocalDapCommand for Initialize { } #[derive(Clone, Debug, Hash, PartialEq)] -pub(super) struct ConfigurationDone; +pub(super) struct ConfigurationDone {} impl LocalDapCommand for ConfigurationDone { type Response = (); @@ -1581,7 +1581,7 @@ impl LocalDapCommand for ConfigurationDone { } fn to_dap(&self) -> ::Arguments { - dap::ConfigurationDoneArguments + dap::ConfigurationDoneArguments {} } fn response_from_dap( diff --git a/crates/project/src/debugger/dap_store.rs b/crates/project/src/debugger/dap_store.rs index 3a90b68f5c..7f266f8c41 100644 --- a/crates/project/src/debugger/dap_store.rs +++ b/crates/project/src/debugger/dap_store.rs @@ -843,12 +843,17 @@ fn create_new_session( cx.notify(); })?; - match session - .update(cx, |session, cx| { - session.initialize_sequence(initialized_rx, cx) - })? - .await - { + match { + session + .update(cx, |session, cx| session.request_initialize(cx))? + .await?; + + session + .update(cx, |session, cx| { + session.initialize_sequence(initialized_rx, cx) + })? + .await + } { Ok(_) => {} Err(error) => { this.update(cx, |this, cx| { diff --git a/crates/project/src/debugger/session.rs b/crates/project/src/debugger/session.rs index 6d205e70bb..c38dbe2f87 100644 --- a/crates/project/src/debugger/session.rs +++ b/crates/project/src/debugger/session.rs @@ -10,7 +10,7 @@ use super::dap_command::{ VariablesCommand, }; use super::dap_store::DapAdapterDelegate; -use anyhow::{Result, anyhow}; +use anyhow::{Context as _, Result, anyhow}; use collections::{HashMap, HashSet, IndexMap, IndexSet}; use dap::adapters::{DebugAdapter, DebugAdapterBinary}; use dap::messages::Response; @@ -190,7 +190,7 @@ impl LocalMode { delegate: DapAdapterDelegate, messages_tx: futures::channel::mpsc::UnboundedSender, cx: AsyncApp, - ) -> Task> { + ) -> Task> { Self::new_inner( debug_adapters, session_id, @@ -214,7 +214,7 @@ impl LocalMode { caps: Capabilities, fail: bool, cx: AsyncApp, - ) -> Task> { + ) -> Task> { use task::DebugRequestDisposition; let request = match config.request.clone() { @@ -349,7 +349,7 @@ impl LocalMode { messages_tx: futures::channel::mpsc::UnboundedSender, on_initialized: impl AsyncFnOnce(&mut LocalMode, AsyncApp) + 'static, cx: AsyncApp, - ) -> Task> { + ) -> Task> { cx.spawn(async move |cx| { let (adapter, binary) = Self::get_adapter_binary(®istry, &config, &delegate, cx).await?; @@ -374,11 +374,11 @@ impl LocalMode { message_handler, cx.clone(), ) - .await? + .await + .with_context(|| "Failed to start communication with debug adapter")? }, ); - let adapter_id = adapter.name().to_string().to_owned(); let mut session = Self { client, adapter, @@ -387,11 +387,8 @@ impl LocalMode { }; on_initialized(&mut session, cx.clone()).await; - let capabilities = session - .request(Initialize { adapter_id }, cx.background_executor().clone()) - .await?; - Ok((session, capabilities)) + Ok(session) }) } @@ -541,6 +538,12 @@ impl LocalMode { self.config.label.clone() } + fn request_initialization(&self, cx: &App) -> Task> { + let adapter_id = self.adapter.name().to_string(); + + self.request(Initialize { adapter_id }, cx.background_executor().clone()) + } + fn initialize_sequence( &self, capabilities: &Capabilities, @@ -590,7 +593,7 @@ impl LocalMode { cx.update(|cx| this.send_all_breakpoints(false, cx))?.await; if configuration_done_supported { - this.request(ConfigurationDone, cx.background_executor().clone()) + this.request(ConfigurationDone {}, cx.background_executor().clone()) } else { Task::ready(Ok(())) } @@ -849,7 +852,7 @@ impl Session { let (message_tx, message_rx) = futures::channel::mpsc::unbounded(); cx.spawn(async move |cx| { - let (mode, capabilities) = LocalMode::new( + let mode = LocalMode::new( debug_adapters, session_id, parent_session.clone(), @@ -870,7 +873,6 @@ impl Session { initialized_tx, message_rx, mode, - capabilities, cx, ) }) @@ -893,7 +895,7 @@ impl Session { let (message_tx, message_rx) = futures::channel::mpsc::unbounded(); cx.spawn(async move |cx| { - let (mode, capabilities) = LocalMode::new_fake( + let mode = LocalMode::new_fake( session_id, parent_session.clone(), breakpoint_store.clone(), @@ -915,7 +917,6 @@ impl Session { initialized_tx, message_rx, mode, - capabilities, cx, ) }) @@ -1007,6 +1008,25 @@ impl Session { } } + pub(super) fn request_initialize(&mut self, cx: &mut Context) -> Task> { + match &self.mode { + Mode::Local(local_mode) => { + let capabilities = local_mode.clone().request_initialization(cx); + + cx.spawn(async move |this, cx| { + let capabilities = capabilities.await?; + this.update(cx, |session, _| { + session.capabilities = capabilities; + })?; + Ok(()) + }) + } + Mode::Remote(_) => Task::ready(Err(anyhow!( + "Cannot send initialize request from remote session" + ))), + } + } + pub(super) fn initialize_sequence( &mut self, initialize_rx: oneshot::Receiver<()>, @@ -1947,7 +1967,6 @@ fn create_local_session( initialized_tx: oneshot::Sender<()>, mut message_rx: futures::channel::mpsc::UnboundedReceiver, mode: LocalMode, - capabilities: Capabilities, cx: &mut Context, ) -> Session { let _background_tasks = vec![cx.spawn(async move |this: WeakEntity, cx| { @@ -2003,7 +2022,7 @@ fn create_local_session( child_session_ids: HashSet::default(), parent_id: parent_session.map(|session| session.read(cx).id), variables: Default::default(), - capabilities, + capabilities: Capabilities::default(), thread_states: ThreadStates::default(), output_token: OutputToken(0), ignore_breakpoints: false,