lldb-dap: Stop using replicated variable ids #124232

Anthony-Eid · 2025-01-24T06:14:10Z

Closes #119784

This PR fixes two bugs:

It generates unique variable reference IDs per suspended debuggee state.
It stores all created variables in a stopped state instead of dropping variables in unselected scopes. So it can properly handle all scope/variable requests

It does this by storing all variables in their respective scopes and using that mapping in request handlers that relied on the old mapping. It dynamically creates new variable/scope IDs instead of resetting IDs whenever a new scope is created.

I also removed some unused code as well.

github-actions · 2025-01-24T06:14:28Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

medismailben · 2025-08-24T01:04:13Z

@Anthony-Eid could you rebase this PR, that'd be better to look at it

Anthony-Eid · 2025-08-26T22:46:15Z

@medismailben I just updated the branch by merging with main. I started a rebase but I don't have enough time to finish it until later tonight. I might see if I can cherry pick my changes instead of rebase because I think that would be easier to test

Anthony-Eid · 2025-08-27T17:10:53Z

@medismailben I fixed most of extra lines being added in the PR, but it's not 100% working in the new version. I'll ping you when it's functional.

I believe I'm mainly going to need help rewriting the tests once I get this functionality and following any llvm standards that I'm unaware of.

medismailben · 2025-08-27T17:14:55Z

Sounds good, let me know when you're ready 🙂

Anthony-Eid · 2025-08-27T22:21:24Z

@medismailben lldb-dap is usable from Zed with my most recent commit now! Do you have time this week to pair on getting this mergable? I'm pretty sure using new variable ids is going to break some tests and idk if there's formatters or anything else I have to fix to get CICD passing

medismailben · 2025-08-27T22:58:46Z

@medismailben lldb-dap is usable from Zed with my most recent commit now! Do you have time this week to pair on getting this mergable? I'm pretty sure using new variable ids is going to break some tests and idk if there's formatters or anything else I have to fix to get CICD passing

Awesome! I've kicked the llvm PR testing for your patch and it looks like the formatter job passed however there is some DAP test failures:

********************
Unresolved Tests (1):
  lldb-api :: tools/lldb-dap/databreakpoint/TestDAP_setDataBreakpoints.py
********************
Failed Tests (3):
  lldb-api :: tools/lldb-dap/memory/TestDAP_memory.py
  lldb-api :: tools/lldb-dap/variables/TestDAP_variables.py
  lldb-unit :: DAP/./DAPTests/VariablesTest/GetTopLevelScope_ReturnsCorrectScope
Testing Time: 93.86s

You should be able to look at the logs here: https://github.com/llvm/llvm-project/actions/runs/17280350831/job/49047894529?pr=124232

You can run the ninja check-lldb-api-tools-lldb-dap to run the DAP tests locally.

It would be nice to add your own test to make sure this doesn't regress in the future. Also, make sure to turn your draft into a PR when it's ready for review.

medismailben · 2025-08-27T23:19:03Z

Looking at the logs:

TestDAP_variables.py fails on line 290. It looks like we call self.set_local("argc", 123). I went looking for this implementation of that helper function. It located in lldbdap_testcase.py and calls dap_server.request_setVariable. It looks like when calling that function, we didn't pass any id for the variable. May be that's the issue you're having.
TestDAP_memory.py fails on line 73, which also calls to dap_server.request_setVariable.

The test and API changes now get scope references dynamically instead of using hardcoded values. This fixes the below tests: - testDAP_memory - TestDAP_variables - GetTopLevelScope_ReturnsCorrectScope

github-actions · 2025-08-28T04:46:16Z

✅ With the latest revision this PR passed the Python code formatter.

Variable reference 1 was hard coded to always be the local scope but since the var_ref of local scope is dynamic now it uses a helper function in the test

medismailben · 2025-08-28T05:01:43Z

@Anthony-Eid Almost there! Looks like all the tests are passing now you just need to fix the formatting for the python changes (you can run pip3 install darker && darker -r HEAD~ $LLVM/lldb) and mark the PR ready for review. Once you do that I'll add some reviewer to this PR so you can get some feedback before we merge it.

Anthony-Eid · 2025-08-28T06:35:00Z

@medismailben It's ready for review! Thank you for your guidance

Anthony-Eid · 2025-08-28T16:37:24Z

So I ran /Users/anth/Library/Python/3.9/bin/darker -r HEAD~ ~/Developer/llvm-project/lldb but it doesn't seem to be formatting the code correctly. What am I missing?

ashgti · 2025-08-28T17:40:40Z

lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp

+  int64_t variable_reference = dap.variables.GetNewVariableReference(false);
+  scopes.push_back(CreateScope("Locals", variable_reference,
+                               dap.variables.locals.GetSize(), false));
+
+  dap.variables.AddScopeKind(variable_reference, ScopeKind::Locals, frame_id);


Should AddScopeKind make a new ref internally and return the ref?

Or when we ready a frame, should this be handled then?

ashgti · 2025-08-28T17:42:21Z

lldb/tools/lldb-dap/Handler/ScopesRequestHandler.cpp

