Add lldb-repr command for tests/debuginfo

[Walnut356]

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:

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]

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]

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

[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

[Walnut356]

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 min-lldb-verison: 22.1 and only-x86_64-unknown-linux-gnu, but that has consequences (that test being skipped for GDB too) in the meantime.

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.

[Kobzol]

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.

[Walnut356]

fair enough. So if i'm understanding correctly, I should split the test conversions into their own PRs?

[Kobzol]

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.

[Walnut356]

Rad, it should be good to go now

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@bors p=101
Scheduling

[rust-bors]

💔 Test for 3cafe1c failed: CI. Failed job:

• auto - pr-check-2 (web logs, enhanced plaintext logs)

[JonathanBrouwer]

@bors retrt

[rust-bors]

Unknown command "retrt". Run @bors help or go to https://bors.rust-lang.org/help to see available commands.

[JonathanBrouwer]

@bors retry

[rust-bors]

💔 Test for 0f8b5c4 failed: CI. Failed job:

• auto - dist-x86_64-freebsd (web logs, enhanced plaintext logs)

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@bors retry

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[rust-log-analyzer]

The job optional-x86_64-gnu-parallel-frontend failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

diff of stderr:

20 LL | |     Self::Item: Baz,
21    | |____________________^
22    = note: ...which again requires coherence checking all impls of trait `Foo`, completing the cycle
- note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:12:1: 14:20>` is well-formed
-   --> $DIR/next-solver-region-resolution.rs:12:1
+ note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed
+   --> $DIR/next-solver-region-resolution.rs:18:1
25    |
- LL | / impl<'a, T> Foo for &'a T
+ LL | / impl<'a, T> Foo for &T
27 LL | | where
- LL | |     Self::Item: 'a,
-    | |___________________^
+ LL | |     Self::Item: Baz,
+    | |____________________^
30    = note: for more information, see <https://rustc-dev-guide.rust-lang.org/overview.html#queries> and <https://rustc-dev-guide.rust-lang.org/query.html>
31 
32 error: aborting due to 1 previous error

Note: some mismatched output was normalized before being compared
- note: cycle used when checking that `<impl at /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed
-   --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1
+ note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed
+   --> $DIR/next-solver-region-resolution.rs:18:1
+ LL | / impl<'a, T> Foo for &T
+ LL | |     Self::Item: Baz,
+    | |____________________^

Compare output by lines enabled, diff by lines:
Expected contains these lines that are not in actual:
   | |___________________^
  --> $DIR/next-solver-region-resolution.rs:12:1
LL | / impl<'a, T> Foo for &'a T
LL | |     Self::Item: 'a,
note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:12:1: 14:20>` is well-formed
Actual contains these lines that are not in expected:
   | |____________________^
  --> $DIR/next-solver-region-resolution.rs:18:1
LL | / impl<'a, T> Foo for &T
LL | |     Self::Item: Baz,
note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed

The actual stderr differed from the expected stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args specialization/min_specialization/next-solver-region-resolution.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(test,FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "-Zthreads=4" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/specialization/min_specialization/next-solver-region-resolution" "-A" "unused" "-W" "unused_attributes" "-A" "internal_features" "-A" "incomplete_features" "-A" "unused_parens" "-A" "unused_braces" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Znext-solver=globally"
stdout: none
--- stderr -------------------------------
error[E0391]: cycle detected when coherence checking all impls of trait `Foo`
##[error]  --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:6:1
   |
LL | trait Foo { //~ ERROR cycle detected when coherence checking all impls of trait `Foo`
   | ^^^^^^^^^
   |
   = note: ...which requires building specialization graph of trait `Foo`...
note: ...which requires computing whether impls specialize one another...
  --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:12:1
   |
LL | / impl<'a, T> Foo for &'a T
LL | | where
LL | |     Self::Item: 'a,
   | |___________________^
note: ...which requires computing normalized predicates of `<impl at /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1: 20:21>`...
  --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1
   |
LL | / impl<'a, T> Foo for &T
LL | | where
LL | |     Self::Item: Baz,
   | |____________________^
   = note: ...which again requires coherence checking all impls of trait `Foo`, completing the cycle
note: cycle used when checking that `<impl at /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed
  --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1
   |
LL | / impl<'a, T> Foo for &T
LL | | where
LL | |     Self::Item: Baz,
   | |____________________^
   = note: for more information, see <https://rustc-dev-guide.rust-lang.org/overview.html#queries> and <https://rustc-dev-guide.rust-lang.org/query.html>

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0391`.
------------------------------------------

[rust-bors]

☀️ Test successful - CI
Approved by: Kobzol,jieyouxu
Duration: 3h 8m 15s
Pushing 9451e06 to main...

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling glob-match v0.2.1
   Compiling diff v0.1.13
   Compiling citool v0.1.0 (/home/runner/work/rust/rust/src/ci/citool)
    Finished `release` profile [optimized] target(s) in 43.67s
     Running `target/release/citool post-merge-report 5d4886964b04e1aa6aca158a677183a778e2950d 9451e069c34e1d7db23a155dcf9c25d1bc127055`
Downloading metrics of job aarch64-gnu
Downloading metrics of job aarch64-gnu-debug
Downloading metrics of job arm-android
Downloading metrics of job armhf-gnu
Downloading metrics of job dist-aarch64-linux
Downloading metrics of job dist-android
Downloading metrics of job dist-arm-linux-gnueabi

[rust-timer]

Finished benchmarking commit (9451e06): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.4%, secondary 3.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.1%	[3.1%, 3.1%]	1
Regressions ❌ (secondary)	3.8%	[2.3%, 6.5%]	3
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[-2.2%, 3.1%]	2

Cycles

Results (secondary -0.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.9%	[4.9%, 4.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.7%	[-6.7%, -6.7%]	1
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 490.416s -> 489.176s (-0.25%)
Artifact size: 387.71 MiB -> 387.73 MiB (0.01%)