[Walnut356]

Adds a comprehensive LLDB test using the new test format. This should cover pretty much all the edge case issues I've encountered before. In a followup, I'll add the GDB commands and test data (and remove redundant Vec testing from other tests).

r? @Kobzol, @jieyouxu

[rustbot]

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

[Kobzol]

Thank you! So if I understand it correctly, this test would be focused on Vecs, which makes sense, and Vec-related tests would be mostly removed from other tests. It still leaves the question of what to do about all the types, IIRC we were discussing that we would be printing those only in some tests, on an opt-in basis?

View changes since this review

[Kobzol]

We should figure out some magic comment syntax, like // cfg(bootstrap), to make it easier to grep for things that are waiting for a LLDB update. Maybe something like // fixme(min-lldb)?

[Kobzol]

I kinda forgot where we ended in our discussion about printing those types in all tests. Many of the types below here seem quite.. not useful, especially the nested options and such.

[Walnut356]

Atm types are only included if the type in question has a visualizer (or if it's a generic or field of a type with a visualizer). There's maybe more heuristic optimizations that can be made there, but it's tricky to figure out what exactly each visualizer "needs" to know.

In this case, a lot of the type data comes from Vec just having a lot of nesting (RawVec, RawVecInner) alongside each generic instantiation (though I did try to keep the number of different instantiations low). We do end up traversing all of that nesting in the visualizer. Option<Vec<T>> was included specifically to cover #155509. Spotting that error relies on knowing the discriminant, so having e.g. "$variant$4294967295" present is useful. On MSVC I'll eventually need to add some extra logic to extract the discriminant since they're stored as static fields.

We could maybe condense generic instantiations into a single Vec<T> entry since, for example, the layout of Vec<usize> and Vec<u8> are identical. We could either special case it for common types like Vec and HashMap, or we could add in a more generalized solution (though that comes with extra complexity in debugger_tester).

FWIW, there shouldn't be more than 1 test in this format per type we have a provider for (right now it'd be ~14 tests total i think?). I don't think this level of specificity is necessary for the majority of the tests in tests/debuginfo (e.g. stepping behavior, variable shadowing).

[Walnut356]

Hmm, i've had another thought.

I mentioned a while ago that we could amortize the type storage by storing them top-level for all tests in a single file. Not only would this share types between multiple tests, but we could also share types between targets (e.g. most Vec<T> s are identical, target-to-target).

The difficulty with that is knowing when types in that file should be deleted. Since they're only "observed" on a test-by-test basis, we wouldnt be able to tell whether types are unused because it's no longer needed vs when it's unused because it's only used in a different test.

That can be solved by tagging the types with the tests they're used in. We can check against that to ensure that we've used every type a given test expects, and when blessing, we can remove the given tests listing from all types, and then re-add it to each type used in the test.

The other issue is conflicting types with the same name. For user defined types in each test, we csn ensure they're namespaced uniquely by adding a compiler option to match the object name to the test name. The tricky part is handling std types that differ by OS (e.g. OsString, which uses a Utf8Buf on non-windows and a Wtf8Buf on windows). Additionally, iirc, sum-type enums also arent guaranteed to (and currently dont) choose the same discriminant value on windows-gnu vs linux-gnu for types like Option<String> that niche optimize on NoHiBit<T>.

That's only really a problem if we use a direct dict[type_name, type_data] mapping though. Instead, we could use a flat array and lookup by index, something like:

class TypeIndex:
    windows_msvc: Optional[int]
    windows_gnu: Optional[int]
    non_windows: Optional[int]

class Types:
    index_by_name: dict[str, TypeIndex]
    types: list[TypeData]

That would also neatly solve the first issue, since we can just check that every index in range(len(Types.types)) is present, and cull any type entry that dont appear in any TypeIndex.

[Walnut356]

oh wait, i suppose the issue with that would be race conditions during --bless since the tests are run in parallel 🤦‍♂️

If we still want a central types.json (without serializing tests/debuginfo), we could have each test output their own temporary types.json during --bless, and then have compiletest stitch them into a single file as a final step after all the tests complete. Not an amazing solution, but it's something I guess

[Kobzol]

I think that having a shared state in between all tests is too complicated (nevertheless thanks for exploring that option :) ).

I think that a simpler solution could be to simply have an opt-in flag for each test, which would enable/disable the gathering and rendering of all the types (depending on what we decide to be the "right" default). Maybe there are tests for which all the recursive types are not super interesting, or could be displayed only for debugging the test itself. But I feel like this gets back into a territory that we discussed a couple of times, so I don't want to get into a circle :)

I'm fine with the test as-is, the large test data in the JSON file is an orthogonal issue to this PR.

You can r=me after changing the LLVM comment (#164457 (comment)).

[Walnut356]

@bors r=@Kobzol

[rust-bors]

📌 Commit bd31a67 has been approved by Kobzol

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 5. This pull request will be tested once the tree is reopened.

Reason for tree closure: spurious failures