+  if (name == "Locals")
    scope.presentationHint = Scope::eScopePresentationHintLocals;
-  else if (variablesReference == VARREF_REGS)
+  else if (name == "Registers")
    scope.presentationHint = Scope::eScopePresentationHintRegisters;


Should we move this up a layer and take the presentation hint as a paramter instead of checking for specific names?

I passed ScopeKind in to the function instead of name now, and use ScopeKind variant to generate the correct presentation hint and name for the scope. Is that an adequate fix for this?

ashgti · 2025-08-28T17:44:42Z

lldb/tools/lldb-dap/Variables.cpp

+void Variables::ReadyFrame(uint32_t frame_id, lldb::SBFrame &frame) {
+  if (m_frames.find(frame_id) == m_frames.end()) {
+
+    auto locals = frame.GetVariables(/*arguments=*/true,
+                                     /*locals=*/true,
+                                     /*statics=*/false,
+                                     /*in_scope_only=*/true);
+
+    auto globals = frame.GetVariables(/*arguments=*/false,
+                                      /*locals=*/false,
+                                      /*statics=*/true,
+                                      /*in_scope_only=*/true);
+
+    auto registers = frame.GetRegisters();
+
+    m_frames.insert(
+        std::make_pair(frame_id, std::make_tuple(locals, globals, registers)));
+  }
+}


We may want to go ahead and make a variable reference for the locals, globals, registers here that we can use to keep track of the SBValueList.

What exactly is an SBValueList?

Would doing something like this work (under m_frames.insert())?

m_variables.insert(locals.var_ref, locals); m_variables.insert(global.var_ref, global); m_variables.insert(registers.var_ref, registers);

A SBValueList is the container for the variables, like the local or global or registers.

Would doing something like this work (under m_frames.insert())?

Yea

da-viper · 2025-08-28T18:40:32Z

I am not too happy with the SwitchFrame and ReadyFrame as it implies that we have a state within the stopped state.

It may not work if we are changing scopes in a stopped state (have not tested this patch locally) from #147105.

It seems we are storing redundant information. For example to get the Global scope we have to SwitchFrame then use the public Variable:global. We could drop some of the current abstraction that may no longer apply like the Variables::global , local ...

we can either have separate maps for variables and scope. or merge them together.

e.g

// separate 
struct ScopeData { // or something similar
     ScopeKind kind ; // Global, Local,  Register
     SBValueList value; // variables in that scope. 
}; 

// Map ref -> ScopeData 
denseMap<uint32_t, ScopeData> scopes; 
denseMap<uint32_t, SBValue> tempValues;

struct ReferencesTypes { // or something similar
     ReferenceType type ; // Global, Local,  Register, TempVariable (skip Permanent Variable) 
     std::variant<SBValueList, SBValue> value; // variables for that scope or variable.  
};

denseMap<uint32_t, ReferencesTypes> scopes_types;

Anthony-Eid · 2025-08-28T22:30:42Z

@da-viper Thank you for your feedback! To address some of it

I am not too happy with the SwitchFrame and ReadyFrame as it implies that we have a state within the stopped state.
I think it would make sense to unify these functions and see if we can get rid of SwitchFrame.

It may not work if we are changing scopes in a stopped state (have not tested this patch locally) from #147105.

This is one of the bugs I'm aiming to fix with this PR, specially because it's extremely noticeable in Zed's debugger. Zed caches most of the requests it makes, and because lldb-dap reuses scope/variable references it's only able to get correct scopes/variables for the first frame it requests from.

After running lldb-dap with my current branch the issue is fixed. I think it's because the variables request handler gets the correct scope from GetTopLevelScope (This calls SwitchFrame under the hood) and GetScopeKind correctly now.

It seems we are storing redundant information. For example to get the Global scope we have to SwitchFrame then use the public Variable:global. We could drop some of the current abstraction that may no longer apply like the Variables::global , local ...

we can either have separate maps for variables and scope. or merge them together.

I'll see if I can implement this. I might need some help because I've been primarily programming in Rust for a while now and am quite rusty when it comes to C++ 😀

Edit:

I removed Variables::global, local, registers and the SwitchFrame function. Variable/Scopes can be access via these methods

  struct ScopeData {
    ScopeKind kind;
    lldb::SBValueList scope;
 };

  std::optional<ScopeData> GetScopeKind(const int64_t variablesReference);
  lldb::SBValueList *GetScope(const uint32_t frame_id, const ScopeKind kind);

And the data is already stored in variable list via these mappings

  // Variable Reference,                 frame_id
  std::map<int64_t, std::pair<ScopeKind, uint32_t>> m_scope_kinds;

  /// Variables that are alive in this stop state.
  /// Will be cleared when debuggee resumes.
  llvm::DenseMap<int64_t, lldb::SBValue> m_referencedvariables;

  /// Key = frame_id
  /// Value = (locals, globals Registers) scopes
  std::map<uint32_t,
           std::tuple<lldb::SBValueList, lldb::SBValueList, lldb::SBValueList>>
      m_frames;

