Skip to content

[LLDB] Switch to using DIL as default implementation for 'frame var'. #147887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 15, 2025

Conversation

cmtice
Copy link
Contributor

@cmtice cmtice commented Jul 10, 2025

No description provided.

@cmtice cmtice requested a review from JDevlieghere as a code owner July 10, 2025 04:40
@llvmbot llvmbot added the lldb label Jul 10, 2025
@cmtice cmtice requested a review from labath July 10, 2025 04:41
@llvmbot
Copy link
Member

llvmbot commented Jul 10, 2025

@llvm/pr-subscribers-lldb

Author: None (cmtice)

Changes

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

2 Files Affected:

  • (modified) lldb/source/Target/TargetProperties.td (+2-2)
  • (modified) llvm/docs/ReleaseNotes.md (+8)
diff --git a/lldb/source/Target/TargetProperties.td b/lldb/source/Target/TargetProperties.td
index 4aa9e046d6077..656503bb8d228 100644
--- a/lldb/source/Target/TargetProperties.td
+++ b/lldb/source/Target/TargetProperties.td
@@ -5,8 +5,8 @@ let Definition = "target_experimental" in {
     Global, DefaultTrue,
     Desc<"If true, inject local variables explicitly into the expression text. This will fix symbol resolution when there are name collisions between ivars and local variables. But it can make expressions run much more slowly.">;
   def UseDIL : Property<"use-DIL", "Boolean">,
-    Global, DefaultFalse,
-    Desc<"If true, use the alternative DIL implementation for frame variable evaluation.">;
+    Global, DefaultTrue,
+    Desc<"If true, use the DIL implementation for frame variable evaluation.">;
 }
 
 let Definition = "target" in {
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index daf822388a2ff..5d2146b7f2f75 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -306,6 +306,14 @@ Changes to LLDB
     stop reason = SIGSEGV: sent by tkill system call (sender pid=649752, uid=2667987)
   ```
 * ELF Cores can now have their siginfo structures inspected using `thread siginfo`.
+* LLDB now uses
+  [DIL](https://discourse.llvm.org/t/rfc-data-inspection-language/69893) as the
+  default implementation for 'frame variable'. This should not change the
+  behavior of 'frame variable' at all, at this time. To revert to using the
+  old implementation use
+  ```
+     settings set target.experimental.use-DIL false
+   ```
 
 ### Changes to lldb-dap
 

@cmtice cmtice requested review from jimingham, kuilpd and asl July 10, 2025 04:41
@labath
Copy link
Collaborator

labath commented Jul 10, 2025

There are a couple of failures in the CI. The backtraces don't make a whole lot sense, but it looks like there's something wrong with retrieving the list of variables from a CU.

@kuilpd
Copy link
Contributor

kuilpd commented Jul 10, 2025

I addressed lldb-shell-subprocess tests failing in #147955 , but there's still 2 data formatter tests failing, not sure what's happening there yet.

@Michael137
Copy link
Member

but there's still 2 data formatter tests failing, not sure what's happening there yet.

FWIW, the object child of a std::shared_ptr should be equivalent to dereferencing it. How does DIL handle the $$dereference$$ operator? Does it do anything special?

@cmtice
Copy link
Contributor Author

cmtice commented Jul 11, 2025

but there's still 2 data formatter tests failing, not sure what's happening there yet.

FWIW, the object child of a std::shared_ptr should be equivalent to dereferencing it. How does DIL handle the $$dereference$$ operator? Does it do anything special?

DIL handles it exactly the same way the current 'frame variable' handles it, namely it uses the provided synthetic children and data formatters. (Gets the synthetic value for the shared pointer, then calls 'GetChildMemberWithName("object")', which returns the correct thing.

 - Remove not-always-valid field-name check when evaluating field members.
 - Fix unexpected passes in TestDAP_evaluate (test owner said it was ok).
 - Add more anonymous namespace tests.
Copy link

github-actions bot commented Jul 11, 2025

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

@cmtice
Copy link
Contributor Author

cmtice commented Jul 11, 2025

I think this PR is ready to be reviewed again. Please take a look when you have a few minutes. :-)

@labath
Copy link
Collaborator

labath commented Jul 14, 2025

I gave this patch a spin on a mac. The only failure I got was in test/Shell/SymbolFile/DWARF/TestDedupWarnings.test, which fails it does not generate both of the warnings, and that's because the "p" (aka dwim-print) now resolves to "frame var" instead of expr". Changing the test to use "expr a/b" instead should fix it.

@cmtice
Copy link
Contributor Author

cmtice commented Jul 14, 2025

I gave this patch a spin on a mac. The only failure I got was in test/Shell/SymbolFile/DWARF/TestDedupWarnings.test, which fails it does not generate both of the warnings, and that's because the "p" (aka dwim-print) now resolves to "frame var" instead of expr". Changing the test to use "expr a/b" instead should fix it.

Done.

Copy link
Collaborator

@labath labath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤞

@cmtice cmtice merged commit f5c676d into llvm:main Jul 15, 2025
10 checks passed
@labath
Copy link
Collaborator

labath commented Jul 16, 2025

It looks like this just barely didn't make it into the 21.x release branch. Do we want to cherry-pick that over? I think it would be a good way to flush out any issues...

@DavidSpickett
Copy link
Collaborator

Perhaps with a release note saying this happened and how to switch back to the non-default way?

@cmtice
Copy link
Contributor Author

cmtice commented Jul 16, 2025

Perhaps with a release note saying this happened and how to switch back to the non-default way?

This PR updates the release notes; is that not good enough?

[DIL](https://discourse.llvm.org/t/rfc-data-inspection-language/69893) as the
default implementation for 'frame variable'. This should not change the
behavior of 'frame variable' at all, at this time. To revert to using the
old implementation use
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a : on the end here, or put the command on the same line, so it's one sentence.

@DavidSpickett
Copy link
Collaborator

This PR updates the release notes; is that not good enough?

Yes that's perfect, didn't see that it was already there.

cmtice added a commit to cmtice/llvm-project that referenced this pull request Jul 16, 2025
A post-commit review on PR llvm#147887 requested a minor update
to the formatting of the LLDB DIL implementation release note.
cmtice added a commit that referenced this pull request Jul 16, 2025
A post-commit review on PR #147887 requested a minor update to the
formatting of the LLDB DIL implementation release note.
@kuilpd
Copy link
Contributor

kuilpd commented Jul 17, 2025

It looks like this just barely didn't make it into the 21.x release branch. Do we want to cherry-pick that over? I think it would be a good way to flush out any issues...

I think so too, this and #149117. Do we just create a PR with the same changes into release/21.x branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants