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]

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

[Walnut356]

yeah, I definitely like that better. Should be a quick fix

[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

[Kobzol]

I tried changing some values or even key names in non_windows.json (on Linux), but the test stays green 🤔 With LLDB 22.

[Walnut356]

Hmm, I'm not able to replicate that. Changing a key gave me an exception when INPUT_DATA is inspected since one of the internal vars failed to deserialize. Changing input data gave a failure + the expected error message.

Are you running with --force-rerun? I don't think there's a check yet for changes in the input data

[Kobzol]

Ah, I see what happened. I set my lldb to point at a non-existent path, and the test suite silently skipped the LLDB part of the test 🥶 That's why I saw a different test count while trying LLDB 18 vs 22. That's not a great behavior, if LLDB is set explicitly and it doesn't work, it should be a loud error. I'll send a PR to fix that.

After fixing the LLDB path, it works.

I realized that we should ideally also ensure that we have at least one CI job that actually runs with LLDB 22, otherwise this test will not be executed on CI at all. @jieyouxu Do you happen to know if we have such a job? 😆 I guess that one way to find out would be to break the test in this PR and try to merge it 🙈

[Kobzol]

I tried changing the size of bool to 2, and noticed that the error message has quite a lot of data, and it's not easy for me to understand what happened. It seems like the other variables are still checked even if the first check fails?

I'd suggest erroring out early if an error is made, to make it obvious where is the problem, and if possible print the line where it happened (not sure if that's easy to do). I'd also move (or copy) the main part of the error to the end of the output, because that is the first thing that you see when the test fails. Or hide the verbose output behind a -v flag.

[jieyouxu]

@Kobzol we have no CI jobs that exercise lldb beyond whatever aarch64-apple (and now aarch64-apple-macos26) AFAIK, and those use the Apple/Swift lldb (the macos-15 one IIRC was stuck on something like Apple/Swift lldb 17).

I will take a look at the pin-debugger problems this weekend.

[Kobzol]

Opened #159056 for the LLDB discovery improvement.

[Walnut356]

IIRC Apple's LLDB version number is something like 17xx.xx.xxx, but based on the featureflags it's at least equivalent to LLVM-lldb 19 or 20

[jieyouxu]

IIRC Apple's LLDB version number is something like 17xx.xx.xxx, but based on the featureflags it's at least equivalent to LLVM-lldb 19 or 20

Even better 🙈

[Walnut356]

I'd suggest erroring out early if an error is made, to make it obvious where is the problem, and if possible print the line where it happened (not sure if that's easy to do).

Maybe a kindof --no-fail-fast flag would work? It isn't uncommon to have to diagnose issues for targets you don't have based on the error output from CI. Assuming we run it in PR CI (rather than bors), 70 minutes is still a rough amount of time to wait just to discover there was more than 1 error that needed resolving.

[Kobzol]

I was thinking that when you bless, you will overwrite the whole thing anyway, but you're right about the repeated waiting. Even if we had such a flag, we likely wouldn't turn it on in CI by default.

Well, I don't think people should debug issues on CI though, we will have to find some way of actually blessing the output, and that won't be doable by waiting for CI and then manually modifying the JSON files, hmm.

Anyway, you're right that more context helps. Maybe just try to print the data so that the error(s) is/are also repeated at the end? Otherwise it's not that easy to find the error.

[jieyouxu]

Well, I don't think people should debug issues on CI though, we will have to find some way of actually blessing the output, and that won't be doable by waiting for CI and then manually modifying the JSON files, hmm.

That is true. I don't have a very good idea for how to handle the problem of not having access to the specific $environment.

[Walnut356]

stderr is not propagated outside of the debugger script. Only stdout is.

🤦‍♂️

I somehow missed the fact that lldb.SBDebugger has .GetErrorFileHandle() (which returns a python-native io.TextIoWrapper) and .GetErrorFile() (which returns an lldb.SBFile object with .Read() and .Write()).

I did check this and it works with compiletest, so we can print to stderr whenever we want.

[Kobzol]

But before you were using just print(..., file=sys.stderr), or not? That has some connection to the methods that you described?

