[lldb-dap] Show assembly depending on `stop-disassembly-display` settings #136494

eronnen · 2025-04-20T16:03:19Z

Show assembly code when the source code for a frame is not available in the debugger machine

Edit: this functionality will work only when using stop-disassembly-display = no-source in the settings

Fix #136492

After the fix:

Screencast.From.2025-04-20.18-00-30.webm

llvmbot · 2025-04-20T16:03:53Z

@llvm/pr-subscribers-lldb

Author: Ely Ronnen (eronnen)

Changes

Show assembly code when the source code for a frame is not available in the debugger machine

Fix #136492

After the fix:

Screencast.From.2025-04-20.18-00-30.webm

Full diff: https://github.com/llvm/llvm-project/pull/136494.diff

1 Files Affected:

(modified) lldb/tools/lldb-dap/JSONUtils.cpp (+3-1)

diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 33f10c93d2ada..5b647950cfc6e 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -750,9 +750,11 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame,
   EmplaceSafeString(object, "name", frame_name);
 
   auto line_entry = frame.GetLineEntry();
+  auto file_spec = line_entry.GetFileSpec();
   // A line entry of 0 indicates the line is compiler generated i.e. no source
   // file is associated with the frame.
-  if (line_entry.GetFileSpec().IsValid() &&
+  if (file_spec.IsValid() &&
+      file_spec.Exists() &&
       (line_entry.GetLine() != 0 ||
        line_entry.GetLine() != LLDB_INVALID_LINE_NUMBER)) {
     object.try_emplace("source", CreateSource(line_entry));

github-actions · 2025-04-20T16:05:40Z

✅ With the latest revision this PR passed the C/C++ code formatter.

JDevlieghere

The change LGTM but please add a test. You should be able to remove the source file between building and launching LLDB. I believe there are some examples of that in the LLDB test suite.

jasonmolenda · 2025-04-20T17:42:38Z

Maybe I'm missing something, but I disagree with this change. We're talking about when you're stopped in a stack frame that has debug info (a file and line number), but the source file is unavailable, right. There is a setting to control this behavior already -- stop-disassembly-display. It defaults to no-debuginfo, but it can be never, always, no-source, or no-debuginfo.

People who are debugging at a source level, generally, do not want to see assembly, and in fact will be confused if they are shown assembly. We do not want to show assembly for a stack frame where we have a debug filename and line number, by default. The user may not have set up the target.source-map to map lldb to the source file, and could be looking at it in another source window as they're stepping, or they may know roughly how many lines a method has as they step through it. The assembly only adds distraction to them.

Am I misunderstanding what we're talking about here?

eronnen · 2025-04-20T17:51:17Z

@jasonmolenda fair enough, but still as a user who would like to see assembly instead of an error when the source is not available I don't have any solution currently. If I were to check the stop-disassembly-display value when returning the source info, would that be acceptable?

jasonmolenda · 2025-04-20T18:24:42Z

One side note, I was commenting while not 100% awake and didn't realize this was only in the DAP side of the codebase, I have less weight to argue one way or the other there. After I posted that I was like, I wonder why I didn't see the stop-assembly-display preference used around that diff. :)

My comment is much weaker when it's only impacting the DAP replies, fwiw. I think from a UI perspective defaulting to not showing assembly when we have filename & line number is the best tradeoff for users, but like most UI things it's debatable. I think using stop-assembly-display here would be a good thing for sure. If people think that the DAP front end should default to showing assembly in this case, I don't have a strong opinion on that, I haven't lived on those IDEs myself and I don't feel qualified to say anything.

jasonmolenda · 2025-04-20T18:50:42Z

Just to be super clear, if we were talking about core lldb behavior, I would be insistent about how it behaves. But we're talking about DAP and i'm just a bystander on that part of the codebase, and my feedback is just an opinion that you might want to consider, but don't feel like you need to address my point.

github-actions · 2025-04-20T21:02:22Z

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

eronnen · 2025-04-20T21:11:17Z

I refactored the code now to use the stop-assembly-display setting, I think it makes sense to use it especially since it supports the no-source value which was what I was looking for

eronnen · 2025-04-20T21:22:41Z

Maybe this setting should be exposed in the vscode launch configuration so it will be more findable for users though

JDevlieghere · 2025-04-21T19:52:47Z

lldb/tools/lldb-dap/LLDBUtils.cpp

+  debugger.GetCommandInterpreter().HandleCommand(
+      "settings show stop-disassembly-display", result);
+  if (result.Succeeded()) {
+    std::string output = result.GetOutput();
+    size_t pos = output.find("stop-disassembly-display");
+    if (pos != std::string::npos) {
+      size_t start = output.find("= ", pos) + 2;
+      size_t end = output.find("\n", start);
+      stop_disassembly_display =
+          output.substr(start, end - start); // trim whitespace
+    }
+  }


Can this use SBDebugger::GetSetting?

Changed it to SBDebugger::GetSetting now, apparently OptionValueEnumeration::ToJSON implementation was missing

created a pull request. I made the change for some time but forgot to push it.

#137007

JDevlieghere · 2025-04-23T22:17:56Z

lldb/source/Interpreter/OptionValueEnumeration.cpp

+    if (m_enumerations.GetValueAtIndexUnchecked(i).value == m_current_value) {
+      return m_enumerations.GetCStringAtIndex(i).GetStringRef();
+    }


Suggested change

if (m_enumerations.GetValueAtIndexUnchecked(i).value == m_current_value) {

return m_enumerations.GetCStringAtIndex(i).GetStringRef();

}

if (m_enumerations.GetValueAtIndexUnchecked(i).value == m_current_value)

return m_enumerations.GetCStringAtIndex(i).GetStringRef();

JDevlieghere · 2025-04-23T22:24:59Z

lldb/tools/lldb-dap/DAP.h

+  /// The value of stop-disassembly-display setting in LLDB.
+  std::string stop_disassembly_display;


I'm wondering if we should be caching this. Querying the debugger for a setting should be cheap enough that we could do it whenever we need to know. The added benefit is that you'll be able to change the setting and have it take immediate effect, while with the current approach you'd have to create new debug session.

Personally I'm leaning toward computing this once per request.

Agree, changed it

JDevlieghere · 2025-04-23T22:30:54Z

lldb/tools/lldb-dap/LLDBUtils.cpp

+    return result_string;
+  }
+
+  return "no-debuginfo";