I got rid of a variables locals, globals, and register fields and added some helper methods to access scopes based on a var_ref or a frame_id and a ScopeKind variant

ashgti · 2025-08-29T16:00:05Z

lldb/tools/lldb-dap/Variables.cpp

+void Variables::AddScopeKind(int64_t variable_reference, ScopeKind kind,
+                             uint32_t frame_id) {
+
+  m_scope_kinds[variable_reference] = std::make_pair(kind, frame_id);
+}


When we ReadyFrame we could go ahead and add the m_scope_kinds[] for the frame local/globals/registers. No need to separate those out.

ashgti · 2025-08-29T16:01:58Z

lldb/tools/lldb-dap/Variables.cpp

+  return nullptr;
+}
+
+void Variables::ReadyFrame(uint32_t frame_id, lldb::SBFrame &frame) {


We could have this return a std::vector<Scope> for the local/global/register scope.

ashgti · 2025-08-29T16:07:34Z

lldb/tools/lldb-dap/JSONUtils.cpp

@@ -11,6 +11,7 @@
 #include "ExceptionBreakpoint.h"
 #include "LLDBUtils.h"
 #include "ProtocolUtils.h"
+#include "Variables.h"


Is variables needed any here? I don't see any usage...

ashgti · 2025-08-29T16:07:58Z

lldb/tools/lldb-dap/JSONUtils.h

@@ -9,6 +9,7 @@
 #ifndef LLDB_TOOLS_LLDB_DAP_JSONUTILS_H
 #define LLDB_TOOLS_LLDB_DAP_JSONUTILS_H

+#include "DAP.h"


We have DAPForward on line 13, do we need the full DAP here?

Anthony-Eid mentioned this pull request Aug 22, 2025

Debugger: Zed should use the official lldb-dap server instead of CodeLLDB zed-industries/zed#36730

Open

Fix variable request from reusing variable_ids

30658e9

Anthony-Eid force-pushed the fix-variable-request branch from cccddfc to 30658e9 Compare August 27, 2025 17:04

Add createScope changes

9707978

Anthony-Eid and others added 2 commits August 27, 2025 18:19

Fix some bugs

50230c2

Merge branch 'main' into fix-variable-request

be3e97d

remove unused GetScopedsKind func

c73d4c4

Fix hardcoded scope references in DAP variable handling

2712abd

The test and API changes now get scope references dynamically instead of using hardcoded values. This fixes the below tests: - testDAP_memory - TestDAP_variables - GetTopLevelScope_ReturnsCorrectScope

Fix set data breakpoint test by using the correct variable reference

d0f1e86

Variable reference 1 was hard coded to always be the local scope but since the var_ref of local scope is dynamic now it uses a helper function in the test

Anthony-Eid added 3 commits August 28, 2025 02:16

Have llm (Claude Opus 4) Generate a regression test

875b926

Format test files with darker

4dae801

Remove unused global defines and set First Var Ref to 1

a52fe07

Anthony-Eid changed the title ~~WIP: Stop using replicated variable ids~~ lldb-dap: Stop using replicated variable ids Aug 28, 2025

Anthony-Eid marked this pull request as ready for review August 28, 2025 06:34

Anthony-Eid requested a review from JDevlieghere as a code owner August 28, 2025 06:34

JDevlieghere requested review from ashgti, da-viper and eronnen August 28, 2025 16:58

ashgti reviewed Aug 28, 2025

View reviewed changes

Use scopekind instead of hardcoded comparisons in CreateScope

5f47586

Anthony-Eid added 3 commits August 29, 2025 01:43

Stop storing duplicate scopes in variables

b7548f1

I got rid of a variables locals, globals, and register fields and added some helper methods to access scopes based on a var_ref or a frame_id and a ScopeKind variant

Apply Python formatting to fix CI darker checks

c8196a2

Hopefully fix failing tests (They're not running on my end)

2387952

ashgti reviewed Aug 29, 2025

View reviewed changes

Actually fix the failing tests for real

bc00325

lldb-dap: Stop using replicated variable ids #124232

Are you sure you want to change the base?

lldb-dap: Stop using replicated variable ids #124232

Conversation

Anthony-Eid commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 24, 2025

Uh oh!

medismailben commented Aug 24, 2025

Uh oh!

Anthony-Eid commented Aug 26, 2025

Uh oh!

Anthony-Eid commented Aug 27, 2025

Uh oh!

medismailben commented Aug 27, 2025

Uh oh!

Anthony-Eid commented Aug 27, 2025

Uh oh!

medismailben commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medismailben commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

medismailben commented Aug 28, 2025

Uh oh!

Anthony-Eid commented Aug 28, 2025

Uh oh!

Anthony-Eid commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

da-viper commented Aug 28, 2025

Uh oh!

Anthony-Eid commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Anthony-Eid commented Jan 24, 2025 •

edited

Loading

medismailben commented Aug 27, 2025 •

edited

Loading

medismailben commented Aug 27, 2025 •

edited

Loading

github-actions bot commented Aug 28, 2025 •

edited

Loading

Anthony-Eid commented Aug 28, 2025 •

edited

Loading