[Walnut356]

I think so.

Basically (as i understand it), each SBDebugger gets its own set of stdio file handles. Python gets its own, but LLDB pipes them to the main LLDB process (maybe?). For some reason that piping doesn't work in the context of compiletest, or compiletest isn't capturing it correctly or something? I don't know, because running lldb standalone, it works fine:

(lldb) script import sys; print("asdf", file=sys.stderr)
asdf

My best guess is that LLDB has to wait for the python script to end execution before it will pipe stderr output, but that we're killing LLDB before that can happen? Which would explain why, when python exits via an uncaught exception, it still shows up in stderr even though printing to stderr doesn't work.

The following seems to support that, since stdout is printing before both stderr prints:

(lldb) script
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> def a():
...     print("1", file=sys.stderr)
...     print("2", file=sys.stdout)
...     print("3", file=sys.stderr)
... 
>>> exit()
now exiting InteractiveConsole...
(lldb) script a()
2
1
3

lldb.debugger (i.e. the SBDebugger that the python script is running in) holds the stdio file handles for the main LLDB process, so if my guess is right, it means we're printing to stderr immediately and "bypassing" LLDB's need to pipe it.

[Walnut356]

Yeah, it definitely looks like lldb is just batch-printing it after the command finishes. Note that it prints out of order if i use a mix of sys.stdout and GetErrorFileHandle, but it prints in order if i use both GetErrorFileHandle and GetOutputFileHandle

(lldb) script 
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> def b():
...     print("1", file=lldb.debugger.GetErrorFileHandle())
...     print("2", file=sys.stdout)
...     print("3", file=lldb.debugger.GetErrorFileHandle())
... 
>>> exit()
now exiting InteractiveConsole...
(lldb) script b()
1
3
2
(lldb) script 
Python Interactive Interpreter. To exit, type 'quit()', 'exit()' or Ctrl-D.
>>> def c():
...     print("1", file=lldb.debugger.GetErrorFileHandle())
...     print("2", file=lldb.debugger.GetOutputFileHandle())
...     print("3", file=lldb.debugger.GetErrorFileHandle())
... 
>>> exit()
now exiting InteractiveConsole...
(lldb) script c()
1
2
3

[Walnut356]

I tried changing the size of bool to 2, and noticed that the error message has quite a lot of data, and it's not easy for me to understand what happened.