I don't like hard-coding a default value here. If for whatever reason this ever were to change, that person needs to remember to do the same thing here. Instead, could we make this return a std::optional<std::string>.

Even better, instead of having this return a string, I would create a new enum for this setting in lldb-dap and have this return an enum value instead of a string. If we move the enum from Debugger.h into lldb-enumerations.h we can parse it with an llvm::StringSwitch and reuse it across lldb-dap.

enum StopDisassemblyType { eStopDisassemblyTypeNever = 0, eStopDisassemblyTypeNoDebugInfo, eStopDisassemblyTypeNoSource, eStopDisassemblyTypeAlways };

I guess that would still require a default value, but at least it's easier to grep for the enum value than for the corresponding string.

not sure I understand the suggestion, do you mean having an alternative setting in the lldb-dap launch configuration and use it instead of debugger.GetSetting, or just export the existing StopDisassemblyType enum and convert the current GetSetting to use it?

anyway I moved the enum to lldb-enumerations.h

JDevlieghere

This is exactly what I had in mind, thank you! This LGTM modulo the nit.

JDevlieghere · 2025-04-24T03:22:24Z

lldb/include/lldb/lldb-enumerations.h

@@ -1383,6 +1383,14 @@ enum CommandReturnObjectCallbackResult {
  eCommandReturnObjectPrintCallbackHandled = 1,
 };

+// Used to determine when to show disassembly


Suggested change

// Used to determine when to show disassembly

/// Used to determine when to show disassembly.

eronnen · 2025-04-24T07:05:21Z

@JDevlieghere Fixed comments and conflicts with main, from my part it's ready to merge

...test/API/tools/lldb-dap/stackTraceDisassemblyDisplay/TestDAP_stackTraceDisassemblyDisplay.py

lldb/test/API/tools/lldb-dap/stackTraceDisassemblyDisplay/main.c

eronnen · 2025-04-24T19:52:27Z

@da-viper Fixed your comments, also now that the stop-disassembly-dispaly setting is not cached I combined the tests to one test and verified that the assembly changes mid-run according to the settings

da-viper · 2025-04-24T21:43:13Z

lldb/test/API/tools/lldb-dap/stackTraceDisassemblyDisplay/main.c

@@ -0,0 +1,8 @@
+#include <stdint.h>


don't need the header

Suggested change

#include <stdint.h>

da-viper · 2025-04-24T21:43:23Z

LGTM

eronnen · 2025-04-25T14:50:24Z

@JDevlieghere any chance to merge?

JDevlieghere · 2025-04-25T16:41:39Z

@JDevlieghere any chance to merge?

There's a conflict in StackTraceRequestHandler.cpp, let me know once you've resolved that and I'm happy to go ahead and merge this.

fix TestDAP_coreFile.py with source maps use stop-disassembly-display setting to determine when to show disassembly * Use GetSetting instead of Handle command * Implement OptionValueEnumeration::ToJSON export StopDisassemblyType enum fix comment cleaner stackTraceDisassemblyDisplay tests remove include in main.c in test

