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
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)
```
We had a long discussion of this, so trying to document what seems to be
the conclusion... and also clean up the exceptions that I could find.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
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>
Noted CopyNameFromImportIR while glancing around (this one's interesting
because it's NameId, not void nor auto), did a scan just for a few other
cases. Not an exhaustive fix, and TBH assuming we'd prefer `auto ... ->
auto` since equivalent Carbon syntax would probably be `fn ... -> auto`
`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).
This should make it easy to add more tests which need the specific
behavior here. It also isolates where we reach into GoogleTest's
internals to a single common place.
Christopher Di BellaandCarbon Infra Bot
Chandler Carruth
Geoff Romer
Jon Ross-PerkinsandRichard Smith
Boaz Brickner
Dana Jansens
Thomas Köppe
DavidLoftusandJon Ross-Perkins