This doesn't split apart the current error type into one that tracks
location and one that doesn't, although that might be easier to do once
we have this.
Instead, this is primarily intended to support custom error types that
lazily materialize the error message in case that can be avoided by
completely handling the error. For example, many file system operations
are *expected* to produce errors even in the hot path and we don't want
to render `ENOENT` (for example) to a pretty string and instead will
directly query the error to understand and handle it in code.
The type parameter ordering isn't the most obvious, but helpfully allows
us to default the error type in a useful way.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
These have unique challenges for our hashing scheme, and so its useful
to make sure the hash functions we use can handle them.
Some other work on Abseil's hash tables uncovered that this might be
risky and may have surfaced some improvements to reduce the impact here,
but the first step seems to try and start covering this path in the
benchmarks.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
When finding an executable, this validates that the returned binary is a
symlink back to the same thing as /proc/self/exe, also using that as a
fallback for different things.
Looking back at #3912, we started using `findProgramByName` in order to
avoid path canonicalization done by `GetMainExecutable`. That created
issues as in #5096, wherein an `argv[0]` that's not explicit enough
(`llvm-symbolizer` instead of the full path, done in [LLVM's
Signals.cpp](https://github.com/llvm/llvm-project/blob/4f60f45130c6bd96c79e468fe9927a29af760f56/llvm/lib/Support/Signals.cpp#L198))
leads to incorrect results (finding an `llvm-symbolizer` in `$PATH`).
One option to fix this would be to patch LLVM to provide an absolute
path for `llvm-symbolizer`. However, I'll suggest that passing a
filename in `argv[0]` is not terribly uncommon, and could be a migration
limitation if we force it. The failure mode is also opaque; for example:
```
$ /bin/sh -c "exec -a llvm-symbolizer ./bazel-bin/toolchain/carbon"
error: expected carbon-busybox symlink at `/usr/lib/llvm-19/bin/llvm-symbolizer`
```
Combined with the `setenv` of `LLVM_SYMBOLIZER_PATH` in
`busybox_main.cpp`, this is intended to fix#5096.
Use it to replace most existing modernize-loop-convert lints with
range-based for loops. As requested in review of #5475.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
We already go to some effort to avoid moving these, but we end up still
moving them twice: once when adding to the worklist and again when
reversing a chunk of the worklist.
* To avoid a move when constructing the worklist, add an `EmplaceResult`
utility that allows the result of a function call to be emplaced into a
container.
* To avoid moves when reversing the list, stop reversing it. Instead of
reversing the list and popping tasks as we run them, we accumulate a
sequence of tasks for a deferred definition region, run them in the
order they were enqueued, then pop them all at the end. This will in
some cases increase the high-water-mark of the size of the worklist, but
not asymptotically. The same high-water-mark could be reached with the
old approach by reordering the declarations in the source file.
In passing, we no longer create `LeaveDeferredDefinitionRegion` tasks
for non-nested regions. We don't need them, because we can detect that
condition by our reaching the end of the worklist. This means that the
enter / leave region actions are now always in correspondence -- we only
create them for *nested* regions. The tasks have been renamed to convey
this.
We still move the suspended function states around if the worklist grows
to over 64 entries and gets reallocated. We could potentially address
that issue too by switching to a chunked allocation strategy as is used
by `ValueStore` and then make the tasks noncopyable, but I'm not
attempting that in this PR.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This reverts commit 1889ee3904.
We have identified that this is causing ODR violations, because the
`fuzzer` feature is being added `cc_fuzz_test` targets, and thus any
includes they make, but not to the rest of the build. Any include that
is seen from both places has ODR violations if it branches on
FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION.
We need to apply fuzzer globally when building fuzz targets somehow, or
not set different defines in fuzzer.
I'm trying to make the offsetting a little easier to understand, and
also get a better `requires` structure on calls. The second is for an
attempt to refactor the `Formatter` API, but also changing the `InstId`
`derived_from` requires seems helpful for clarity on what's really
happening.
Trying to make repeated `std::same_as` easier to write. Calling it
"concepts.h" because I figure we'll maybe have a couple more things like
this.
Was looking at this because I may add a couple more similar constructs.
The `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` flag is a standard flag
proposed by LibFuzzer that is meant to inform compiled code that it is
being built for fuzzing, as described here:
https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode
We add the flag to our `fuzzing` feature/config, and enable DCHECKs when
under fuzzing so that we can catch bugs that currently are caught on the
other side of DCHECK, even if they don't cause ASAN to trap a read/write
beyond the capacity of a value store.
Teach CARBON_KIND_SWITCH to handle mutable lvalues and rvalues, and
CARBON_KIND to forward along rvalues so that it's possible to write
`case CARBON_KIND(const T& t)`, `case CARBON_KIND(T& t)`, and `case
CARBON_KIND(T&& t)`, depending on the type that was passed to
CARBON_KIND_SWITCH.
Replace all uses of VariantMatch with their equivalent of a switch using
CARBON_KIND_SWITCH, and remove the VariantMatch helper from the
codebase.
The version of clangd/clang-tidy on developer machines has slowly
diverged from the one on the CI builders, which is causing a slowly
increasing amount of pain as clang-tidy CI runs fail (incorrectly) over
things that a newer clangd/clang-tidy was perfectly fine with locally.
This bumps the Clang version used in the ubuntu builders to 19, which is
the most recent in Debian stable.
We use https://apt.llvm.org instead of LLVM's GitHub releases
(https://github.com/llvm/llvm-project/releases) as the former more
reliably has packages for newer Clang/LLVM versions on x64. The
community-build releases binaries on LLVM's GitHub have stopped
including Ubuntu packages that match the GitHub x64 Ubuntu workers for
some time (for at least the 18 and 19 releases).
By moving to apt.llvm.org packages we only download and install the
headers and libraries needed for development, rather than every output
of building llvm, which is much faster and saves lots of disk space. We
also remove the system installations of other versions of clang/llvm so
we should end up using negative disk space. We can no longer easily
cache the installation but apt.llvm.org is a reliable end point.
We bump the ubuntu image version for the github workers to 24.04, as
apt.llvm.org has stopped building images for 22.10 in 2022 at its end of
life.
The `pre_commit` workflow disabled sudo unlike the other workflows that
install Clang/LLVM, including the `clang-tidy` workflow (which is also
run on `pull_request`). We bring it into alignment with the other
workflows so that we can install the llvm packages. And we lock its
ubuntu image to 24.04 so that it can be moved in lockstep with the other
workflows that depend on Clang/LLVM.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
#5445 updates to bazel 8.2.1, this does more updates (including to
buildifier, which does autofixes like the `sh_test` loads in the other
PR).
Note I'm using the latest available clang-format wheel. That's not
really something I expect people to have installed, but should mostly be
consistent. I'm specifically skipping clang-format 18 because it had
some broad regressions, and 19 got really confused by a `requires` on a
trailing return. Using the latest seemed probably okay since most people
won't see the difference. Do note that trailing returns in macros,
https://github.com/llvm/llvm-project/issues/47664, seems to be cropping
up again as an issue.
- 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 building the definition of a thunk immediately when we
generate the thunk declaration, wait until we reach the `}` of the
outermost class, interface, etc. -- at the same time when we would parse
the definition of the thunk if it were defined inline.
This fixes issues where we fail to define the thunk because it requires
an enclosing class to be complete, or its definition depends on
something declared later in the enclosing class.
Make the representation of a suspended function scope, and its
constituent suspended components, be move-only, and switch to passing it
around by rvalue reference instead of by value because it's expensive
both to move and especially to copy.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
`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]]`
This fixes a `copy constructor must pass its first argument by
reference` compilation error when compiled with a recent enough Clang
(after
https://github.com/llvm/llvm-project/commit/fe0d3e3764961b62f43f1b129f30aaec5f30bc16,
targeted for LLVM 21 release).
```
carbon/lang/common/set.h:81:59: error: copy constructor must pass its first argument by reference
81 | SetView(SetView<std::remove_const_t<KeyT>, KeyContextT> other_view)
| ^
```
This makes it friendlier in interactive debuggers. If you want to print
a value without a newline from code, you will have to be calling Print()
anyway since Dump() is private, and Print() does not add a newline.
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>
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`
When I open a .def file, there are often 4 errors:
- The #error
- The #define is not defined
- Missing `;`
- Identifier naming
This PR is meant to disable all of these, since they can be distracting
from fixable diagnostics.
Also makes the style guide explicitly comment on void, but this was the
intent IIRC because it matches Carbon's `-> ()` (and "always" versus
"except for void", which we definitely went back and forth on).
Includes adjusting function pointers, which I definitely forget this
syntax works sometimes.
Excludes utils/tree_sitter/src/scanner.c because it claims to be C, but
really we should probably fix that to be cpp.
### Context & Motivation
The error handling utilities in `//base/error.h` are very useful for
writing code with strong safety guarantees. While hardening the `Dump`
debug utilities (from review in #4866), I encountered a rough edge with
references and pointers. After a [brief Discord discussion in
#contributing-help](https://discord.com/channels/655572317891461132/1052653651895779359/1334675462877610038),
it was suggested that adding support for references to `ErrorOr` would
be a good candidate to move forward.
Using a reference type with the `ErrorOr` class (e.g. `ErrorOr<Node&>`)
produces two errors:
<ol>
<li><strong><code>variant can not have a reference type as an
alternative</code></strong>
<ul><li>From private field: <code>std::variant<Error, T>
val_;</code></li></ul>
</li>
<li><strong><code>'operator->' declared as a pointer to a
reference</code></strong>
<ul><li>From member function: <code>auto operator->() ->
T*</code></li></ul>
</li>
</ol>
### Changes
To support reference types, both errors are resolved:
1. `std::reference_wrapper` is conditionally used for storage when `T`
is a reference type
2. type trait aliases like `using ValueT = std::remove_reference_t<T>`
are used to produce compatible types for methods like `auto operator->()
-> ValueT*`
- Escape dumped token strings (what got me here)
- Change the quoting from backticks to quotes
- Also add a `FormatEscaped` helper function for this, updating other
`.write_escaped` uses
`toolchain/autoupdate_testdata.py --allow-check-fail` can now be used to
perform an autoupdate even if some `CARBON_CHECK`s are failing. What
this does will depend on how the toolchain behaves after the `CHECK`
failure, and of course there's no guarantees there, but this can be
useful if it's easier to debug the `CHECK` failure by looking at the
produced SemIR.
Internally, this uses `bazel build --config=non-fatal-checks`, which in
turn specifies a `--per_file_copt` for `check_internal.cpp`. The intent
here is that the rebuild required to enable or disable this mode is as
small as reasonably possible.
This mode is not compatible with `-c opt`, as it's important that check
failure calls are `[[noreturn]]` in `-c opt` mode.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Came up on #4831, style:
"For a type T whose values can be compared for equality, define a
non-member operator== and document when two values of type T are
considered equal."
https://google.github.io/styleguide/cppguide.html#Operator_Overloading
Note while we could put some of these out-of-line, it's helpful to keep
them inside the braces:
- For private member access
- For templated cases so that we aren't duplicating templates
- Very mild preference for keeping class's API documented within the
braces
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>
Both of these I noticed from testing #4785, but they occur at head.
```
(elided)/execroot/_main/common/raw_hashtable.h:532:40: error: do not use nested 'std::max' calls, use an initializer list instead [modernize-min-max-use-initializer-list,-warnings-as-errors]
532 | static constexpr ssize_t Alignment = std::max<ssize_t>(
| ^
533 | {alignof(MetadataGroup), alignof(StorageEntry<KeyT, ValueT>)});
| ~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| static_cast<long>(alignof(MetadataGroup)) static_cast<long>(alignof(StorageEntry<KeyT, ValueT>))
(elided)/execroot/_main/toolchain/install/busybox_info_test.cpp:259:8: error: unused local variable 'usr_prefix' of type 'std::filesystem::path' [bugprone-unused-local-non-trivial-variable,-warnings-as-errors]
259 | auto usr_prefix = MakeInstallTree(dir_ / "usr");
| ^
(elided)/execroot/_main/toolchain/install/busybox_info_test.cpp:260:8: error: unused local variable 'usr_local_prefix' of type 'std::filesystem::path' [bugprone-unused-local-non-trivial-variable,-warnings-as-errors]
260 | auto usr_local_prefix = MakeInstallTree(dir_ / "usr/local");
| ^
```
The std::max diagnostic seems a little confused, but the initializer
list seems like it can be dropped without any loss. The unused locals
diagnostic is correct.
Neither of these seem like they should be newer than my last clang-tidy
pass, maybe I just missed them in other sweeps.
Avoid misaligned loads from StaticRandomData in the size [4, 8] hashing
case. We can use aligned loads in this case for lower latency. We
introduce the SampleAlignedRandomData function for this purpose.
When using a byte-encoding for matched group metadata we need to mask
down to a single bit in each matching byte to make the iteration of a
range of match indices work. In most cases, this mask can be folded into
the overall match computation, but for Arm Neon, there is avoidable
overhead from this. Instead, we can defer the mask until starting to
iterate. Doing more than one iteration is relative rare so this doesn't
accumulate much waste and makes common paths a bit faster.
For the M1 this makes the SIMD match path about 2-4% faster. This isn't
enough to catch the portable match code path on the M1 though.
For some Neoverse cores the difference here is more significant (>10%
improvement) and it makes the SIMD and scalar code paths have comparable
latency. Still not clear which is better as the latency is comparable
and beyond latency the factors are very hard to analyze -- port pressure
on different parts of the CPU, etc.
Leaving the selected code path as portable since that's so much better
on the M1, and I'm hoping to avoid different code paths for different
Arm CPUs for a while.
---------
Co-authored-by: Danila Kutenin <danilak@google.com>
I was wondering, instead of treating `misc` differently and enabling
specific checks, maybe we can flip that since we actually seem okay with
most of the checks?
The main check I'm enabling, with significant edits here, is
`misc-no-recursion`. But maybe this is helpful to enable, even with the
necessary NOLINTs, since we want to avoid recursion in the toolchain?
This PR shows some example fixes in subst.cpp (which are more stylistic,
since the code shouldn't actually have recursed due to its structure; I
think we could remove the warning on TryResolveInst the same way). Some
also just don't seem worth fixing, like those in tests files (I didn't
see a way to exclude files in .clang-tidy, so instead I'm using
NOLINTBEGIN). But I think we might actually want to fix inst_namer, and
there's enough in convert that I didn't look closely.
Also, I made some protected -> private style fixes based on
`misc-non-private-member-variables-in-classes` (this is also how I
noticed `class Real` versus `struct Real`). With node_stack, it looks
like the `protected` wasn't even used. [Per
style](https://google.github.io/styleguide/cppguide.html#Access_Control),
data members should be private outside tests. But since we can't
trivially exclude `protected` members in tests, I'm turning it off -- I
don't view it as offering enough benefit on the whole.
migrate_cpp issues are preexisting (I believe we just aren't monitoring
it), but changes there make `bazel build --config=clang-tidy -k //...`
work cleanly.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
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.
Given code like the following:
```
auto kind = ConversionTarget::Kind{0};
CARBON_CHECK(!loc_id.is_valid(), "hello {0} world", kind);
```
Currently we would print 'hello <the next line>', as the check string
would be treated as terminating at the '{0}', so it does not print the
rest of the string or a newline. This is because ConversionTarget::Kind
is an enum with underlying type `int8_t` which is a char, and
llvm::formatv does not look if the type is an enum and treat is
specially. So it prints it as a char rather than a number, which in this
case is a nul terminator.
With this change, the '{0}' value will be converted to a larger integer
before being passed through to llvm::formatv so that char-sized enums
will print as a number, and the result is that we will print 'hello 0
world\n' as the developer intended.
Chandler CarruthandJon Ross-Perkins
Richard SmithandJon Ross-Perkins
Thomas Köppe
Alexander Kornienko
Boaz Brickner
Calvin
ezbr
