[Lldb-commits] [PATCH] D105166: Fix expression evaluation result expansion in lldb-vscode
Greg Clayton via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Tue Jun 29 19:33:44 PDT 2021
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Mostly LLDB and LLVM coding guideline conventions that need to be followed. Many comments and code suggestions. But there are some code changes like having unique IDs for temp variable references and permanent ones.
Main issues are:
- LLVM coding guidelines can't go over 80 columns, must wrap. If you use Phabricator it would fix these for you
- LLDB variable names are lower case with underscores between words ("isPermanent" should be "is_permament")
- LLDB methods are camel case and start with upper case letter ("getNewVariableReference" should be "GetNewVariableReference")
- LLVM coding guidelines want no braces on single line if/else
- LLVM coding guidelines for inline variable names are to use inline C comments with equal sign after variable name: "/*is_permanent=*/true"
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:33
VSCode::VSCode()
- : variables(), broadcaster("lldb-vscode"), num_regs(0), num_locals(0),
- num_globals(0), log(),
+ : broadcaster("lldb-vscode"), log(),
exception_breakpoints(
----------------
as long as we are changing this, we don't need to default construct anything
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:384-387
+ scopes.emplace_back(CreateScope("Locals", VARREF_LOCALS, g_vsc.variableHolder.locals.GetSize(), false));
scopes.emplace_back(
- CreateScope("Globals", VARREF_GLOBALS, num_globals, false));
- scopes.emplace_back(CreateScope("Registers", VARREF_REGS, num_regs, false));
+ CreateScope("Globals", VARREF_GLOBALS, g_vsc.variableHolder.globals.GetSize(), false));
+ scopes.emplace_back(CreateScope("Registers", VARREF_REGS, g_vsc.variableHolder.registers.GetSize(), false));
----------------
over 80 character column limit
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:529
+void VariablesHolder::clear() {
+ this->locals.Clear();
----------------
LLDB function naming is camel case with upper case first letter.
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:530-533
+ this->locals.Clear();
+ this->globals.Clear();
+ this->registers.Clear();
+ this->expandableVariables.clear();
----------------
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:537-539
+ const int64_t newVariableReference = isPermanent ? ((1ll << PermanentVariableReferenceTagBit) | nextVariableReferenceSeed) : nextVariableReferenceSeed;
+ ++nextVariableReferenceSeed;
+ return newVariableReference;
----------------
80 columns max and use separate var ref indexes
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:542-544
+bool VariablesHolder::isPermanentVariableReference(int64_t variableReference) {
+ return (variableReference & (1ll << PermanentVariableReferenceTagBit)) != 0;
+}
----------------
fix LLDB coding guidelines
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:546
+
+lldb::SBValue VariablesHolder::getVariableFromVariableReference(int64_t value) {
+ lldb::SBValue variable;
----------------
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:548-553
+ auto isPermanent = isPermanentVariableReference(value);
+ if (isPermanent) {
+ variable = expandablePermanentVariables.find(value)->second;
+ } else {
+ variable = expandableVariables.find(value)->second;
+ }
----------------
Follow LLVM coding guidelines where there are no braces on single statement if/else, lldb var names
================
Comment at: lldb/tools/lldb-vscode/VSCode.cpp:557-565
+int64_t VariablesHolder::insertNewExpandableVariable(lldb::SBValue expandableVariable, bool isPermanent) {
+ auto newVariablesReferences = getNewVariableRefence(isPermanent);
+ if (isPermanent) {
+ expandablePermanentVariables.insert(std::make_pair(newVariablesReferences, expandableVariable));
+ } else {
+ expandableVariables.insert(std::make_pair(newVariablesReferences, expandableVariable));
+ }
----------------
LLDB coding guidelines and 80 column limit
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:84
+struct VariablesHolder {
+ // Bit tag bit to tell if a variableReference is inside expandablePermanentVariables or not.
----------------
Rename
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:86
+ // Bit tag bit to tell if a variableReference is inside expandablePermanentVariables or not.
+ static constexpr int PermanentVariableReferenceTagBit = 32;
+
----------------
We should just define this as a mask instead of a bit number as we always use it as a mask.
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:92
+
+ int64_t nextVariableReferenceSeed{VARREF_FIRST_VAR_IDX};
+ llvm::DenseMap<int64_t, lldb::SBValue> expandableVariables;
----------------
LLDB naming conventions, and we should have two IDs, one for permanent and one for temp
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:93
+ int64_t nextVariableReferenceSeed{VARREF_FIRST_VAR_IDX};
+ llvm::DenseMap<int64_t, lldb::SBValue> expandableVariables;
+ llvm::DenseMap<int64_t, lldb::SBValue> expandablePermanentVariables;
----------------
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:94
+ llvm::DenseMap<int64_t, lldb::SBValue> expandableVariables;
+ llvm::DenseMap<int64_t, lldb::SBValue> expandablePermanentVariables;
+
----------------
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:97
+ // Check if \p variableReference points to variable in a expandablePermanentVariables.
+ bool isPermanentVariableReference(int64_t variableReference);
+
----------------
Follow LLDB coding guidelines for locals which differs from LLVM, and functions start with capital letter
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:100
+ // \return a new variableReference. If isPermanent is true the returned
+ // value will have PermanentVariableReferenceTagBit set.
+ int64_t getNewVariableRefence(bool isPermanent);
----------------
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:101
+ // value will have PermanentVariableReferenceTagBit set.
+ int64_t getNewVariableRefence(bool isPermanent);
+
----------------
"is_permanent" is the LLDB coding style for locals and function names
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:103
+
+ // \return the expandable variable corresponding with variableReference value of \p value.
+ lldb::SBValue getVariableFromVariableReference(int64_t value);
----------------
80 columns max, wrap this comment per llvm coding guidelines.
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:104
+ // \return the expandable variable corresponding with variableReference value of \p value.
+ lldb::SBValue getVariableFromVariableReference(int64_t value);
+
----------------
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:107
+ // Insert a new \p expandableVariable.
+ int64_t insertNewExpandableVariable(lldb::SBValue expandableVariable, bool isPermanent);
+
----------------
80 columns max, wrap this comment per llvm coding guidelines and make LLDB variable names.
================
Comment at: lldb/tools/lldb-vscode/VSCode.h:119
lldb::SBTarget target;
- lldb::SBValueList variables;
+ VariablesHolder variableHolder;
lldb::SBBroadcaster broadcaster;
----------------
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:112
+void willContinue() {
+ g_vsc.variableHolder.clear();
+}
----------------
make a "g_vsc.willContinue()" method in case we need to add more things later and move "g_vsc.variableHolder.clear();" into that new method
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:115
+
+lldb::SBValueList* getScopeRef(int64_t variablesReference) {
+ assert(VARREF_IS_SCOPE(variablesReference));
----------------
Lets just call this function to see if the variable reference is a top level scope. This will avoid any asserts and clean up the code. Name of this function can improve
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:116
+lldb::SBValueList* getScopeRef(int64_t variablesReference) {
+ assert(VARREF_IS_SCOPE(variablesReference));
+ switch (variablesReference) {
----------------
remove assert, we can return NULL to indicate this isn't a top level scope.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1238-1239
if (value.MightHaveChildren()) {
- auto variablesReference = VARIDX_TO_VARREF(g_vsc.variables.GetSize());
- g_vsc.variables.Append(value);
- body.try_emplace("variablesReference", variablesReference);
+ auto variableReference = g_vsc.variableHolder.insertNewExpandableVariable(value, /*isPermanent*/context == "repl");
+ body.try_emplace("variablesReference", variableReference);
} else {
----------------
80 column limit
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1918-1925
+ g_vsc.variableHolder.locals = frame.GetVariables(true, // arguments
true, // locals
false, // statics
- true)); // in_scope_only
- g_vsc.num_locals = g_vsc.variables.GetSize();
- g_vsc.variables.Append(frame.GetVariables(false, // arguments
+ true); // in_scope_only
+ g_vsc.variableHolder.globals = frame.GetVariables(false, // arguments
false, // locals
true, // statics
----------------
LLDB and LLVM inline comment coding guideline issues
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2771-2776
} else if (VARREF_IS_SCOPE(variablesReference)) {
// variablesReference is one of our scopes, not an actual variable it is
// asking for a variable in locals or globals or registers
int64_t start_idx = 0;
int64_t end_idx = 0;
+ lldb::SBValueList *pScopeRef = getScopeRef(variablesReference);
----------------
Use new GetTopLevelScope function return value
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2778
for (int64_t i = end_idx - 1; i >= start_idx; --i) {
+ lldb::SBValue curr_variable = pScopeRef->GetValueAtIndex(i);
----------------
We don't have to search backwards anymore since we make unique variable names when we have duplicates. So this for loop can now be:
```
uint64_t end_idx = pScopeRef->GetSize();
for (uint64_t i=0; i<end_idx; ++i)
```
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2779
for (int64_t i = end_idx - 1; i >= start_idx; --i) {
- lldb::SBValue curr_variable = g_vsc.variables.GetValueAtIndex(i);
+ lldb::SBValue curr_variable = pScopeRef->GetValueAtIndex(i);
std::string variable_name = CreateUniqueVariableNameForDisplay(
----------------
use lldb variable name
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2795
// withing the container variable by name.
- const int64_t var_idx = VARREF_TO_VARIDX(variablesReference);
- lldb::SBValue container = g_vsc.variables.GetValueAtIndex(var_idx);
+ lldb::SBValue container = g_vsc.variableHolder.getVariableFromVariableReference(variablesReference);
variable = container.GetChildMemberWithName(name.data());
----------------
80 column exceeded. I would suggest renaming "getVariableFromVariableReference" to a shorter name like just "GetVariable(int64_t var_Ref)"
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2811
if (variable.MightHaveChildren()) {
- newVariablesReference = VARIDX_TO_VARREF(g_vsc.variables.GetSize());
- g_vsc.variables.Append(variable);
+ auto isPermanent = g_vsc.variableHolder.isPermanentVariableReference(variablesReference);
+ g_vsc.variableHolder.insertNewExpandableVariable(variable, isPermanent);
----------------
80 column limit and naming conventions.
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2923-2930
if (VARREF_IS_SCOPE(variablesReference)) {
// variablesReference is one of our scopes, not an actual variable it is
// asking for the list of args, locals or globals.
int64_t start_idx = 0;
int64_t num_children = 0;
- switch (variablesReference) {
- case VARREF_LOCALS:
- start_idx = start;
- num_children = g_vsc.num_locals;
- break;
- case VARREF_GLOBALS:
- start_idx = start + g_vsc.num_locals + start;
- num_children = g_vsc.num_globals;
- break;
- case VARREF_REGS:
- start_idx = start + g_vsc.num_locals + g_vsc.num_globals;
- num_children = g_vsc.num_regs;
- break;
- default:
- break;
- }
+ lldb::SBValueList *pScopeRef = getScopeRef(variablesReference);
+ assert(pScopeRef != nullptr);
----------------
Use return value of GetTopLevelScope() and rename vars
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2950
+
+ int64_t variableReference = 0;
+ if (variable.MightHaveChildren()) {
----------------
lldb var names
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2951-2953
+ if (variable.MightHaveChildren()) {
+ variableReference = g_vsc.variableHolder.insertNewExpandableVariable(variable, /*isPermanent*/false);
+ }
----------------
clayborg wrote:
> 80 columns exceeded
remove braces and obey 80 column limit
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2951-2954
+ if (variable.MightHaveChildren()) {
+ variableReference = g_vsc.variableHolder.insertNewExpandableVariable(variable, /*isPermanent*/false);
+ }
+ variables.emplace_back(CreateVariable(variable, variableReference, variableReference != 0? variableReference: UINT64_MAX,
----------------
80 columns exceeded
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2954
+ }
+ variables.emplace_back(CreateVariable(variable, variableReference, variableReference != 0? variableReference: UINT64_MAX,
hex,
----------------
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2961
// children.
- const int64_t var_idx = VARREF_TO_VARIDX(variablesReference);
- lldb::SBValue variable = g_vsc.variables.GetValueAtIndex(var_idx);
+ lldb::SBValue variable = g_vsc.variableHolder.getVariableFromVariableReference(variablesReference);
if (variable.IsValid()) {
----------------
80 cols, use new LLDB var name for variablesReference
================
Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2970-2973
+ auto isPermanent = g_vsc.variableHolder.isPermanentVariableReference(variablesReference);
+ auto childVariablesReferences = g_vsc.variableHolder.insertNewExpandableVariable(child, isPermanent);
variables.emplace_back(
+ CreateVariable(child, childVariablesReferences, childVariablesReferences, hex));
----------------
80 columns, use lldb variable names
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105166/new/
https://reviews.llvm.org/D105166
More information about the lldb-commits
mailing list