Page MenuHomePhabricator

[lldb-vscode] Distinguish shadowed variables in the scopes request
AbandonedPublic

Authored by wallace on Apr 6 2021, 1:30 PM.

Details

Summary

VSCode doesn't render multiple variables with the same name in the variables view. It only renders one of them. This is a situation that happens often when there are shadowed variables.
The nodejs debugger solves this by adding a number suffix to the variable, e.g. "x", "x2", "x3" are the different x variables in nested blocks.

In this patch I'm doing something similar, but the suffix is " @ <file_name:line>), e.g. "x @ main.cpp:17", "x @ main.cpp:21". The fallback would be an address if the source and line information is not present, which should be rare.

This fix is only needed for globals and locals. Children of variables don't suffer of this problem.

When there are shadowed variables

Without shadowed variables

Modifying these variables through the UI works

Diff Detail

Event Timeline

wallace requested review of this revision.Apr 6 2021, 1:30 PM
wallace created this revision.
wallace edited the summary of this revision. (Show Details)
clayborg requested changes to this revision.Apr 6 2021, 4:24 PM

I think this really should be fixed in the VS code variable view. It allows debugging of languages that support having multiple variables with the same name, so their debugger should support it.

But that won't happen right away, so I added some suggestions to make this better using SBDeclaration

lldb/tools/lldb-vscode/JSONUtils.cpp
994–998

We could use the data in the SBValue's declaration:

lldb::SBDeclaration SBValue::GetDeclaration();

Something like:

lldb::SBStream name_builder;
name_builder.Print(name ? name : "<null>");
if (name_repeats > 1) {
  name_builder.Print(" @ ")
  v.GetDeclaration().GetDescription(name_builder);
}
EmplaceSafeString(object, "name", name_builder.GetData());

Then the name would look something like "i @ main.cpp:12"

997–998

Can we just add spaces after the variable name here? So the first would be "i", the next would be "i " and the next would be "i ". We need to make sure that we use the real name for the "evaluateName" key:

[
  {"evaluateName":"i","id":1,"name":"i","type":"int","value":"0","variablesReference":0},
  {"evaluateName":"i","id":2,"name":"i ","type":"int","value":"0","variablesReference":0},
  {"evaluateName":"i","id":3,"name":"i  ","type":"int","value":"0","variablesReference":0},
]
This revision now requires changes to proceed.Apr 6 2021, 4:24 PM
wallace updated this revision to Diff 335881.Apr 7 2021, 11:35 AM

Some changes:

  • Now adding the source + line information to distinguish variables, and using the address as fallback, which should happen pretty rarely.
  • Now also supporting modifying these variables via request_setVariable. Before we could only modify the innermost variable, now we can change any variable due to uniquing.
  • Added more comprehensive tests
wallace edited the summary of this revision. (Show Details)Apr 7 2021, 11:36 AM
clayborg requested changes to this revision.Apr 7 2021, 1:37 PM
clayborg added inline comments.
lldb/tools/lldb-vscode/JSONUtils.cpp
922

variables don't always have a load address, as they can be in a register. SBValue objects can return their location to you as a string already:

const char *SBValue::GetLocation();
lldb/tools/lldb-vscode/lldb-vscode.cpp
2934

This variable name can be improved to accurately describe what it is doing. "variable_name_counts" would be my suggestion since that is what it is really doing.

You can also just make the key of this map "const char *" since the variable names come from ConstString values inside LLDB the pointers will be unique

This revision now requires changes to proceed.Apr 7 2021, 1:37 PM
wallace updated this revision to Diff 336945.Apr 12 2021, 1:26 PM

Address comments

clayborg accepted this revision.Tue, Apr 20, 4:13 PM

Just one inline fix switch from int to uint32_t and this is good to go.

lldb/tools/lldb-vscode/JSONUtils.cpp
917

This should be uint32_t instead of int, and probably add "const uint32_t" as well.

This revision is now accepted and ready to land.Tue, Apr 20, 4:13 PM
This revision was landed with ongoing or failed builds.Wed, Apr 21, 3:09 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Apr 21, 3:09 PM
teemperor reopened this revision.Thu, Apr 22, 4:58 AM
teemperor added a subscriber: teemperor.
teemperor added inline comments.
lldb/tools/lldb-vscode/lldb-vscode.cpp
2934

Let's not. The last time someone did this whole "This const char * is actually coming from a ConstString so assume ConstString semantics" it just caused us a bunch of pain (see D88490 and the related revisions).

Either add an assert that the strings are coming from a ConstString (assuming we really care about the performance benefit here) or use some StringMap/std::string-key approach.

This revision is now accepted and ready to land.Thu, Apr 22, 4:58 AM
teemperor requested changes to this revision.Thu, Apr 22, 4:58 AM
This revision now requires changes to proceed.Thu, Apr 22, 4:58 AM
clayborg added inline comments.Thu, Apr 22, 11:21 AM
lldb/tools/lldb-vscode/lldb-vscode.cpp
2934

We are getting a variable name via the public API which returns a "const char *". The only way this works is if the string comes from a ConstString otherwise there are lifetime issues. So this is and should be ok. The diff you are talking about was from internal LLDB code. For internal LLDB code we can enforce this by ensuring that people use ConstString, but externally through the API, we assume any "const char *" that comes out of it is uniqued and comes form a ConstString.

teemperor added inline comments.Thu, Apr 22, 12:33 PM
lldb/tools/lldb-vscode/lldb-vscode.cpp
2934

I don't think there is any promise that every const char * coming from the SB API is generated from a ConstString. If there was such a promise a good chunk of the SB API would already violate it.

That the const char * needs to have a reasonable life time (e.g., the same life time as the SB API object that returns it) seems like a basic foundation to have the API work at all. But promising that they are uniquified really serves essentially no purpose? I sure hope that simply returning string literals doesn't suddenly become an API violation (or that we need to forever store every generated error message that we return from the SB API).

Also I think my example is spot on: An unverified assumption that doesn't hold true. And once we change the underlying code someone has to spend a week tracking down bogus string comparisons in the year 2021.

wallace added inline comments.Thu, Apr 22, 7:54 PM
lldb/tools/lldb-vscode/lldb-vscode.cpp
2934

The number of variables shouldn't be massive, so I'll change it to std::string just for safety. I don't want to spend a lot of time debugging this if at some point the assumption doensn't hold.

wallace accepted this revision.Mon, May 3, 7:54 AM
wallace abandoned this revision.