Add lldb-repr command for tests/debuginfo#158298
Conversation
e416d4e to
7f7680a
Compare
There was a problem hiding this comment.
Maybe we should set --batch to ensure that LLDB will always end after executing the given script commands?
I had some troubles testing this on LLDB 18. I know that it is an ancient version, but I still wonder about if we are making assumptions that might only hold for specific LLDB version too much.
- stderr is not propagated outside of the debugger script. Only stdout is.
LLDB_ARCH_DEFAULTissystemArch, and passing it todebugger.CreateTargetWithFileAndArchends with an error (error: unable to find a plug-in for the platform named "systemArch")- Using
SBDebugger::CreateTargetWithFileAndTargetTriplejust hangs (probably some UB/segfault happens?)
Maybe LLDB 18 is just broken too much to have any hope of running this script? It works with LLDB 22, except for stderr, which just doesn't seem to be propagated.
| target: lldb.SBTarget = debugger.CreateTarget( | ||
| target_path, None, None, True, target_error | ||
| ) |
There was a problem hiding this comment.
| target: lldb.SBTarget = debugger.CreateTarget( | |
| target_path, None, None, True, target_error | |
| ) | |
| target_triple = get_env_arg("LLDB_BATCHMODE_TARGET_TRIPLE") | |
| target: lldb.SBTarget = debugger.CreateTarget( | |
| target_path, target_triple, None, True, target_error | |
| ) |
Setting None for the target triple segfaults my LLDB 18. The C++ code dereferences that pointer (?), so it shouldn't be NULL.
There was a problem hiding this comment.
I unfortunately dont have a copy of lldb 18 handy. Does the following work?
lldb.debugger.CreateTargetWithFileAndTargetTriple(
target_path,
lldb.SBPlatform.GetHostPlatform().GetTriple()
)There was a problem hiding this comment.
No, it also get stuck. But nevermind, LLDB 18 is old and we shouldn't try to support it. I downloaded LLDB 22 to be able to test this.
There was a problem hiding this comment.
Oh, it just occurred to me that even if this function worked in lldb 18, importing from_lldb.py wouldn't because it uses a constant that doesn't exist prior to lldb 22 (lldb.eBasicTypeFloat128).
I did check (for sanity's sake) and all other eBasicType values have existed for 14-15 years except for char8 which was added in 2022, which is about 2 years prior to lldb 18 releasing
|
Converted 1 more test to help confirm the last bits of the comparison logic. The 4 converted tests cover the core functionality: primitives, non-primitive builtins (slice, str), some of our standard container types (with generics, and with synthetic and summary providers), and sum-type, niche-optimized enums (which are a huge pain in general).
I'll give this a try later today. It definitely sounds ideal, but I want to make sure it preserves the exit status code and doesn't somehow break everything when we run commands in the |
9000709 to
6dd48f8
Compare
|
That should do it for all the major checks. Here are some example error messages for the variable side of things: Synthetic throwing exceptionsFor this error I just commented out the part of Further down in the same error message, we get an error for We also have the summary error: Synthetic/Summary not registeredFor this, i just commented out the code that registers the And OsString reports: Too many childrenThe following is reported when adding an additional element to the vec in Summary throws exceptionFor this error, I just set the first line of Mismatching `BasicType`/`TypeClass`For these, the error will seem "backwards" because I had to edit the test data: I know performance isn't the most important concern, but I wanted to make sure I wasn't absolutely destroying the run time of the test suite (python being python and all). From a really scuffed benchmark of a passing test (the error path is obviously much slower due to both the additional logic, and needing to print way more stuff):
So, I'd guess quite a bit slower than the old string-based checks, but not horrible considering the quality of the errors. I've got some experience writing performance-sensitive python, so I can always clean this up and reduce the runtime more later. There's definitely some redundant iteration and such in the error checking, but I was more concerned with keeping things simple and easy to reason about. |
|
Some changes occurred in src/tools/compiletest cc @jieyouxu
|
There was a problem hiding this comment.
I'll just note that this PR kind of grown out of proportions 😆 Some of the separate functionality, like nice printing of errors, could definitely land in separate PRs, the next time (fine by me to keep it in this PR to avoid splitting complexity). I hope we'll have enough of the base infra landed so that follow-up changes can be done more incrementally, because reviewing 3k diffs (even if some of that is JSON) is.. ooh :)
Otherwise it looks good.
|
Oh yeah, we should also probably gate these tests since they're dependent on lldb 22.1 (and only have data for one target atm). Simplest would be I'm not sure exactly how you guys wanted the transition from the old tests to the new tests to go, especially w.r.t CI. This PR marks the point where the core functions are there and most(?) of the LLDB tests could reasonably be translated to the new format. We can also step back the number of tests we actually change in this PR if that would work better. I mostly included them to give y'all tests that cover all the error scenarios during the draft and review. |
|
Regarding the transition, currently we run the tests only on macOS, IIRC? So if we start migrating the tests one by one, and the migration will move it to the new format, while making it also work on other OSes, then that would be ideal. But for that we have to prepare the right LLDB version on CI first, of course. |
|
fair enough. So if i'm understanding correctly, I should split the test conversions into their own PRs? |
|
I'd leave one test in this PR to check if this works, but then migrate the rest of the tests in follow-up PRs. |
24a240c to
30152a8
Compare
|
Rad, it should be good to go now |
|
Nice find! You can revert the test change now, and maybe squash the commits, and I think we can try moving forward with this. |
|
Since these passed on essentially LLDB 19, should we test with whatever version we currently use on CI for linux/windows (lldb 21 i think)? |
|
If the tests pass, I would leave it like this, because updating LLDB on Apple might be its own can of worms.. Let's try to get this PR merged, to unblock further work. |
|
Ah, i meant running |
|
Ah, I see. Well, I think that running the new test suite at all, even just on Apple, is already a big improvement, and I would try other improvements in follow-up PRs. |
509f53d to
22f5bae
Compare
There was a problem hiding this comment.
Looks good to me, so I'd try to merge it to see what happens on CI. But I'll let @jieyouxu approve it too.
|
I'll have some time to look closer tmrw |
| @@ -154,6 +154,7 @@ pub(crate) const KNOWN_DIRECTIVE_NAMES: &[&str] = &[ | |||
| "known-bug", | |||
| "lldb-check", | |||
| "lldb-command", | |||
| "lldb-repr", | |||
There was a problem hiding this comment.
Suggestion: could we leave a quick note on what lldb-repr is supposed to do in rustc-dev-guide?
There was a problem hiding this comment.
(Entirely fine as a follow-up)
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
22f5bae to
be8f150
Compare
be8f150 to
d5f9130
Compare
|
@rustbot ready |
There was a problem hiding this comment.
Thanks, it'd be cool if we can remark lldb-repr being a new debuginfo test suite directive in r-d-g but I don't want that to block this PR (let's try to get it merged ASAP)
@bors r=Kobzol,jieyouxu rollup=never (debuginfo test suite)
|
For sure, i'm working on filling out the testing section, as well as updating a few odds and ends that have changed on LLDB's end |
Add `lldb-repr` command for `tests/debuginfo`
Very much so a work in progress, but there's a bunch of stuff that probably needs discussing, so I figured I'd start that up now.
To recap, the control flow is as follows:
1. `lldb-repr` is automatically split into, essentially `lldb-command:repr var` + `lldb-check:var: Ok`
2. the relevant target, bless state, paths, etc. are passed to the LLDB command via env vars
3. The lldb command runs `lldb_batchmode.main()`
4. the input commands are executed line by line.
5. If the command is a `repr` pseudo-command, it is intercepted and passed to the checking logic
5a. if blessing, `INPUT_DATA` is blank. Each check inserts the appropriate var into `INPUT_DATA`, and then runs the checking logic against `INPUT_DATA` as normal (for sanity reasons)
5b. the checking logic checks the variable against `INPUT_DATA` (<-- this is incomplete atm, see below for what still isn't finished) and prints any mismatches that occur.
6. Once all commands have been run (or if a `quit`/`exit` command is encountered) `lldb_batchmode` checks 1. have we seen every type that exists in the input data? 2. have we seen every variable that exists in the input data? If not, it reports what was missing and exits with an error code.
6a. If `--bless`, and no errors occurred, and at least 1 `repr` pseudo-command was processed, `INPUT_DATA` is serialized and written to the input data file.
To preemptively answer the question "When blessing, why not just build a second `TargetData` instance and compare `INPUT_DATA` to that?" - the goal of diffing in python (rather than rust) is to still have access to the underlying LLDB objects for error reporting purposes.
<details>
<summary>Sample Output (`basic-types.rs`)</summary>
The input data was manually tampered with to force errors (`char32_t` size set to 1, double type renamed to `half`, `int` type deleted)
```txt
error: Error while running LLDB
status: exit status: 1
... # skipping to repr output for brevity
repr b
b: Ok
repr i
i: Ok
repr c
[repr error: type 'char32_t'] size does not match.
Expected: '1'
Got: '4'
repr i8
i8: Ok
repr i16
i16: Ok
repr i32
i32: Ok
repr i64
i64: Ok
repr u
u: Ok
repr u8
u8: Ok
repr u16
u16: Ok
repr u32
[repr error: type 'unsigned int'] type not found in input data
repr u64
u64: Ok
repr f32
f32: Ok
repr f64
[repr error: type 'double'] type not found in input data
[repr error] The following types were expected, but were not tested:
{'half'}
```
</details>
<details>
<summary>Sample Output (`simple-struct.rs`)</summary>
The input data was manually tampered with to force errors.
* `simple_struct::NoPadding64` field name `y` to `q`
* `simple_struct::NoPadding163264` last field moved to first position
* `simple_struct::InternalPadding` `y` field deleted
* `simple_struct::PaddingAtEnd` `q` field added
Keep in mind that I edited the *expected* data, not the actual structs in the test file, so the output reports the opposite of the changes i made (sortof) i.e. it reports that `q` was deleted because it exists in the expected data but not in the LLDB object.
```txt
repr no_padding16
no_padding16: Ok
repr no_padding32
no_padding32: Ok
repr no_padding64
[repr error: type 'simple_struct::NoPadding64'] The following field(s) appear to have been renamed. If this is expected,
consider rerunning with the `--bless` option:
['q -> y']
repr no_padding163264
[repr error: type 'simple_struct::NoPadding163264'] Field(s) appear to have been rearranged. If this was expected, try re-running with the `--bless` option
Expected:
[ Field(name='a', type='short', offset=12),
Field(name='b', type='unsigned short', offset=14),
Field(name='c', type='int', offset=8),
Field(name='d', type='unsigned long', offset=0)]
Got:
[ Field(name='d', type='unsigned long', offset=0),
Field(name='a', type='short', offset=12),
Field(name='b', type='unsigned short', offset=14),
Field(name='c', type='int', offset=8)]
repr internal_padding
[repr error: type 'simple_struct::InternalPadding'] The following field(s) appear to have been added to the type:
{Field(name='y', type='long', offset=0)}
repr padding_at_end
[repr error: type 'simple_struct::PaddingAtEnd'] The following field(s) appear to have been removed from the type:
{Field(name='q', type='float', offset=12)}
[repr error] The following types were expected, but were not tested:
{'double', 'long', 'unsigned long'}
```
</details>
The errors are of the form `[repr error <error source>] <error info>`. The errors are slightly indented to make it easier to scan and associate errors with their source. The formatting is 1st draft and largely abstracted away via [these functions](https://github.com/Walnut356/rust/blob/bd4a0c7b80b448778247dda4b203074d58eff64d/src/etc/lldb_batchmode/common.py#L20-L39). We can pretty trivially change how they're formatted by modifying/replacing these functions.
Also, each type in a test is only tested once per test, regardless of how many variables/fields reference it. When a type is seen a second time, if the prior check was a mismatch, it reports that the type does not match and **does not** repeat the specific error messages of the mismatch. Instead, it directs people to look at the original type error further up in the output.
# TODO
- [x] Variable child mismatches don't have error reporting
- [x] Rearrange the checks a bit so the type check happens midway through the variable check. Some of the variable's information is useful for the type's error reporting and vice versa.
- [x] use some info from `lldb_providers` and the original `SBValue` and `SBType` to improve error messages
- [ ] Some sort of warning if the python/lldb version and/or feature flags don't match
- [x] A few more tests with input data to help verify behavior (one with generics, one with a synthetic/summary provider, probably need to enforce a formatter for `u8`/`i8` at some point to test that too).
- [x] Various improvements to the error message wording/information/formatting/consistency.
- [ ] Probably some other stuff i'm forgetting about that I'll add in later
r? @jieyouxu, @Kobzol

View all comments
Very much so a work in progress, but there's a bunch of stuff that probably needs discussing, so I figured I'd start that up now.
To recap, the control flow is as follows:
lldb-repris automatically split into, essentiallylldb-command:repr var+lldb-check:var: Oklldb_batchmode.main()reprpseudo-command, it is intercepted and passed to the checking logic5a. if blessing,
INPUT_DATAis blank. Each check inserts the appropriate var intoINPUT_DATA, and then runs the checking logic againstINPUT_DATAas normal (for sanity reasons)5b. the checking logic checks the variable against
INPUT_DATA(<-- this is incomplete atm, see below for what still isn't finished) and prints any mismatches that occur.quit/exitcommand is encountered)lldb_batchmodechecks 1. have we seen every type that exists in the input data? 2. have we seen every variable that exists in the input data? If not, it reports what was missing and exits with an error code.6a. If
--bless, and no errors occurred, and at least 1reprpseudo-command was processed,INPUT_DATAis serialized and written to the input data file.To preemptively answer the question "When blessing, why not just build a second
TargetDatainstance and compareINPUT_DATAto that?" - the goal of diffing in python (rather than rust) is to still have access to the underlying LLDB objects for error reporting purposes.Sample Output (`basic-types.rs`)
The input data was manually tampered with to force errors (`char32_t` size set to 1, double type renamed to `half`, `int` type deleted)error: Error while running LLDB status: exit status: 1 ... # skipping to repr output for brevity repr b b: Ok repr i i: Ok repr c [repr error: type 'char32_t'] size does not match. Expected: '1' Got: '4' repr i8 i8: Ok repr i16 i16: Ok repr i32 i32: Ok repr i64 i64: Ok repr u u: Ok repr u8 u8: Ok repr u16 u16: Ok repr u32 [repr error: type 'unsigned int'] type not found in input data repr u64 u64: Ok repr f32 f32: Ok repr f64 [repr error: type 'double'] type not found in input data [repr error] The following types were expected, but were not tested: {'half'}Sample Output (`simple-struct.rs`)
The input data was manually tampered with to force errors.simple_struct::NoPadding64field nameytoqsimple_struct::NoPadding163264last field moved to first positionsimple_struct::InternalPaddingyfield deletedsimple_struct::PaddingAtEndqfield addedKeep in mind that I edited the expected data, not the actual structs in the test file, so the output reports the opposite of the changes i made (sortof) i.e. it reports that
qwas deleted because it exists in the expected data but not in the LLDB object.repr no_padding16 no_padding16: Ok repr no_padding32 no_padding32: Ok repr no_padding64 [repr error: type 'simple_struct::NoPadding64'] The following field(s) appear to have been renamed. If this is expected, consider rerunning with the `--bless` option: ['q -> y'] repr no_padding163264 [repr error: type 'simple_struct::NoPadding163264'] Field(s) appear to have been rearranged. If this was expected, try re-running with the `--bless` option Expected: [ Field(name='a', type='short', offset=12), Field(name='b', type='unsigned short', offset=14), Field(name='c', type='int', offset=8), Field(name='d', type='unsigned long', offset=0)] Got: [ Field(name='d', type='unsigned long', offset=0), Field(name='a', type='short', offset=12), Field(name='b', type='unsigned short', offset=14), Field(name='c', type='int', offset=8)] repr internal_padding [repr error: type 'simple_struct::InternalPadding'] The following field(s) appear to have been added to the type: {Field(name='y', type='long', offset=0)} repr padding_at_end [repr error: type 'simple_struct::PaddingAtEnd'] The following field(s) appear to have been removed from the type: {Field(name='q', type='float', offset=12)} [repr error] The following types were expected, but were not tested: {'double', 'long', 'unsigned long'}The errors are of the form
[repr error <error source>] <error info>. The errors are slightly indented to make it easier to scan and associate errors with their source. The formatting is 1st draft and largely abstracted away via these functions. We can pretty trivially change how they're formatted by modifying/replacing these functions.Also, each type in a test is only tested once per test, regardless of how many variables/fields reference it. When a type is seen a second time, if the prior check was a mismatch, it reports that the type does not match and does not repeat the specific error messages of the mismatch. Instead, it directs people to look at the original type error further up in the output.
TODO
lldb_providersand the originalSBValueandSBTypeto improve error messagesu8/i8at some point to test that too).r? @jieyouxu, @Kobzol