Fix dap variable value format issue #90799
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
added 2 commits
May 1, 2024 16:47
|
@llvm/pr-subscribers-lldb Author: None (jeffreytan81) ChangesWhile adding a UI feature in VSCode to toggle hex/dec in variables view window. I noticed that it does not work after second toggle. Then I noticed that there is a bug that we only explicitly set hex format not reset back to default during further toggle. The new test demonstrates the bug. This PR resets the format back to default if not using hex. One complexity is that, we explicitly set registers value format to AddressInfo, which shouldn't be overridden by default or hex settings. Full diff: https://github.com/llvm/llvm-project/pull/90799.diff 3 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index 5838281bcb1a10..e2126d67a5fe77 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -448,7 +448,7 @@ def get_completions(self, text, frameId=None):
response = self.request_completions(text, frameId)
return response["body"]["targets"]
- def get_scope_variables(self, scope_name, frameIndex=0, threadId=None):
+ def get_scope_variables(self, scope_name, frameIndex=0, threadId=None, is_hex=None):
stackFrame = self.get_stackFrame(frameIndex=frameIndex, threadId=threadId)
if stackFrame is None:
return []
@@ -462,7 +462,7 @@ def get_scope_variables(self, scope_name, frameIndex=0, threadId=None):
for scope in frame_scopes:
if scope["name"] == scope_name:
varRef = scope["variablesReference"]
- variables_response = self.request_variables(varRef)
+ variables_response = self.request_variables(varRef, is_hex=is_hex)
if variables_response:
if "body" in variables_response:
body = variables_response["body"]
@@ -476,9 +476,9 @@ def get_global_variables(self, frameIndex=0, threadId=None):
"Globals", frameIndex=frameIndex, threadId=threadId
)
- def get_local_variables(self, frameIndex=0, threadId=None):
+ def get_local_variables(self, frameIndex=0, threadId=None, is_hex=None):
return self.get_scope_variables(
- "Locals", frameIndex=frameIndex, threadId=threadId
+ "Locals", frameIndex=frameIndex, threadId=threadId, is_hex=is_hex
)
def get_registers(self, frameIndex=0, threadId=None):
@@ -486,28 +486,32 @@ def get_registers(self, frameIndex=0, threadId=None):
"Registers", frameIndex=frameIndex, threadId=threadId
)
- def get_local_variable(self, name, frameIndex=0, threadId=None):
- locals = self.get_local_variables(frameIndex=frameIndex, threadId=threadId)
+ def get_local_variable(self, name, frameIndex=0, threadId=None, is_hex=None):
+ locals = self.get_local_variables(
+ frameIndex=frameIndex, threadId=threadId, is_hex=is_hex
+ )
for local in locals:
if "name" in local and local["name"] == name:
return local
return None
- def get_local_variable_value(self, name, frameIndex=0, threadId=None):
+ def get_local_variable_value(self, name, frameIndex=0, threadId=None, is_hex=None):
variable = self.get_local_variable(
- name, frameIndex=frameIndex, threadId=threadId
+ name, frameIndex=frameIndex, threadId=threadId, is_hex=is_hex
)
if variable and "value" in variable:
return variable["value"]
return None
- def get_local_variable_child(self, name, child_name, frameIndex=0, threadId=None):
+ def get_local_variable_child(
+ self, name, child_name, frameIndex=0, threadId=None, is_hex=None
+ ):
local = self.get_local_variable(name, frameIndex, threadId)
if local["variablesReference"] == 0:
return None
- children = self.request_variables(local["variablesReference"])["body"][
- "variables"
- ]
+ children = self.request_variables(local["variablesReference"], is_hex=is_hex)[
+ "body"
+ ]["variables"]
for child in children:
if child["name"] == child_name:
return child
@@ -1035,12 +1039,16 @@ def request_threads(self):
self.threads = None
return response
- def request_variables(self, variablesReference, start=None, count=None):
+ def request_variables(
+ self, variablesReference, start=None, count=None, is_hex=None
+ ):
args_dict = {"variablesReference": variablesReference}
if start is not None:
args_dict["start"] = start
if count is not None:
args_dict["count"] = count
+ if is_hex is not None:
+ args_dict["format"] = {"hex": is_hex}
command_dict = {
"command": "variables",
"type": "request",
diff --git a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
index d886d0776ce58b..ab7dfb5216ae19 100644
--- a/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
+++ b/lldb/test/API/tools/lldb-dap/variables/TestDAP_variables.py
@@ -754,3 +754,43 @@ def test_darwin_dwarf_missing_obj_with_symbol_ondemand_enabled(self):
"""
initCommands = ["settings set symbols.load-on-demand true"]
self.darwin_dwarf_missing_obj(initCommands)
+
+ @no_debug_info_test
+ @skipIfWindows
+ @skipIfRemote
+ def test_value_format(self):
+ """
+ Test that toggle variables value format between decimal and hexical works.
+ """
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(program)
+ source = "main.cpp"
+ breakpoint1_line = line_number(source, "// breakpoint 1")
+ lines = [breakpoint1_line]
+
+ breakpoint_ids = self.set_source_breakpoints(source, lines)
+ self.assertEqual(
+ len(breakpoint_ids), len(lines), "expect correct number of breakpoints"
+ )
+ self.continue_to_breakpoints(breakpoint_ids)
+
+ # Verify locals value format decimal
+ is_hex = False
+ var_pt_x = self.dap_server.get_local_variable_child("pt", "x", is_hex=is_hex)
+ self.assertEquals(var_pt_x["value"], "11")
+ var_pt_y = self.dap_server.get_local_variable_child("pt", "y", is_hex=is_hex)
+ self.assertEquals(var_pt_y["value"], "22")
+
+ # Verify locals value format hexical
+ is_hex = True
+ var_pt_x = self.dap_server.get_local_variable_child("pt", "x", is_hex=is_hex)
+ self.assertEquals(var_pt_x["value"], "0x0000000b")
+ var_pt_y = self.dap_server.get_local_variable_child("pt", "y", is_hex=is_hex)
+ self.assertEquals(var_pt_y["value"], "0x00000016")
+
+ # Toggle and verify locals value format decimal again
+ is_hex = False
+ var_pt_x = self.dap_server.get_local_variable_child("pt", "x", is_hex=is_hex)
+ self.assertEquals(var_pt_x["value"], "11")
+ var_pt_y = self.dap_server.get_local_variable_child("pt", "y", is_hex=is_hex)
+ self.assertEquals(var_pt_y["value"], "22")
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index b4a2718bbb096e..21798078f4d7fa 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -755,7 +755,6 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame) {
} else {
object.try_emplace("line", 0);
object.try_emplace("column", 0);
- object.try_emplace("presentationHint", "subtle");
}
const auto pc = frame.GetPC();
@@ -988,8 +987,13 @@ VariableDescription::VariableDescription(lldb::SBValue v, bool format_hex,
display_type_name =
!raw_display_type_name.empty() ? raw_display_type_name : NO_TYPENAME;
- if (format_hex)
- v.SetFormat(lldb::eFormatHex);
+ // Only format hex/default if there is no existing special format.
+ if (v.GetFormat() == lldb::eFormatDefault || v.GetFormat() == lldb::eFormatHex) {
+ if (format_hex)
+ v.SetFormat(lldb::eFormatHex);
+ else
+ v.SetFormat(lldb::eFormatDefault);
+ }
llvm::raw_string_ostream os_display_value(display_value);
|
walter-erquinigo
approved these changes
May 2, 2024
@walter-erquinigo , do you mean upstream to VSCode? Yeah, we would like to upstream the changes if Microsoft is willing to take them. |
|
I thought you were meaning UI changes via the fblldb extension, but if you are modifying your fork of VSCode, there's nothing to be done then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While adding a UI feature in VSCode to toggle hex/dec in variables view window. I noticed that it does not work after second toggle. Then I noticed that there is a bug that we only explicitly set hex format not reset back to default during further toggle. The new test demonstrates the bug.
This PR resets the format back to default if not using hex. One complexity is that, we explicitly set registers value format to AddressInfo, which shouldn't be overridden by default or hex settings.