rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
93.58k stars 12.05k forks source link

Implement lldb formatter for "clang encoded" enums (LLDB 18.1+) (V3) #124781

Closed VladimirMakaev closed 1 week ago

VladimirMakaev commented 1 week ago

This is a redo of PR (#124458) which was approved previously but force-pushed out. Then a V2 (#124745) failed debuginfo\msvc-pretty-enums.rs test during merge.

I've fixed the test and checked it to pass on Windows with .\x.ps1 test .\tests\debuginfo\msvc-pretty-enums.rs

Below is the original summary:

Summary:

fixes #79530

I landed a fix last year to enable DW_TAG_variant_part encoding in LLDBs (https://reviews.llvm.org/D149213). This PR is a corresponding fix in synthetic formatters to decode that information.

This is in no way perfect implementation but at least it improves the status quo. But most types of enums will be visible and debuggable in some way.

I've also updated most of the existing tests that touch enums and re-enabled test cases based on LLDB for enums.

Test Plan:

ran tests ./x test tests/debuginfo/. Also tested manually in LLDB CLI and LLDB VSCode

Other Thoughs:

A better approach would probably be adopting formatters from codelldb. There is some neat hack that hooks up summary provider via synthetic provider which can ultimately fix more display issues for Rust types and enums too. But getting it to work well might take more time that I have right now.

rustbot commented 1 week ago

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

dtolnay commented 1 week ago

This is the diff from v1:

diff --git a/tests/debuginfo/coroutine-objects.rs b/tests/debuginfo/coroutine-objects.rs
@@ -27,7 +27,7 @@

 // lldb-command:run
 // lldb-command:v b
-// lldb-check:(coroutine_objects::main::{coroutine_env#0}) b = { value = { _ref__a = 0x[...] } $discr$ = ',' }
+// lldb-check:(coroutine_objects::main::{coroutine_env#0}) b = { value = { _ref__a = 0x[...] } $discr$ = [...] }

 // === CDB TESTS ===================================================================================
diff --git a/tests/debuginfo/msvc-pretty-enums.rs b/tests/debuginfo/msvc-pretty-enums.rs
@@ -3,8 +3,8 @@
 //@ compile-flags:-g

 // === LLDB TESTS ==================================================================================
-
 // lldb-command:run
+
 // lldb-command:v a
 // lldbg-check:(core::option::Option<msvc_pretty_enums::CStyleEnum>) a = { value = { 0 = Low } }

@@ -74,12 +74,7 @@
 // lldbg-check:(msvc_pretty_enums::ArbitraryDiscr) arbitrary_discr2 = { value = { 0 = 5678 } $discr$ = 5000000 }

 // === CDB TESTS ==================================================================================
-// cdb-command: g
-//
-// cdb-command: dx a
-// cdb-check:a                : Some [Type: enum2$<core::option::Option<msvc_pretty_enums::CStyleEnum> >]
-// cdb-check:    [+0x000] __0              : Low (0x2) [Type: msvc_pretty_enums::CStyleEnum]
-//
+
 // cdb-command: g
 //
 // cdb-command: dx a
dtolnay commented 1 week ago

@bors r+

bors commented 1 week ago

:pushpin: Commit fb2d9cdfc54aea306223992e13ce4b2278d9f0c6 has been approved by dtolnay

It is now in the queue for this repository.

bors commented 1 week ago

:hourglass: Testing commit fb2d9cdfc54aea306223992e13ce4b2278d9f0c6 with merge 60a7c191c6151fa771d58c8ec0f0d1173c4a6037...

bors commented 1 week ago

:sunny: Test successful - checks-actions Approved by: dtolnay Pushing 60a7c191c6151fa771d58c8ec0f0d1173c4a6037 to master...

rust-timer commented 1 week ago

Finished benchmarking commit (60a7c191c6151fa771d58c8ec0f0d1173c4a6037): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment. | | mean | range | count | |:----------------------------------:|:-----:|:--------------:|:-----:| | Regressions ❌
(primary) | - | - | 0 | | Regressions ❌
(secondary) | - | - | 0 | | Improvements ✅
(primary) | -1.8% | [-2.2%, -1.5%] | 3 | | Improvements ✅
(secondary) | - | - | 0 | | All ❌✅ (primary) | -1.8% | [-2.2%, -1.5%] | 3 |

Cycles

Results This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment. | | mean | range | count | |:----------------------------------:|:-----:|:--------------:|:-----:| | Regressions ❌
(primary) | - | - | 0 | | Regressions ❌
(secondary) | - | - | 0 | | Improvements ✅
(primary) | - | - | 0 | | Improvements ✅
(secondary) | -2.9% | [-3.5%, -2.2%] | 2 | | All ❌✅ (primary) | - | - | 0 |

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.387s -> 676.069s (0.10%) Artifact size: 315.97 MiB -> 315.95 MiB (-0.01%)