Add lldb-repr command for tests/debuginfo

[Walnut356]

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.

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::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.

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

• Variable child mismatches don't have error reporting
• 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.
• 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
• 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).
• 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

[Kobzol]

A lot of stuff :) I left some initial comments, but I'll have to test this locally to really understand what's going on.

View changes since this review

[Kobzol]

The dash is a bit inconsistent here, as there are underscores in the other two names.

[Kobzol]

This looks a bit sus, is there some guarantee that the check function will be called with increasing values of breakpoint_idx?

[Walnut356]

The breakpoint index is guaranteed to only ever increment in runner.py but i suppose that doesnt prevent e.g. breakpoint with repr -> breakpoint without repr -> breakpoint with repr from causing issues. I dont think the tests ever do that? But better safe than sorry for sure.

[Kobzol]

Yeah that doesn't sound all that far fetched. I'd rather if the code didn't have too many similar assumptions, especially since crashes or logic bugs in the Python layer might be quite difficult to discover and debug.

[Walnut356]

Should be taken care of now. --bless will output a blank breakpoint if it encounters any skipped breakpoints, so indexing into the list should never fail (i added error handling anyway just in case). Outputting empty breakpoints isn't ideal, but it's almost certainly rare enough that it's cheaper than converting breakpoints to dict[int, ...].

[Kobzol]

Unfinished comment?

[Kobzol]

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_DEFAULT is systemArch, and passing it to debugger.CreateTargetWithFileAndArch ends with an error (error: unable to find a plug-in for the platform named "systemArch")
• Using SBDebugger::CreateTargetWithFileAndTargetTriple just 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.

View changes since this review

[Kobzol]

I'd rather turn each test into a directory and store the files next to the Rust file with the code.

[Kobzol]

Suggested change

	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.

[Walnut356]

I unfortunately dont have a copy of lldb 18 handy. Does the following work?

lldb.debugger.CreateTargetWithFileAndTargetTriple(
    target_path,
    lldb.SBPlatform.GetHostPlatform().GetTriple()
)

[Kobzol]

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.

[Walnut356]

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

[Kobzol]

It seems that stderr is not propagated outside of the debugger script? I'd suggest printing everything to stdout instead.

[Walnut356]

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).

Maybe we should set --batch to ensure that LLDB will always end after executing the given script commands?

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 SBCommandInterpreter

[Walnut356]

That should do it for all the major checks. Here are some example error messages for the variable side of things:

Synthetic throwing exceptions

For this error I just commented out the part of StdVecSyntheticProvider that sets the element type and element size. The error includes a heuristic "check the synthetic" warning since all of the children are invalid, and a traceback on the exception that caused the invalid children.

...
repr vec
  [repr error: vec] All children of this object are invalid SBValue objects.
    This is almost always caused by invalid state or logic in the SyntheticProvider.
    This object's synthetic appears to be 'lldb_lookup.StdVecSyntheticProvider'
  [repr error: var 'vec' Synthetic] Error while running SyntheticProvider:
Traceback (most recent call last):
  File "/home/walnut356/notmycode/rust/src/etc/lldb_batchmode/check_lldb.py", line 346, in var_matches
    if not all(
           ^^^
  File "/home/walnut356/notmycode/rust/src/etc/lldb_batchmode/check_lldb.py", line 347, in <genexpr>
    synth.get_child_at_index(i).IsValid()
    ~~~~~~~~~~~~~~~~~~~~~~~~^^^
  File "/home/walnut356/notmycode/rust/src/etc/lldb_providers.py", line 1050, in get_child_at_index
    address = start + index * self.element_type_size
                              ^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'StdVecSyntheticProvider' object has no attribute 'element_type_size'. Did you mean: 'element_type'?
...

Further down in the same error message, we get an error for osstring (which breaks due to the internal vec's synthetic not working). Child errors only print out "leaf" errors. i.e. if a child's children don't match, it only prints the child's children, not the child itself. So in this instance, osstring.inner.inner reports the error:

repr os_string
  [repr error: os_string.inner.inner] All children of this object are invalid SBValue objects.
    This is almost always caused by invalid state or logic in the SyntheticProvider.
    This object's synthetic appears to be 'lldb_lookup.StdVecSyntheticProvider'
  

We also have the summary error:

[repr error: var 'os_string'] pretty_print (Summary Output) does not match.
    Expected: "IAMA OS string 😃"
    Got: "

Synthetic/Summary not registered

For this, i just commented out the code that registers the Vec providers

repr vec
  [repr error: vec] The following children do not match (expected -> got):
    [0]: unsigned long = 4 -> buf: alloc::raw_vec::RawVec<unsigned long, alloc::alloc::Global> = None
    [1]: unsigned long = 5 -> len: unsigned long = 4
  [repr error: var 'vec'] summary (Summary Provider) does not match.
    Expected: lldb_lookup.SizeSummaryProvider
    Got: lldb_lookup.StructSummaryProvider
  [repr error: var 'vec'] synthetic (Synthetic Provider) does not match.
    Expected: lldb_lookup.StdVecSyntheticProvider
    Got: lldb_lookup.synthetic_lookup

And OsString reports:

repr os_string
  [repr error: os_string.inner.inner] The following children do not match (expected -> got):
    [0]: unsigned char = 73 -> buf: alloc::raw_vec::RawVec<unsigned char, alloc::alloc::Global> = None
    [1]: unsigned char = 65 -> len: unsigned long = 19
  [repr error: var 'os_string'] pretty_print (Summary Output) does not match.
    Expected: "IAMA OS string 😃"
    Got: "

Too many children

The following is reported when adding an additional element to the vec in pretty-std.rs

repr vec
  [repr error: vec] The following children were found, but were not expected:
    [4]: unsigned long = 8
  [repr error: var 'vec'] pretty_print (Summary Output) does not match.
    Expected: size=4
    Got: size=5

Summary throws exception

For this error, I just set the first line of lldb_providers.SizeSummaryProvider to raise Exception("uh oh")

repr vec
  [repr error: var 'vec'] pretty_print (Summary Output) not found, expected: size=4
  [repr error: var 'vec' Summary] Error while running Summary provider:
Traceback (most recent call last):
  File "/home/walnut356/notmycode/rust/src/etc/lldb_batchmode/check_lldb.py", line 292, in var_matches
    _ = provider(valobj, {})
  File "/home/walnut356/notmycode/rust/src/etc/lldb_providers.py", line 330, in SizeSummaryProvider
    raise Exception("uh oh")
Exception: uh oh

Mismatching `BasicType`/`TypeClass`

For these, the error will seem "backwards" because I had to edit the test data:

repr b
  [repr error: type 'bool'] basic_type (lldb.eBasicType) does not match.
    Expected: 11 (BasicType.Short)
    Got: 21 (BasicType.Bool)
repr i
  [repr error: type 'long'] type_class (lldb.eTypeClass) does not match.
    Expected: 20 (Builtin|ComplexFloat)
    Got: 4 (Builtin)

---

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):

• Reading/deserializing INPUT_DATA takes a few dozen milliseconds
• Each individual variable takes ~500μs-6ms, with ~1ms being the average. The ones that take longer are typically due to the first couple variables of a test, which have to check a disproportionate number of underlying types. Since type checks are cached, all future variables that test that type just do a dict lookup instead of a full type test.

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.

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

compiletest directives have been modified. Please add or update docs for the
new or modified directive in src/doc/rustc-dev-guide/.

[jieyouxu]

On initial glance this looks reasonable. I also have to play around with this locally to get a better feel

View changes since this review

[Kobzol]

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.

View changes since this review

[Kobzol]

There are trade-offs for both approaches, but once we use a directory, maybe we could name the debugged file main.rs or something? Like we do for runmake tests (where every test is named rmake.rs).

[jieyouxu]

I think that using directories with an entry point or such seems very reasonable