For reference, i have the full output of that below (i truncated the PATH var in the command: listing because it's REALLY long, thanks WSL, but everything else is as-is):

.
[debuginfo-lldb] tests/debuginfo/basic-types/main.rs ... F


failures:

---- [debuginfo-lldb] tests/debuginfo/basic-types/main.rs stdout ----
NOTE: compiletest thinks it is using LLDB version 2200

error: check directive(s) from `/home/walnut356/notmycode/rust/tests/debuginfo/basic-types/main.rs` not found in debugger output. errors:
    (main.rs:51) `b: Ok`
    (main.rs:65) ` fail`
the following subset of check directive(s) was found successfully:
    (main.rs:52) `i: Ok`
    (main.rs:53) `c: Ok`
    (main.rs:54) `i8: Ok`
    (main.rs:55) `i16: Ok`
    (main.rs:56) `i32: Ok`
    (main.rs:57) `i64: Ok`
    (main.rs:58) `u: Ok`
    (main.rs:59) `u8: Ok`
    (main.rs:60) `u16: Ok`
    (main.rs:61) `u32: Ok`
    (main.rs:62) `u64: Ok`
    (main.rs:63) `f32: Ok`
    (main.rs:64) `f64: Ok`
status: exit status: 0
command: LLDB_BATCHMODE_BLESS_TEST_DATA="0" LLDB_BATCHMODE_INPUT_DATA_PATH="/home/walnut356/notmycode/rust/tests/debuginfo/basic-types/lldb_input/non_windows.json" LLDB_BATCHMODE_SCRIPT_PATH="/home/walnut356/notmycode/rust/build/x86_64-unknown-linux-gnu/test/debuginfo/basic-types/main.lldb/main.debugger.script" LLDB_BATCHMODE_TARGET_PATH="/home/walnut356/notmycode/rust/build/x86_64-unknown-linux-gnu/test/debuginfo/basic-types/main.lldb/a" LLDB_BATCHMODE_TARGET_TRIPLE="x86_64-unknown-linux-gnu" PATH="..." PYTHONPATH="/home/walnut356/notmycode/rust/src/etc" PYTHONUNBUFFERED="1" "lldb" "--batch" "--one-line" "script --language python -- import lldb_batchmode; lldb_batchmode.main()"
--- stdout -------------------------------
(lldb) script --language python -- import lldb_batchmode; lldb_batchmode.main()
LLDB batch-mode script
----------------------
Debugger commands script is '/home/walnut356/notmycode/rust/build/x86_64-unknown-linux-gnu/test/debuginfo/basic-types/main.lldb/main.debugger.script'.
Target executable is '/home/walnut356/notmycode/rust/build/x86_64-unknown-linux-gnu/test/debuginfo/basic-types/main.lldb/a'.
Current working directory is '/home/walnut356/notmycode/rust'
Creating a target for '/home/walnut356/notmycode/rust/build/x86_64-unknown-linux-gnu/test/debuginfo/basic-types/main.lldb/a'
settings set auto-confirm true

version
lldb version 22.1.8 
command script import /home/walnut356/notmycode/rust/src/etc/lldb_lookup.py
script print(lldb_lookup.FEATURE_FLAGS)
LLDBFeature.StaticFields|TypeRecognizers|Float128
breakpoint set --file 'main.rs' --line 125
DEBUG: breakpoint added, id = 1
Breakpoint 1: where = a`main::main + 161 at main.rs:125:5, address = 0x0000000000001b81 
DEBUG: registering breakpoint callback, id = 1
DEBUG: successfully registered breakpoint callback, id = 1
repr b
  [repr error: type 'bool'] size does not match.
    Expected: 2
    Got: 1
  [repr error: type 'bool'] It appears these changes do not affect the type's providers. Consider rerunning with the `--bless` option
repr i
i: Ok
repr c
c: Ok
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
u32: Ok
repr u64
u64: Ok
repr f32
f32: Ok
repr f64
f64: Ok
------------------------------------------
--- stderr -------------------------------
warning: This version of LLDB has no plugin for the language "rust". Inspection of frame variables will be limited.
------------------------------------------

---- [debuginfo-lldb] tests/debuginfo/basic-types/main.rs stdout end ----

failures:
    [debuginfo-lldb] tests/debuginfo/basic-types/main.rs

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 344 filtered out; finished in 845.31ms

I'm not a huge fan of the formatting of the tests/debuginfo output in general, so i'm gonna dump my thoughts here while I ponder solutions.

---

The output can be chunked into ~3 different categories:

1. compiletest's errors (i.e. which lines were found and not found)
2. compiletest and lldb_batchmode vomiting a bunch of config and environment information
3. raw input and output from the actual test script that lldb_batchmode is running.

1 and 3 are obviously the most useful when debugging errors (1 less-so with the new testing framework), and it's unfortunate that they are split by the config/env vomit. There's also a bit of info in 3 that we don't necessarily care that much about, like the breakpoint registration. There's not a whole lot of vertical separation between any of the sections (there's also weak separation between multiple tests)

Currently, the new test output for section 3 functions "the same" as the old tests (i.e. the command is printed, and the output is printed on the following line(s)). Outputting that way does mean passing and failing variables are interspersed though.

One other issue (compared to a live REPL) is that it's not necessarily clear what is a command and what is an output if you don't know what to look for. LLDB prefixes repl commands with (lldb) by default, and has no prefix for output. It might be a good idea to do that artificially on our end since we manually print the commands ourselves.

I have an example below, it's more clear what is a command for sure, but the parens also make the error brackets less distinctive.

`(lldb)` prefix example

--- stdout -------------------------------
(lldb) script --language python -- import lldb_batchmode; lldb_batchmode.main()
LLDB batch-mode script
----------------------
Debugger commands script is 'C:\Coding\NotMyCode\rust\build\x86_64-pc-windows-msvc\test\debuginfo\basic-types\main.lldb\main.debugger.script'.
Target executable is 'C:\Coding\NotMyCode\rust\build\x86_64-pc-windows-msvc\test\debuginfo\basic-types\main.lldb\a.exe'.
Current working directory is 'C:\Coding\NotMyCode\rust'
Creating a target for 'C:\Coding\NotMyCode\rust\build\x86_64-pc-windows-msvc\test\debuginfo\basic-types\main.lldb\a.exe'
(lldb) settings set auto-confirm true

(lldb) version
lldb version 22.1.2 (https://github.com/llvm/llvm-project revision 1ab49a973e210e97d61e5db6557180dcb92c3e98) clang revision 1ab49a973e210e97d61e5db6557180dcb92c3e98 llvm revision 1ab49a973e210e97d61e5db6557180dcb92c3e98
(lldb) command script import C:\Coding\NotMyCode\rust\src/etc/lldb_lookup.py
(lldb) script print(lldb_lookup.FEATURE_FLAGS)
LLDBFeature.StaticFields|TypeRecognizers|Float128
(lldb) breakpoint set --file 'main.rs' --line 125
DEBUG: breakpoint added, id = 1
Breakpoint 1: where = a.exe`main::main + 167 at main.rs:125, address = 0x0000000140001157
DEBUG: registering breakpoint callback, id = 1
DEBUG: successfully registered breakpoint callback, id = 1
(lldb) repr b
  [repr error: type 'bool'] size does not match.
    Expected: 2
    Got: 1
  [repr error: type 'bool'] It appears these changes do not affect the type's providers. Consider rerunning with the `--bless` option
(lldb) repr i
i: Ok
(lldb) repr c
c: Ok
(lldb) repr i8
i8: Ok
(lldb) repr i16
i16: Ok
(lldb) repr i32
i32: Ok
(lldb) repr i64
i64: Ok
(lldb) repr u
u: Ok
(lldb) repr u8
u8: Ok
(lldb) repr u16
u16: Ok
(lldb) repr u32
u32: Ok
(lldb) repr u64
u64: Ok
(lldb) repr f32
f32: Ok
(lldb) repr f64
f64: Ok

I tried to make the errors visually distinct via indenting, but that doesn't seem to be enough, especially with the (lldb) prefix. Some color might make things more visually distinct:

We could also probably keep track of a list of the variables with errors and dump that to stderr (which appears at the bottom of the full test output) e.g.:

--- stderr -------------------------------
[repr error] The following variable(s) did not match the input data. See [repr error] listings in `stdout` for more details:
	var1, var2, var3
------------------------------------------

It's somewhat redundant with compiletest's line check reporting, but it also means you don't have config vomit inbetween the "this is what failed" and "this is how it failed"

Alternatively, i could batch all the errors and wait to print them til the end of stdout (or print them in stderr). I'd need to restructure things a tiny bit though since we don't get the "which variable did this indent-level occur for" for free

(lldb) repr b
(lldb) repr i
i: Ok
(lldb) repr c
c: Ok
(lldb) repr i8
i8: Ok
(lldb) repr i16
i16: Ok
(lldb) repr i32
i32: Ok
(lldb) repr i64
i64: Ok
(lldb) repr u
u: Ok
(lldb) repr u8
u8: Ok
(lldb) repr u16
u16: Ok
(lldb) repr u32
u32: Ok
(lldb) repr u64
u64: Ok
(lldb) repr f32
f32: Ok
(lldb) repr f64
f64: Ok
--- stderr -------------------------------
[repr error] var "b" had the following error(s):
	[repr error: type 'bool'] size does not match.
	    Expected: 2
		Got: 1
	[repr error: type 'bool'] It appears these changes do not affect the type's providers. Consider rerunning with the `--bless` option
------------------------------------------

[Kobzol]

I think that this part:

the following subset of check directive(s) was found successfully:
    (main.rs:52) `i: Ok`
    (main.rs:53) `c: Ok`
    (main.rs:54) `i8: Ok`
    (main.rs:55) `i16: Ok`
    (main.rs:56) `i32: Ok`
    (main.rs:57) `i64: Ok`
    (main.rs:58) `u: Ok`
    (main.rs:59) `u8: Ok`
    (main.rs:60) `u16: Ok`
    (main.rs:61) `u32: Ok`
    (main.rs:62) `u64: Ok`
    (main.rs:63) `f32: Ok`
    (main.rs:64) `f64: Ok`

is kinda useless, I'd remove it. But it seems like you already did that in your latest draft.

I like the (lldb) prefix, that makes it much more clear what is "input" and what is output. I also like the red color marking the error.

When an error happens, I'm usually not very interested in what worked, but I want to see what didn't work, and most importantly where it was. Seeing a bunch of "repr X. Ok" lines IMO isn't very useful, if the code didn't print any warning or something like that, then the fact that there were Ok lines is kind of a given, otherwise they would be marked as errors. So the useful information I get from that is mostly zero. But since it's part of the LLDB input/output sequence, probably we should keep it anyway to avoid artificially hiding part of that communication.

[Kobzol]

I realize now that the error output is very bikesheddable, we should deal with that in a follow-up. Let's try to get this PR merged as soon as possible, so that we can move forward.

But to do that, we have to either get LLDB 22 on some Linux CI job, or ensure that this new test runs on the macOS jobs that currently execute debuginfo tests. Let's see what happens on Apple:

@bors try jobs=aarch64-apple

[Walnut356]

is kinda useless, I'd remove it. But it seems like you already did that in your latest draft.

I omitted it for space, but it's still present in the actual output. It's part of the default output from filecheck, so the decision on whether to remove it should factor in all tests that use filecheck, rather than just tests/debuginfo.

When an error happens, I'm usually not very interested in what worked, but I want to see what didn't work, and most importantly where it was. Seeing a bunch of "repr X. Ok" lines IMO isn't very useful, if the code didn't print any warning or something like that, then the fact that there were Ok lines is kind of a given, otherwise they would be marked as errors

Keep in mind, what is printed after the filecheck results is the entirety of lldb and lldb_batchmode's stdout, which is the same data that is passed to filecheck. Due to that, it's semi-unavoidable that we print passing variables, since they filecheck needs that data in stdout to be able to see if something passed or failed.

We could maybe avoid it by desugarring lldb-repr to JUST lldb-command instead of lldb_command + lldb-check. We could then output the failing var list ourselves, and then have lldb exit with an error code that the test failed.

The downside of that would be that the error reporting would be inconsistent with other filecheck-based tests. Also, if a test ever used a mix of lldb-check and lldb-repr commands, half the error reporting would be at the top of the test results and the other half would be at the bottom.

[Walnut356]

Hmm, the test might end up filtered out on apple, depending on the version number. If it's still 17xx, compiletest will think it's less than 2200 and ignore it.

I need to fix the way compiletest reads versions anyway. Technically, we should have gated behind 2210 (since these tests are built around lldb 22.1.X), but when compiletest reads the version number from lldb, it ignores everything after the major version number and multiplies non-apple-lldb versions by 100 to "normalize" it to vaguely near apple's version numbers. That means compiletest thinks 22.1.X isnt a valid version for min-lldb-version:2210.

Bare minimum, i need to make it so the full semantic version is handled correctly, but it might be worth having a separate min-xcode-lldb-version

[rust-bors]

☀️ Try build successful (CI)
Build commit: d91b90e (d91b90e4d43f3d2c04a963547b358262a27cf450)
Base parent: b69e089 (b69e089eb3ec9976ddec602bf18c3d6a8ce72512)

[Kobzol]

I omitted it for space, but it's still present in the actual output. It's part of the default output from filecheck, so the decision on whether to remove it should factor in all tests that use filecheck, rather than just tests/debuginfo.

Yeah, I was probably unclear, but what I meant was that we should still keep the OK communication in the actual stdout, just not duplicate it in our own custom output when an error happens (because one of the checks failed).

Bare minimum, i need to make it so the full semantic version is handled correctly, but it might be worth having a separate min-xcode-lldb-version

If there is no simple mapping between the Apple and non-Apple versions, then yes, this would possibly be the most straightforward solution.