[Debug Info] replace lldb_commands with __lldb_init_module#155336
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
now that i think on it, apple's LLDB is a custom fork. I don't think the version extraction logic will work for it. Lemme fix that real quick. |
This comment has been minimized.
This comment has been minimized.
|
You should be able to run try jobs on this PR urself after this: |
|
✌️ @Walnut356, you can now perform try builds on this pull request! You can now post |
e4db5e7 to
e8cc4eb
Compare
|
rad, thank you |
This comment has been minimized.
This comment has been minimized.
e8cc4eb to
40198c7
Compare
|
@bors try jobs=aarch64-apple |
[Debug Info] replace `lldb_commands` with `__lldb_init_module` try-job: aarch64-apple
This comment has been minimized.
This comment has been minimized.
|
💔 Test for d52c8f9 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
|
Okay, bad news is this test didn't go exactly as planned because
i'm still iffy on whether I want to just update the tests, or if i want to force a |
|
I guess I should also note that, in additional bad news, apples versioning scheme seems to have 0 relation to llvm's, so using raw version numbers probably won't work. I should be able to just check for features via |
This comment has been minimized.
This comment has been minimized.
…youxu Account for `GetSyntheticValue` failures `GetSyntheticValue` returns an invalid `SBValue` if no synthetic is present. That wasn't a problem before when we were attaching synthetics to every type, but it won't be the case once github.com/rust-lang/pull/155336 or similar lands. Additionally, codelldb subverts `lldb_commands` to apply similar behavior that doesn't attach synthetics to every type, so this fixes a regression there too. Additionally, I removed 1 useless instance of `GetSyntheticValue`, since pointers should always be `IndirectionSyntheticProvider`, not `DefaultSyntheticProvider`.
I think i'm just gonna update the tests for now since it's a bit safer. I can always go in and enforce |
|
Nevermind, I'm really not fond of the formatting inconsistency in nested structs. |
|
(Will try to get to this next Monday or so) |
This comment has been minimized.
This comment has been minimized.
c0b1826 to
3b90f74
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
There was a problem hiding this comment.
This largely looks good to me, and I also tried this locally. I do like the __lldb_init_module approach much better than the plain-text lldb_commands which was a pain to read let alone write.
Only one thing with tests/debuginfo/captured-fields-1.rs, where I think the apple lldb used in aarch64-apple is borked on this test case.
@rustbot author
There was a problem hiding this comment.
Review notes:
- I tested the changes locally on a
aarch64-apple-darwinhost w/:- Apple lldb 2100
- LLVM lldb 22.1.6
There was a problem hiding this comment.
Review notes: see https://lldb.llvm.org/use/tutorials/writing-custom-commands.html#loading-commands for __lldb_init_module
| //@ lldb-command:continue | ||
| //@ lldb-command:v test | ||
| //@ lldb-check:(captured_fields_1::main::{closure_env#2}) test = { _ref__my_ref = 0x[...] } | ||
| //@ lldb-check:[...] = {_ref__my_ref:{*_ref__my_ref:{my_field1:11, my_field2:22}}} |
There was a problem hiding this comment.
Problem: on llvm lldb 22 locally (aarch64-apple-darwin host), this check is failing for me.
LLVM lldb 22
lldb version 22.1.6
v test
(captured_fields_1::main::{closure_env#0}) test = {_ref__my_ref__my_field1:0x000000016fdfe520}
continue
v test
(captured_fields_1::main::{closure_env#1}) test = {_ref__my_ref__my_field2:0x000000016fdfe524}
continue
v test
(captured_fields_1::main::{closure_env#2}) test = {_ref__my_ref:0x000000016fdfe528}
continue
v test
(captured_fields_1::main::{closure_env#3}) test = {my_ref:{my_field1:11, my_field2:22}}
continue
v test
(captured_fields_1::main::{closure_env#4}) test = {my_var__my_field2:22}
continue
v test
(captured_fields_1::main::{closure_env#5}) test = {my_var:{my_field1:11, my_field2:22}}
continue
quit
Apple lldb 2100
lldb-2100.0.17.108
Apple Swift version 6.3.2 (swiftlang-6.3.2.1.108 clang-2100.1.1.101)
v test
(captured_fields_1::main::{closure_env#0}) test = {_ref__my_ref__my_field1:0x000000016fdfe450}
continue
v test
(captured_fields_1::main::{closure_env#1}) test = {_ref__my_ref__my_field2:0x000000016fdfe454}
continue
v test
(captured_fields_1::main::{closure_env#2}) test = {_ref__my_ref:0x000000016fdfe458}
continue
v test
(captured_fields_1::main::{closure_env#3}) test = {my_ref:{my_field1:11, my_field2:22}}
continue
v test
(captured_fields_1::main::{closure_env#4}) test = {my_var__my_field2:22}
continue
v test
(captured_fields_1::main::{closure_env#5}) test = {my_var:{my_field1:11, my_field2:22}}
continue
quit
There was a problem hiding this comment.
IOW something seems broken with this with the lldb that aarch64-apple CI has (LLDB version 1703) 🤔 I think that is a bug with an older Apple lldb version.
I would suggest gating this with minimum lldb version 21 (or 2100 apple lldb...), or we can look into bumping the lldb that aarch64-apple is using, I suppose 🤔
There was a problem hiding this comment.
It looks like the macos-15 runner in our aarch64-apple CI currently has Apple LLVM 17 as the baseline... It can become Apple LLVM 21 baseline if we can figure out how to bump to macos-26 without having that exceed the 6h timeout... 🤦:
There was a problem hiding this comment.
huh, fair enough. I'm almost tempted to figure out what actually changed in the printing logic, but that whole submodule of LLDB is really obtuse and i don't think I have it in me to debug LLDB atm.
At the very least, the old and new outputs both happen to be acceptable ways of visualizing the struct from an end-user perspective
There was a problem hiding this comment.
Yes. And I'm not so worried about this particular case because Apple lldb 2100+ seems to not, you know, give up on this case (and actually seems to converge back with whatever formatting LLVM lldb 22 gives). It's the Apple lldb 17xx series that seem to have this problem.
|
Reminder, once the PR becomes ready for a review, use |
3b90f74 to
7f84e5c
Compare
|
✌️ @Walnut356, you can now approve this pull request! If @jieyouxu told you to " |
|
@bors r=jieyouxu |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 4530eac (parent) -> 4804ad7 (this PR) Test differencesShow 5 test diffsStage 2
Additionally, 4 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 4804ad7e93e1b31f4605b7083871d0d3d85a2afe --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (4804ad7): comparison URL. Overall result: ❌✅ regressions and improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (primary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 510.437s -> 509.892s (-0.11%) |
Use alternate means of detecting enums in `is_udt` This fixes a small regression from rust-lang#155336 Flat enums are excluded from `is_udt` since LLDB natively handles them correctly. The previous logic (`SBType.IsScopedEnumerationType()`) behaved correctly for non-msvc targets, but for some reason on msvc enums don't count as scoped enumerations? The new logic checks the type returned by `SBType.GetEnumerationIntegerType()`. If the queried type isn't an enum, the returned type is invalid. This behaves correctly on both msvc and non-msvc targets, and its behavior doesn't conflict with sum-type enums. As always, testing on msvc isn't really possible atm. That should change soon though =) --- try-job: aarch64-apple
Rollup merge of #157298 - Walnut356:msvc_enum, r=jieyouxu Use alternate means of detecting enums in `is_udt` This fixes a small regression from #155336 Flat enums are excluded from `is_udt` since LLDB natively handles them correctly. The previous logic (`SBType.IsScopedEnumerationType()`) behaved correctly for non-msvc targets, but for some reason on msvc enums don't count as scoped enumerations? The new logic checks the type returned by `SBType.GetEnumerationIntegerType()`. If the queried type isn't an enum, the returned type is invalid. This behaves correctly on both msvc and non-msvc targets, and its behavior doesn't conflict with sum-type enums. As always, testing on msvc isn't really possible atm. That should change soon though =) --- try-job: aarch64-apple
Use alternate means of detecting enums in `is_udt` This fixes a small regression from rust-lang/rust#155336 Flat enums are excluded from `is_udt` since LLDB natively handles them correctly. The previous logic (`SBType.IsScopedEnumerationType()`) behaved correctly for non-msvc targets, but for some reason on msvc enums don't count as scoped enumerations? The new logic checks the type returned by `SBType.GetEnumerationIntegerType()`. If the queried type isn't an enum, the returned type is invalid. This behaves correctly on both msvc and non-msvc targets, and its behavior doesn't conflict with sum-type enums. As always, testing on msvc isn't really possible atm. That should change soon though =) --- try-job: aarch64-apple
View all comments
DO NOT MERGE. Please run throughaarch64-apple. Ideally the tests fail. If they don't, the test runner is using too old of a version of LLDB and I need to fiddle with this some more.TL;DR I hate
lldb_commands, it's annoying to update and maintain since it's just a raw text file of CLI commands. All of the functionality can be replicated via the debugger's own API.__lldb_init_moduleis called immediately uponcommand script import lldb_lookup.py, so it should function identically (with one exception, see below). We can do things like let python tell us the names of the functions and classes, making them easier to keep in sync. Most importantly, we can conditionally use one set of visualizers or another depending on the version of the debugger. This is a precursor to using callback-based type matching, which was added in LLDB 19.The one exception i mentioned above replaces our
".*"regex that dumps everything throughlldb_lookup.synthetic_lookup. That wildcard also includes primitives which is less than ideal, especially for performance. It's bad enough that codelldb intercepts this command and replaces it with a type recognizer. If i removelldb_commands, codelldb can no longer do this. To prevent a regression on their end, I added a conditional that replicates that behavior.Here's the problem: for some godforsaken reason, if a struct contains a field with a
SyntheticProvider(even one that is "empty", like forDefaultSyntheticProviderfor primitives) it is formatted differently than a struct whose fields don't have providers. That breaks a bunch of the tests because instead of{ x = 10 y = 30 }we get(x = 10, y = 30). Very cool.So, we have divergent behavior depending on the version of LLDB. That's nothing new, since MSVC enums are broken prior to LLDB 18, so it's whatever (and the difference hardly matters to the end user). If the CI test runners have a new enough version of LLDB that it uses the type recognizer instead of the wildcard, the tests will fail. I'll update them and we'll be good to go. If they don't fail, I need to probably force a struct summary provider so that all versions look the same (which isn't ideal performance), or I can just wait until xcode happens to update to a newer version of LLDB.
try-job: aarch64-apple