[lldb-dap] Include [opt] in the frame name only if a custom frame format is not specified.

[walter-erquinigo]

Currently there's an include in which [opt] might be emitted twice if the frame format also asks for it. As a trivial fix, we should manually emit [opt] only if a custom frame format is not specified.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Walter Erquinigo (walter-erquinigo)

Changes

Currently there's an include in which [opt] might be emitted twice if the frame format also asks for it. As a trivial fix, we should manually emit [opt] only if a custom frame format is not specified.

---

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

1 Files Affected:

• (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+4-2)

diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 03a43f9da87f24..e55a69ecd0e9f2 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -803,9 +803,11 @@ llvm::json::Value CreateStackFrame(lldb::SBFrame &frame) {
     llvm::raw_string_ostream os(frame_name);
     os << llvm::format_hex(frame.GetPC(), 18);
   }
-  bool is_optimized = frame.GetFunction().GetIsOptimized();
-  if (is_optimized)
+
+  // We only include `[opt]` if a custom frame format is not specified.
+  if (!g_dap.frame_format && frame.GetFunction().GetIsOptimized())
     frame_name += " [opt]";
+
   EmplaceSafeString(object, "name", frame_name);
 
   auto source = CreateSource(frame);

[bulbazord]

I think conceptually this makes sense, but I somewhat wonder if folks would get confused when they have a custom frame format and don't see the [opt] in there? The current behavior is that [opt] is always there so folks know they don't have to put it in their custom frame format. When it's missing after this change, I wonder if they'll notice.

[jimingham]

Would it make sense to have opt be the result of a frame-format token, which we could put in the default format (function.optimization?) and people could add or not in custom formats? It seems weird that we're special casing this one. Jim

…

On Dec 8, 2023, at 3:59 PM, Alex Langford ***@***.***> wrote: @bulbazord commented on this pull request. I think conceptually this makes sense, but I somewhat wonder if folks would get confused when they have a custom frame format and don't see the [opt] in there? The current behavior is that [opt] is always there so folks know they don't have to put it in their custom frame format. When it's missing after this change, I wonder if they'll notice. — Reply to this email directly, view it on GitHub <#74861 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW47YIOI3CWTY6ULIJDYIOSXTAVCNFSM6AAAAABAM4WLDCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZTGMYTONBTGY>. You are receiving this because you are on a team that was mentioned.

[clayborg]

{${function.is-optimized} [opt]} is how is already is and can be done with frame formats, so I agree with this patch that if the user customizes it, they have complete control and we shouldn't add anything. Does everyone else agree?

[bulbazord]

{${function.is-optimized} [opt]} is how is already is and can be done with frame formats, so I agree with this patch that if the user customizes it, they have complete control and we shouldn't add anything. Does everyone else agree?

I think I agree with this in principle -- this will give users complete control over the format of their frame, like frame formats were made to do. This change is breaking in the sense that folks who forgot to put is-optimized or is using a frame format they did not create (without is-optimized) will no longer know that they're debugging optimized code. I think this an acceptable trade-off.