Right now I think everyone has the habit of doing an autoupdate then
using source control for a diff. This is offering an option of better
diff output from the test.
For example:
```
TEST: toolchain/driver/testdata/fail_flush_errors.carbon !
Ran 1 tests in 81 ms wall time, 8 ms across threads
testing/file_test/file_test_base.cpp:264: Failure
Value of: SplitOutput(test_file.actual_stderr)
Expected: matches elements with union diff
Actual: { "fail_flush_errors.carbon:22:3: error: name `undeclared1` not found [NameNotFound]", " undeclared1;", " ^~~~~~~~~~~", "", "fail_flush_errors.carbon:31:3: error: `Core.String` implicitly referenced here, but package `Core` not found [CoreNotFound]", " \"undec\\x6Cared2\";", " ^~~~~~~~~~~~~~~~", "", "fail_flush_errors.carbon:35:3: error: name `undeclared2` not found [NameNotFound]", " undeclared2;", " ^~~~~~~~~~~", "", "fail_flush_errors.carbon:43:3: error: name `undeclared3` not found [NameNotFound]", " undeclared3;", " ^~~~~~~~~~~", "", "" }, union diff (- expected, + actual):
=== diff in expected elements 0 to 2:
+ fail_flush_errors.carbon:22:3: error: name `undeclared1` not found [NameNotFound]
undeclared1;
^~~~~~~~~~~
=== diff in expected elements 4 to 9:
"undec\x6Cared2";
^~~~~~~~~~~~~~~~
+ fail_flush_errors.carbon:35:3: error: name `undeclared2` not found [NameNotFound]
undeclared2;
^~~~~~~~~~~
=== diff end
Stack trace:
0x55e476d29efd: Carbon::Testing::FileTestCase::TestBody()
0x55e476dbd1f2: testing::internal::HandleExceptionsInMethodIfSupported<>()
0x55e476dbcf57: testing::Test::Run()
0x55e476dbf0bf: testing::TestInfo::Run()
... Google Test internal frames ...
To test this file alone, run:
bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/driver/testdata/fail_flush_errors.carbon
testing/file_test/file_test_base.cpp:277: Failure
Failed
Autoupdate would make changes to the file content. Run:
bazel run //toolchain/testing:file_test -- --autoupdate --file_tests=toolchain/driver/testdata/fail_flush_errors.carbon
Stack trace:
0x55e476d2a5f0: Carbon::Testing::FileTestCase::TestBody()
0x55e476dbd1f2: testing::internal::HandleExceptionsInMethodIfSupported<>()
0x55e476dbcf57: testing::Test::Run()
0x55e476dbf0bf: testing::TestInfo::Run()
... Google Test internal frames ...
[ FAILED ] ToolchainFileTest.toolchain/driver/testdata/fail_flush_errors.carbon, where GetParam() = toolchain/driver/testdata/fail_flush_errors.carbon (93 ms)
```
Assisted-by: Google Antigravity with Gemini
Common case is going to be like:
```
Running tests with 64 thread(s)
Autoupdate can't discard non-CHECK lines inside conflicts:
......................!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!..!!!!!!!!!!!!!!!!!!!!.
```
->
```
Running tests with 64 thread(s)
toolchain/check/testdata/as/unsafe_as.carbon: Autoupdate can't discard non-CHECK lines inside conflicts:
......................!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!..!!!!!!!!!!!!!!!!!!!!.
```
(i.e., the conflict line was blank)
One error had the test name, I'm dropping it here, meaning:
```
................................................................................
Missing AUTOUPDATE/NOAUTOUPDATE setting: toolchain/codegen/testdata/assembly/basic.carbon
................................................................................
```
->
```
................................................................................
toolchain/codegen/testdata/assembly/basic.carbon: Missing AUTOUPDATE/NOAUTOUPDATE setting
................................................................................
```
For single-threaded runs, at the top there's already:
```
} else if (single_threaded) {
std::unique_lock<std::mutex> lock(output_mutex);
llvm::errs() << "\nTEST: " << test.test_name << ' ';
}
```
When an error diagnostic has an unattached location, for example because
the diagnostic points into a file that's in the prelude, use the next
attached location to position the error diagnostic's CHECK line. In
particular, if the error is followed by a note, use the position of the
note to determine where to place the error.
This exposes a general mechanism to do final fixups of the CHECK lines
to individual file_test binaries, which the toolchain's binary uses to
special-case error / warning CHECK lines.
Use it to dump the AST that includes a generated C++ thunk.
Based on #5917.
Also added printing of the full actual text when check fails to make
debugging easier.
Changed line replacement to allow removing complete lines.
Part of #5514.
When tests crash by stack overflow (and maybe other ways), they don't
print a stack trace so you don't have any way to know which file it was.
By running with --threads=1 you can figure this out, if we print out the
name of each test before we run it.
This prints each test name on its own line, then allows the autoupdate
sigil to be added to the end of that line:
```
TEST: toolchain/check/testdata/alias/basics.carbon .
TEST: toolchain/check/testdata/alias/builtins.carbon !
TEST: toolchain/check/testdata/alias/export_name.carbon .
TEST: toolchain/check/testdata/alias/import.carbon .
...
```
Instead of crashing when run outside of `bazel`, make the toolchain's
`file_test` binary work properly when no test-specific environment
variables are set. This makes it a lot easier to run `file_test` under a
debugger.
There are two main changes here:
- Don't crash if `$TEST_TMPDIR` is unset. Instead, fall back to LLVM's
temporary directory (typically `$TMPDIR`). We already did this in some
places in tests. We now do it in more places.
- Don't fall back to a target label of `<target>` in the reproduction
commands if `$TEST_TARGET` is unset, because this causes all the tests
to fail because their output doesn't match the expected output due to a
differing bazel run command. Instead explicitly specify the target from
the `FileTestBase`-derived class.
Infrastructure for this has been added generally, but only rolled out to
the toolchain `file_test` binary for now.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Trying to make it easier to skim test output for failures. Note this
excludes all the RUN and OK. Failing tests may still be verbose due to
the diff printed, but CHECK-fails should become short.
```
==================== Test output for //toolchain/testing:file_test:
Running tests with 128 thread(s)
...
Done!
[==========] 1272 tests from 1 test suite ran. (688 ms total)
[ PASSED ] 1272 tests.
================================================================================
```
Trying to make it easier to see possible bottlenecks.
Disabling hyperthreading seems like a significant reduction in
contention (15% improvement for me). Going down by half again reduces a
contention a little further, but not significantly from what I see. My
thought is that just flipping the flag is going to work best for people
cross-system versus a "divide by four", but welcome to other opinions
there. Note, I'm not digging into the source of the contention here,
just observing it.
Current default on my system (equivalent to `--threads=128`):
```
Running tests with 128 thread(s)
...
Ran 1272 tests in 3955 ms wall time, 397615 ms across threads
```
Disabling hyperthreads (equivalent to `--threads=64`):
```
Running tests with 64 thread(s)
...
Ran 1272 tests in 3520 ms wall time, 161957 ms across threads
```
`--threads=32`:
```
Running tests with 32 thread(s)
...
Ran 1272 tests in 3329 ms wall time, 69327 ms across threads
```
And for `./autoupdate_testdata.py --threads=64 --print_slowest_tests=5`:
```
Running tests with 64 thread(s)
...
Ran 1272 tests in 3417 ms wall time, 157946 ms across threads
Slowest tests:
- toolchain/lower/testdata/function/generic/call_recursive_basic.carbon: 1508 ms, 1484 ms in Run
- toolchain/lower/testdata/builtins/print_read.carbon: 1506 ms, 1506 ms in Run
- toolchain/lower/testdata/array/field.carbon: 1488 ms, 1487 ms in Run
- toolchain/lower/testdata/builtins/int.carbon: 1482 ms, 1475 ms in Run
- toolchain/lower/testdata/function/definition/params_one.carbon: 1472 ms, 1471 ms in Run
```
In test:
```
==================== Test output for //toolchain/testing:file_test:
Running tests with 64 thread(s)
...
Ran 1272 tests in 2968 ms wall time, 177732 ms across threads
Slowest tests:
- toolchain/lower/testdata/builtins/int.carbon: 1544 ms, 1533 ms in Run
- toolchain/lower/testdata/array/function_param.carbon: 1539 ms, 1537 ms in Run
- toolchain/lower/testdata/basics/zero.carbon: 1537 ms, 1536 ms in Run
- toolchain/lower/testdata/function/call/params_one.carbon: 1535 ms, 1534 ms in Run
- toolchain/lower/testdata/function/definition/params_zero.carbon: 1531 ms, 1531 ms in Run
[==========] Running 1272 tests from 1 test suite.
[----------] Global test environment set-up.
```
This narrows the scope of the CRC to try to get better behavior around
mutex lock releasing on crash. Closes#5042.
This breaks apart `ProcessTestFileAndRun` because we need to process the
test file for `SET-CAPTURE-CONSOLE-OUTPUT`. The test file processing
should more reliably not crash than the core `Run` logic though, so
should be reasonably safe to put outside the CRC.
Also support --threads=1 for disabling threading. This is the flipside
for me of reducing how much is in the CRC: make it easier to run on a
single thread if the CRC gets in the way of debugging. This also means a
typical copy-paste execution of a single test will be single-threaded.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Inconsistent execution environments make using a path-as-define
difficult, so switch to an embedded file.
Also fixes the lldb launch so that passing tests run cleanly, and adds
TEST_TARGET to gdb (but without testing there). I'm dropping `sourceMap`
because it's not handled quite correctly (also not great to be trying to
pass source mappings in two different ways), and `env` didn't seem to be
working as intended either; maybe specifying `initCommands` causes other
things to not be evaluated. But the straight `initCommands` looks like
it's working. I used lldb to validate execution of these changes.
```
Running initCommands:
(lldb) command script import external/+llvm_project+llvm-project/llvm/utils/lldbDataFormatters.py
(lldb) settings set target.source-map "." "/usr/local/google/home/jperkins/dev/carbon-lang"
(lldb) settings set target.source-map "/proc/self/cwd" "/usr/local/google/home/jperkins/dev/carbon-lang"
(lldb) env TEST_TARGET=//toolchain/testing:file_test
(lldb) env TEST_TMPDIR=/tmp
Running tests with 128 thread(s)
.
Done!
Note: Google Test filter = ToolchainFileTest.toolchain/check/testdata/const/collapse.carbon
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ToolchainFileTest
[ RUN ] ToolchainFileTest.toolchain/check/testdata/const/collapse.carbon
[ OK ] ToolchainFileTest.toolchain/check/testdata/const/collapse.carbon (0 ms)
[----------] 1 test from ToolchainFileTest (0 ms total)
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1 ms total)
[ PASSED ] 1 test.
Process 3869310 exited with status = 0 (0x00000000)
```
This takes the mechanism currently used for autoupdate and expands it to
the regular tests (deliberately trying to unify logic for
test/autoupdate/dump to deliver consistent behavior). I'm seeing about a
85% reduction in test time, though results will vary based on test
system.
This does some small edits to test output to make it fit better with the
new flow. Note I'm stopping printing of the "here's how to run" on every
test by default, since it's autoupdated into file content by default.
However, it's still there for test failures.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
`ValidateRun` is explorer-specific, so move out the error production to
be specific within explorer. This is related to other work I'm trying to
do which would make this require more special-casing to maintain; since
the toolchain doesn't need it, it's easier to drop.
I'm refactoring logic to try to make these files more manageable,
particularly as I'm looking at ways to use more threads. I'm renaming
FileTestBase::TestContext to TestFile and FileTestBase::TestFile to
TestFile::Split to try to be more consistent in how we talk about test
files and file splits in general. This change is not expected to change
any behavior, it's just refactoring.
This PR has two commits:
- Copying files for viewing deltas in GH
- Moving logic
The delta of the two commits is the main PR. The second commit can be
viewed on its own to see the delta versus file_test_base.* files where
the logic currently rests (there's still a reordering in run_test.cpp as
part of making one function static).
Small cleanup for code sharing. Note, `*ReadFile` will trigger a
CHECK-failure on error; the relevant implementations would previously
have failed silently (empty string).
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Adds @LSP-CALL and refactors LSP keyword handling to implicitly handle a
little more of the LSP structure. This is coming out of textDocument
call handling, where this at least reduces some boilerplate of
`"params": {...}`.
In order to write language-server tests, we need some way to pass stdin
input. This adds support for a split "// --- STDIN" which will be
provided as a temp file for testing.
Note this does more stdin -> input_stream style renaming, this is just
bugging me more since I know shadowing works but it can be subtle to
read, particularly since I'm now making direct use of stdin in a handful
of spots.
This adds a RawStringOstream. Versus TestRawOstream, which is
consolidated over to RawStringOstream, it uses a string for storage
instead of a vector, mainly to support move-to-string semantics. Versus
llvm::raw_string_ostream, it owns the string and supports pwrite (which
is needed for driver and its fd_ostream compatibility requirement).
This converts most uses of llvm::raw_string_ostream, leaving behind a
few in InstNamer that explicitly cannot own the string, such as:
```
llvm::raw_string_ostream(name)
<< "_" << tree.tokens().GetColumnNumber(token);
```
I have this as its own library so that it can use CHECK.
Yes this doesn't save much code, but it's code we repeatedly write.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Add `EXTRA-ARGS:` support to file_test, to add arguments without
overriding the default arguments. Use `EXTRA-ARGS: --no-dump-sem-ir` to
turn off SemIR dumping and thus SemIR testing in the int builtin tests,
which validate correct behavior through diagnostics instead.
This doesn't get us any closer to supporting more targeted SemIR dumping
/ testing, but this seems to be a generally useful feature anyway. Most
existing
tests using `ARGS` have been switched over to using `EXTRA-ARGS`.
Requested in review of #4716.
Disables three new warnings because they lean more towards style
conflicts than fixes. I've brought these up on #style.
Other than that, mostly fixing basic issues, and things that
clang-tidy-20 seems to fire where clang-tiday-16 didn't. One particular
curious case is `llvm::StringLiteral::data()` uses, which are flagged as
not strictly null-terminated; I'm switching to `const char*` in those
spots which matches `llvm::formatv`'s format argument, but feels worse.
I'm removing `run_clang_tidy.py` here because I'm observing it give
fewer warnings than `bazel build --config=clang-tidy -k
//toolchain/...`. The latter matches how we enforce in GitHub actions
(and also caches results, and suppresses output for files that have no
issues), so I'm dropping the bespoke script.
As discussed in #4530 . This required switching to using
`llvm::IntrusiveRefCntPtr` in a number of places.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
This avoids producing an LSan leak report for the objects that got
leaked by the crash, which would otherwise scroll all the useful
information about the crash off the terminal.
Since formatting covers comments, and the CHECK lines are in comments,
it can create recursive behaviors. This introduces AUTOUPDATE-SPLIT as a
way of formally designating a split to exclusively be used for
autoupdate output.
One of the things that ClangRunnerTest is doing is capturing
stderr/stdout because clang prints to it directly. This adds support for
that to FileTest.
I'm renaming the current `capture_output` field to `dump_output` because
the name is ambiguous after this change, and the flag is already named
`--dump_output`. It's still not great, but at least it's more distinct.
Note ClangRunner still doesn't use the vfs; that still needs work. I'm
just moving the NoArgs test over as a trivial test of the functionality.
This switches `DCHECK` and `FATAL` as well.
The goal is to reduce the code size impact of these assertions so that
we can keep more of them enabled. Currently, the largest cost I see from
`CHECK` is not the actual check or the cold code itself, but actually
the failure to inline trivial functions due to the presence of the cold
code. This means that our goal isn't to reduce apparent code size in the
final binary but the LLVM IR cost assessed for these routines in the
inliner, which closely correlates with code size but is a bit different.
As discussed in #4283, experimentation shows that a single function call
with a minimal number of arguments is the lowest cost model for these.
This is easily achieved with a format-string API that internally uses
`llvm::formatv`. This PR is essentially the `CHECK` version of #4283.
However, the check macros are substantially harder to make work with
both format strings and streaming because they also take a condition.
Also, unexpectedly, I was very successful at devising a regular
expression based automated rewrite from the streaming to the format
string form with only low 10s of manual fixes. This includes compacting
strings broken up across lines, etc. Given how well that went, I've
prepared this PR which just directly switches to the format string API
and migrate everything to use it.
One nice side-effect is that the format string approach ends up greatly
simplifying the implementation here as well.
This is ... *shockingly* effective. Parsing speeds up by more than 3%
with just this change. And checking speeds up by **8%** with this change
alone:
```
BM_CompileAPIFileDenseDecls<Phase::Parse>/256 86.3µs ± 1% 82.9µs ± 1% -3.94% (p=0.000 n=17+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/1024 431µs ± 1% 415µs ± 1% -3.76% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/4096 1.77ms ± 1% 1.71ms ± 1% -3.18% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/16384 7.44ms ± 1% 7.17ms ± 2% -3.56% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/65536 30.7ms ± 1% 29.7ms ± 1% -3.15% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/262144 131ms ± 1% 127ms ± 1% -2.81% (p=0.000 n=18+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/256 878µs ± 2% 800µs ± 1% -8.91% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/1024 1.88ms ± 2% 1.72ms ± 1% -8.56% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/4096 5.78ms ± 2% 5.28ms ± 1% -8.70% (p=0.000 n=20+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/16384 21.9ms ± 1% 20.1ms ± 1% -8.02% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/65536 90.4ms ± 2% 83.1ms ± 1% -8.04% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/262144 381ms ± 2% 352ms ± 1% -7.79% (p=0.000 n=19+19)
```
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
This opens the door for replacing all `library ...` lines in toolchain
test files with `library "[[@TEST_NAME]]";`. That's technically more
typing in a lot of cases, but OTOH means we can just do some copy-paste
boilerplate and stop carefully writing library names.
Also cleans up the test setup, because it's getting messy. I'm trying to
make it easier to see the divisions of tests and the output associated
with them. StringSwitch looked like a way to do this, with a few edits
to make it work nicely.
Discussed in the toolchain meeting today - we'd like to try having this
on by default and see if the cost isn't too high.
The nodebug test is a bit verbose, because it doesn't have the
`--exclude-dump-file-prefix` that test_file would usually add. Is there
a nicer way I could write this test to verify that --no-debug-info does
what it's meant to?
At present, I think if we crash from multiple threads in parallel, it
can lead to the stack trace not being printed out. Using
CrashRecoveryContext here seems to more successfully print a stack trace
on errors, which I'm hoping will ease debugging.
i.e., before:
```
-----------------------------------------------------------------------------
.Please report issues to https://github.com/carbon-language/carbon-lang/issues and include the crash backtrace.
Stack dump:
0. performing autoupdate for toolchain/check/testdata/alias/no_prelude/import_order.carbon
1. Program arguments: compile --phase=check --dump-sem-ir --no-prelude-import --exclude-dump-file-prefix=/usr/local/google/home/jperkins/.cache/bazel/_bazel_jper.kins/85deb7d9d96f7e0e80b42618a55969d7/execroot/_main/bazel-out/k8-fastbuild/b.in/toolchain/install/prefix_root/lib/carbon/../../lib/carbon/core a.carbon b.carbon
.CHECK failure at ./toolchain/sem_ir/ids.h:140: is_valid()
CHECK failure at ./toolchain/sem_ir/ids.h:140: is_valid()
external/bazel_tools/tools/test/test-setup.sh: line 328: 1151859 Aborted "${TEST_PATH}" "$@" 2>&1
```
(EOF)
after:
```
-----------------------------------------------------------------------------
Please report issues to https://github.com/carbon-language/carbon-lang/issues and include the crash backtrace.
Stack dump:
0. performing autoupdate for toolchain/check/testdata/alias/no_prelude/import_order.carbon
1. Program arguments: compile --phase=check --dump-sem-ir --no-prelude-import --exclude-dump-file-prefix=/usr/local/google/home/jperkins/.cache/bazel/_bazel_jperkins/85deb7d9d96f7e0e80b42618a55969d7/execroot/_main/bazel-out/k8-fastbuild/bin/toolchain/install/prefix_root/lib/carbon/../../lib/carbon/core a.carbon b.carbon
..CHECK failure at ./toolchain/sem_ir/ids.h:140: is_valid()
CHECK failure at ./toolchain/sem_ir/ids.h:140: is_valid()
......CHECK failure at ./toolchain/sem_ir/ids.h:140: is_valid()
CHECK failure at ./toolchain/sem_ir/ids.h:140: is_valid()
.....................................................................................CHECK failure at ./toolchain/sem_ir/ids.h:140: is_valid()
.CHECK failure at ./toolchain/sem_ir/ids.h:140: is_valid()
............................ #0 0x000056045ee22d7d llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/usr/local/google/home/jperkins/.cache/bazel/_bazel_jperkins/85deb7d9d96f7e0e80b42618a55969d7/execroot/_main/bazel-out/k8-fastbuild/bin/toolchain/testing/file_test.runfiles/_main/toolchain/testing/file_test+0x731dd7d)
```
(elided the full stack trace)
This has is a nice-to-have for me. Frequently I want to run a specific
test, and end up digging through output to be able to copy-paste the run
line. This uses TIP lines to inject the command into the file when using
AUTOUPDATE.
Note, one of the reasons I want this is because "bazel test
//toolchain/testing:file_test --test_output=all" has been regularly
exceeding bazel's output limit for me (workaround is either opening the
output file or specifying an obscure output limit flag), making it a
little harder for me to get the commands. However, frequently I'm adding
a file and want to iterate on it, so that's really the use case I have
in mind here.
The install directory contains the BUILD logic for creating an
installable tree of data files and executables for the toolchain, and
a library to facilitate toolchain code accessing the paths to their data
within this installation.
Then adds an installation of LLD in a synthetic LLVM installation, and
teaches the Clang runner to configure this and use it for linking
instead of the system linker.
Currently, the install paths only really manage access to the LLVM
binaries installed and used by the Clang runner for linking, but
eventually other data files like the prelude and runtime libraries will
be fleshed out as well. There are TODOs for moving more things over here
such as the prelude.
One interesting aspect of this is where to put helpers like parts of
LLVM in our install. This PR suggests nesting those files under
`lib/carbon`. While using a `lib` subdirectory isn't a perfect fit for
the FHS (Filesystem Hierarchy Standard), having a single location where
private data is collected is significantly superior to spreading them
across the system. This also matches similar patterns used by Clang
itself and several other language toolchains and standard libraries.
The install directory also provides a natural place for us to build out
packaging rules to create installable packages in various formats, but
that remains future work.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Right now, debugging a crash means errs never flushes: it's buffered in
the string. As a consequence, -v doesn't print anything. By stopping the
output capture in --dump_output, it prints directly for debugging.
Our testing had started to take more noticable time so I looked at where
the time went to see if it could be improved easily. Almost all of the
time was spent building regex matchers. That code path allocates a lot
of memory and does a lot of processing in order to make the regex fast
to run over the input. While that's not really a great tradeoff for how
we use these matchers, the bigger issue is that we don't need a regex is
the vast majority of cases.
This change inspects the pattern more deeply to avoid most of the work.
First, it looks for the
cases where the pattern doesn't require any adjustment at all and
directly builds a matcher from that if it can. This avoids an extra
string allocation entirely. This is probably only a minor improvement,
but it is also very easy.
The larger change is to process the string in two phases. First, we
expand the keywords and check for a regex region. If we find no regex
region, we can directly build a string equality matcher for the expanded
string. This still allocates an extra copy of the string but is
*dramatically* cheaper that building a regex.
Finally, if we *do* find a regex, we re-process the string to escape
everything and transform the regex sequence into its valid form before
building the matcher.
The result for me is an over 5x reduction in test time. Profiling
afterward shows a lot more opportunities for optimization here if things
get slow again. The actual toolchain code isn't really visible in the
profile yet.
Note, I was profiling the normal build, which has asserts and ASan and
such. I've not looked at the optimized build.
This handles toolchain failures per-file. The intent is to allow placing
both "success" and "fail" tests in the same file, using splits. However,
this PR only adds support and updates existing tests to continue
passing.
Update the compiler-rt patch (maybe this could be upstreamed, or try out
https://github.com/google/fuzztest?) and handle the ThreadPool ->
DefaultThreadPool rename.
When we always had a single file in the test file, we could run the
driver over it to see output. This adds the ability to do something
similar for multi-file tests. So for example in test output, there's
now:
```
[ RUN ] ToolchainFileTest.toolchain/check/testdata/class/fail_todo_import.carbon
To test this file alone, run:
bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon
To view output, run:
bazel run //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon --test_arg=--dump_output
[ OK ] ToolchainFileTest.toolchain/check/testdata/class/fail_todo_import.carbon (64 ms)
```
And here's what a run looks like:
```
$ bazel run //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/class/fail_todo_import.carbon --test_arg=--dump_output
(elided bazel output)
Executing tests from //toolchain/testing:file_test
-----------------------------------------------------------------------------
===============================================================================
= toolchain/check/testdata/class/fail_todo_import.carbon
===============================================================================
= stderr
===============================================================================
b.carbon: ERROR: Semantics TODO: `TryResolveImportRefUnused on ClassDecl`.
b.carbon: ERROR: Semantics TODO: `TryResolveImportRefUnused on ClassDecl`.
b.carbon: ERROR: Semantics TODO: `TryResolveInst on ClassType`.
b.carbon: ERROR: Semantics TODO: `TryResolveInst on ClassType`.
b.carbon:18:29: ERROR: Name `d_ref` not found.
var d: (ForwardDeclared,) = d_ref;
^~~~~
===============================================================================
= stdout
===============================================================================
--- a.carbon
(eliding semir output)
}
===============================================================================
= Exit with success: false
===============================================================================
Done!
```
This also switches from PrettyStackTraceFormat to
PrettyStackTraceString. This is partly so that we can share the produced
command (avoiding two different format strings corresponding to the same
value), but also it's in my mind to step away from
https://github.com/llvm/llvm-project/pull/77351.
Richard Smith
Jon Ross-Perkins
Boaz Brickner
Dana Jansens
Thomas Köppe
josh11bandJosh L
David Blaikie
Jordan Rupprecht