-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[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
Conversation
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesFull diff: https://github.com/llvm/llvm-project/pull/147887.diff 2 Files Affected:
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
|
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. |
I addressed |
FWIW, the |
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.
✅ With the latest revision this PR passed the C/C++ code formatter. |
I think this PR is ready to be reviewed again. Please take a look when you have a few minutes. :-) |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤞
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... |
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 |
There was a problem hiding this comment.
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.
Yes that's perfect, didn't see that it was already there. |
A post-commit review on PR llvm#147887 requested a minor update to the formatting of the LLDB DIL implementation release note.
A post-commit review on PR #147887 requested a minor update to the formatting of the LLDB DIL implementation release note.
I think so too, this and #149117. Do we just create a PR with the same changes into |
No description provided.