`-Wunused-template` was added to `-Wunused`, so clean up the things it
found. One of them was a bug in the Clang warning that I've worked
around and reported upstream:
https://github.com/llvm/llvm-project/issues/218429
When a split file in a test unexpectedly succeeds or fails, include the
test filename in the error as well as the name of the split. This should
make it a bit easier to figure out which test failed from a failing test
log.
Rumdl already appears to have _significantly_ fewer bugs than prettier,
and a solid LSP for editor integration.
The tool is: https://github.com/rvben/rumdl/
I've separated out the change across three commits for easier review.
The configuration tries to match the existing formatting, the changes to
the all the files are to correct issues found by the new tool.
Assisted-by: Antigravity with Gemini
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Rearrange the file_test infrastructure so that we can customize the
mapping of file names to command line arguments. Map `module.modulemap`
files to corresponding Clang driver flags. In passing, also clean up the
interface for specifying custom argument replacements so that we don't
build a string map for each file we process, and stop using
`SmallVector::insert`.
Assisted-by: Gemini via Antigravity
Use `Label` to mark labels that are local to this module. Remove
workspace root when forming manifest. Add explicit import for name that
is not available implicitly in an imported module.
Assisted-by: Gemini via Antigravity
The benchmarks themselves aren't really specific to `driver`.
Keeping the source generation near to the primary use case of
benchmarking also seems like a more discoverable location.
I feel a little bad doing this reorganization right after I gave a talk
with links to a bunch of this code, but seems good to reorganize a bit
before doing some work to extend things now that we have full standard
library support for C++ benchmarking and other improvements.
Assisted-by: Antigravity with Gemini
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
This is so that the last file is more likely what we're trying to
compile in tests. Just splitting out the churn-y change of reordering.
Assisted-by: Google Antigravity with Gemini
The Carbon style guide prefers `const` to be on the left wherever
possible, and also has a de-facto standard for specifier order. Since
the order of specifiers and qualifiers tends to become a part of
muscle-memory, deferring the checking of this to tooling should lift a
small burden on both contributors and reviewers.
---------
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This brings some fixes:
- The handling of `zlib` and `zstd` are much cleaner
- Three of our patches are no longer needed
This also includes the fixes from #6562
It also moves us from `zlib` to `zlib-ng` which is a much better basis
for what we want, and likely makes our toolchain faster when generating
debug info at least.
It fixes another API change in terms of which headers provide the
`createInvocation` we use.
Lastly, it cleans up the deps test to correctly recognize the wrappers
for `zlib-ng` and `zstd`, as well as improving the documentation for why
we allow dependencies on them.
Also consolidate on using `//bazel/cc_rules:defs.bzl` where appropriate.
Also update a couple of Bazel modules deps of `@rules_cc` to the latest
versions.
When building in Bazel actions, notably building runtimes, using
absolute paths makes the results non-hermetic and generally less
cache-friendly.
This restructures the code to only form an absolute path as part of the
`bazel run` change of working directory. It also tries to make the API
for doing this a bit more clear by taking the `exe_path` and
transforming it internally.
To support this, this PR also generalizes the `RemovingDir` to support
relative paths. While these can be tricky -- the working directory needs
to not change while they exist -- that isn't a reason to fully exclude
them and they're useful for implementing relative-path runtimes, etc.
Previously this was kept in `//toolchain/install` so it would be near to
the code that actually defines the installation layout. However, that
creates somewhat unfortunate dependency cycles between
`//toolchain/install` and other directories. Exacerbating this, a
subsequent PR is likely to add dependencies on it from
`//toolchain/base` itself that suggests that is the correct layering.
This PR just moves the code mechanically with as few other edits as
possible.
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 << ' ';
}
```
Add a content keyword to file_test, `[[@0xAB]]`, that expands to the
code unit 0xAB, and use that instead of putting raw malformed code units
in test files.
Instead of printing the raw input bytes in snippets in diagnostics,
replace non-printable characters with <AB> in the output, being careful
to still compute the location of the caret and underscore properly.
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 diagnosing a problem with C++ code imported from Carbon, include
the location of the specific `import Cpp` statement that imported the
C++ code as part of the backtrace, rather than providing a location in a
generated file that doesn't exist on disk.
Also fix handling in autoupdate of check lines that contain multiple
file name and line number pairs to use the matched file name for
remapping of locations rather than the first file name in the line.
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 .
...
```
This builds a mechanism for the toolchain to construct more complex
min_prelude files, which in turn should allow the toolchain to stop
special-casing the min_prelude directory. For example, instead of:
```
args.insert(args.end(), {"--custom-core",
"--exclude-dump-file-prefix=include_files/"});
```
This should allow (in a `min_prelude` file):
```
// EXTRA-ARGS: --custom-core --exclude-dump-file-prefix=include_files/
```
Then when that min_prelude is included, it'd be used.
But also, this should allow sharing between min_prelude files with use
of `INCLUDE-FILE`, which as we make progressively more complex
min_preludes might become useful.
- Updates incompatible flags.
- `rules_flex` is no longer used, so enable its flag.
- Fixes `sh_test` deps for
`--incompatible_disable_autoloads_in_main_repo`
- Broadens the exception for `rules_cc` and `bazel_tools` due to changes
to runfiles deps; trying to avoid minutiae that shouldn't affect the
decision.
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.
```
`FindIfOrNull` returns a pointer to the element in the range if it's
found, and nullptr otherwise. `FindIfOrNone` returns a copy of the
element in the range if it's found, and `T::None` (for a range of
elements of type `T`) otherwise. `Contains` returns a bool indicating
whether the element in the range is found.
These functions replace `llvm::find()` and `llvm::find_if()` when you
want a single answer back instead of an iterator. This avoids the need
to check against `end()`, allowing the return condition to be tested as
a standard bool.
We replace uses of `find()` and `find_if()` that did not require an
iterator with these new helpers.
Note that the return type of `FindIfOrNull` is a pointer since we can
not write `optional<T&>`, which must be tested for null. If the null
check is omitted, UB occurs and the resulting code may end up with an
incorrect pointer (https://crbug.com/40153300) into the range (or
elsewhere), rather than a null dereference. And this would be very
confusing to debug. Hopefully debug builds and sanitizers keep this from
being an issue we sink a bunch of time into debugging.
Rules executed by bazel don't necessarily have the right environment to
find the symbolizer, which was the intent of `cc_env` setting
`LLVM_SYMBOLIZER_PATH`. So far, this has kind of been a case-by-case
fix, but every so often I'm trying to debug a crash in a test that
doesn't provide it. Rather continuing down this route, instead add
drop-in wrappers for cc rules so that it's hard to forget.
Note `bazel/cc_rules` is intended to mirror `bazel/carbon_rules` and
`bazel/cc_toolchains`, rather than `@rules_cc`.
AFAICT there isn't a great way to add this as a default for the `bazel
run` environment. It's not typically going to be set on its own,
forwarding `$PATH` would be too broad, and the [action
`env_sets`](https://bazel.build/docs/cc-toolchain-config-reference#using-action-config)
I think are not quite what we need (I think those don't include output
execution, only compilation).
I was thinking about this after `seq` changes in #5182, and looked for
other uses that might be replaceable. Here's the resulting cleanup
around `seq`:
- Switch to `enumerate` or `zip` when possible.
- `int _` -> `auto _` (it's typically a `size_t`, but there's no reason
to cast when unused)
- Fix a case of cast style `(size_t)...` -> `static_cast<size_t>(...)`
- Switch `(void)close_children_count` to `[[maybe_unused]]`
It's helpful for stability to not have the path in the repo reflected in
test files, something I'm separately running into. So to reduce this,
align INCLUDE-FILE with other split behavior:
- Use the filename (with a "include_files/" subdir to disambiguate),
rather than the full path.
- Note if we eventually want to support splits in these, the same
approach could be extended.
- Only provide as an arg if the user requests files as args.
Factoring AddFile back because it's hard to share; I'm also advocating
to remove the prelude manifest, which would mean the remaining call
could be removed.
Currently file tests for LSP must provide carbon source code as an
escaped string within notify params, i.e.
```
[[@LSP-NOTIFY:textDocument/didOpen:
"textDocument": {
"uri": "file:/class.carbon",
"languageId": "carbon",
"text": "class A {\n fn F();\n fn G() {}\n}\n"
}
]]
```
This works fine for simple, single line files but gets annoying when
working with more complicated files which are necessary when testing
more complicated features e.g. goto-definition
```
--- class.carbon
class A {
fn F();
fn G() {}
}
--- STDIN
[[@LSP-NOTIFY:textDocument/didOpen:
"textDocument": {
"uri": "file:/class.carbon",
"languageId": "carbon",
"text": "AUTOFILL"
}
]]
```
This PR extends file_test_base to be able to parse the notify/call
params and inject files from the test_file's splits into the JSON input.
I purposely avoid using the clangd types and manually parse the
llvm::json::Value here to avoid introducing a depdendency on clangd to
the generic file_test_base, but happy to change if we think that is
fine. Also happy to accept other suggestions on alternative methods to
achieve same result.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
The INCLUDE-FILE option is only used in the toolchain tests for now. If
specified in a file test, the given file path is added to the test's
arguments. For toolchain tests this makes the file's package available
to the test. The `--custom-core` command line flag is added to the
driver, which avoids adding the production `Core` package to the command
line. Together, these allow a test to provide their own minimal `Core`
package.
For example, this would replace `Core` with the package and prelude in
`facet_types.carbon`.
```
// INCLUDE-FILE: toolchain/testing/min_prelude/facet_types.carbon
// EXTRA-ARGS: --custom-core
```
To support this:
* //testing knows how to parse INCLUDE-FILE out of the header of a test
file.
* //testing adds the file to the virtual file system, and includes it in
the test's arguments.
* //toolchain/driver grows the --custom-core command line flag to avoid
loading the production `Core` package.
Tests that were creating their own minimal prelude to define BitAnd on
types are now pointed to
toolchain/testing/min_prelude/facet_types.carbon as the prelude. They no
longer need to `import Core` in each test as a result.
Such tests are no longer `no_prelude`, but instead have their own
prelude. So they are moved to a `min_prelude` subdirectory.
Closes#5076
Chandler Carruth
Richard Smith
josh11bandJosh L
Jon Ross-Perkins
Christopher Di BellaandCarbon Infra Bot
Geoff Romer
Boaz Brickner
Dana Jansens
Thomas Köppe
DavidLoftusandJon Ross-Perkins