JDevlieghere · 2025-04-25T21:30:07Z

@eronnen Let me know if you'd like me to merge this now or wait for #137382.

eronnen · 2025-04-25T21:32:21Z

@JDevlieghere You can merge :)

…ings (llvm#136494) Show assembly code when the source code for a frame is not available in the debugger machine Edit: this functionality will work only when using `stop-disassembly-display = no-source` in the settings Fix llvm#136492 After the fix: [Screencast From 2025-04-20 18-00-30.webm](https://github.com/user-attachments/assets/1ce41715-cf4f-42a1-8f5c-6196b9d685dc)

eronnen requested a review from JDevlieghere as a code owner April 20, 2025 16:03

llvmbot added lldb lldb-dap labels Apr 20, 2025

eronnen mentioned this pull request Apr 20, 2025

[lldb-dap] Debugging with no source code available #136492

Closed

eronnen force-pushed the lldb-dap-disassemble-unavailable-source branch from f9d6bcd to c7da2d8 Compare April 20, 2025 16:07

JDevlieghere reviewed Apr 20, 2025

View reviewed changes

eronnen force-pushed the lldb-dap-disassemble-unavailable-source branch from c7da2d8 to d2062b8 Compare April 20, 2025 21:00

eronnen force-pushed the lldb-dap-disassemble-unavailable-source branch from d2062b8 to dd8735e Compare April 20, 2025 21:08

eronnen requested a review from JDevlieghere April 20, 2025 21:13

eronnen changed the title ~~fallback to assembly when source code is not available~~ [lldb-dap] Show assembly depending on stop-disassembly-display settings Apr 21, 2025

JDevlieghere approved these changes Apr 21, 2025

View reviewed changes

eronnen force-pushed the lldb-dap-disassemble-unavailable-source branch from dd8735e to cd8941d Compare April 21, 2025 21:14

JDevlieghere reviewed Apr 23, 2025

View reviewed changes

eronnen force-pushed the lldb-dap-disassemble-unavailable-source branch 2 times, most recently from 326bdd3 to 2ec7e34 Compare April 24, 2025 01:06

JDevlieghere approved these changes Apr 24, 2025

View reviewed changes

eronnen force-pushed the lldb-dap-disassemble-unavailable-source branch 2 times, most recently from f13a16f to 19dc107 Compare April 24, 2025 07:04

da-viper reviewed Apr 24, 2025

View reviewed changes

da-viper approved these changes Apr 24, 2025

View reviewed changes

eronnen force-pushed the lldb-dap-disassemble-unavailable-source branch from c9dd464 to caf6db4 Compare April 25, 2025 18:37

eronnen mentioned this pull request Apr 25, 2025

Request Commit Access For eronnen #137382

Closed

eronnen force-pushed the lldb-dap-disassemble-unavailable-source branch from caf6db4 to 4061321 Compare April 25, 2025 21:28

JDevlieghere merged commit 290ba28 into llvm:main Apr 25, 2025
7 of 9 checks passed

		/// The value of stop-disassembly-display setting in LLDB.
		std::string stop_disassembly_display;

	// Used to determine when to show disassembly
	/// Used to determine when to show disassembly.

[lldb-dap] Show assembly depending on stop-disassembly-display settings #136494

[lldb-dap] Show assembly depending on stop-disassembly-display settings #136494

Uh oh!

Conversation

eronnen commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 20, 2025

Uh oh!

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

Uh oh!

Uh oh!

JDevlieghere left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmolenda commented Apr 20, 2025

Uh oh!

eronnen commented Apr 20, 2025

Uh oh!

jasonmolenda commented Apr 20, 2025

Uh oh!

jasonmolenda commented Apr 20, 2025

Uh oh!

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

Uh oh!

Uh oh!

eronnen commented Apr 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eronnen commented Apr 20, 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!

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!

JDevlieghere left a comment

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!

eronnen commented Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eronnen commented Apr 24, 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!

da-viper commented Apr 24, 2025

Uh oh!

eronnen commented Apr 25, 2025

Uh oh!

JDevlieghere commented Apr 25, 2025

Uh oh!

JDevlieghere commented Apr 25, 2025

Uh oh!

eronnen commented Apr 25, 2025

Uh oh!

Uh oh!

[lldb-dap] Show assembly depending on `stop-disassembly-display` settings #136494

[lldb-dap] Show assembly depending on `stop-disassembly-display` settings #136494

eronnen commented Apr 20, 2025 •

edited

Loading

github-actions bot commented Apr 20, 2025 •

edited

Loading

github-actions bot commented Apr 20, 2025 •

edited

Loading

eronnen commented Apr 20, 2025 •

edited

Loading

eronnen commented Apr 24, 2025 •

edited

Loading