With this enabled, entities that live in value stores are poisoned
whenever any action is taken that might invalidate pointers and
references to those options -- in particular, adding another item to
that value store, or attempting to load any entity from an import IR.
Subsequent uses of those pointers or references then trigger an ASan
failure.
This detects latent bugs where the pointer or reference to the entity
would become stale if we got unlucky about when the value store
reallocates, even in cases where the reallocation didn't actually
happen.
This is not enabled by default: it finds a lot of latent bugs, so our
tests don't pass with this option. This PR also includes fixes for a few
of those bugs.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
If a function contains instructions whose locations are in another file,
skip providing debug locations for those instructions rather than
CHECK-failing.
This happens when emitting a thunk where the signature is declared in
one file and the call target is in another file: some parts of the thunk
use the original signature as their locations, whereas other parts of it
use the location of the call target.
Non-type values were being represented as "Concrete" in the type
structure's shape, which was incorrect when the value was symbolic. Now
they are represented as "Concrete" or "Symbolic" as appropriate.
When concrete, a matching concrete ConstantId is stored in the type
structure's concrete values, so that they will be used for equality
comparison. And then impl matching comparison is taught to look in the
concrete values for mismatches when it finds a "Concrete" or
"ConcreteOpenParen" shape on both sides of the comparison, and return
false if they are not equal. This reduces the number of candidate impls
selected in impl lookup, which avoids doing type deduction against impls
that won't match anyway due to the concrete values in the query not
matching concrete values in the impl. As a result we see fewer specifics
being generated in the semir.
The TypeStructureBuilder recursively iterates through a type, interface,
or facet value and constructs a type structure that includes each
concrete and symbolic value found. This iteration is a more general
thing that can be useful elsewhere. For instance, to get just the root
type out of a general type, it is the result of the first iteration
step.
We abstract out the iteration logic into a SemIR::TypeIterator to create
a clear boundary between the work of iterating and the work of building
the TypeStructure from it.
Along the way this pointed out some issues in the TypeStructureBuilder
where it could have ambiguity between types that include non-type
values. So we add some tests for these cases, and they now pass. There
are also TODOs left behind, as concrete TypeStructure is overly specific
right now in order to keep these tests passing, which means that the
concrete elements can't be used for impl lookup matching yet. Only the
shape of concrete vs symbolic is used for now, and then type deduction
is used to compare the actual concrete types, which could be skipped
when the concrete values could be compared directly and reject an impl
for not matching.
#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.
The current behavior hits UBSAN and ASAN issues.
Note, `RequiresNullTerminator` is already set to `false` in
`source_buffer.cpp`; setting it in `compile_helper.cpp` is making things
more consistent. The related logic is an [assert
fail](https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/MemoryBuffer.cpp#L52).
This was fuzzer-discovered.
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>
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>
This is fixing a deduce crash, with regression tests added in
binding_pattern.carbon. In `needs_substitution`, it adds the
`BindSymbolicName` with the compile time bind index corresponding to the
wrong generic scope, which causes a bad result. It appears
`needs_substitution` logic is no longer needed (per zygoloid,
`CheckDeductionIsComplete` handles related issues) so can be removed.
This changes the order of IR in use_assoc_const.carbon but the result
appears equivalent to me.
This was a fuzzer-found crash.
```
class C {
impl as I;
}
```
is redeclared
```
impl C.(as I)
```
for purposes of `match_first`/`impl_priority` blocks and definitions.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
`IRBuilderBase::SetInsertPoint` weirdly replaces our debug location with
one copied from the new insertion point, so undo its damage after
calling it.
Also included: a couple of cleanups I made while tracking this down.
Ran into this trying to debug #5428
Before:
```
lex.cpp:793:21: runtime error: null pointer passed as argument 1, which is declared to never be null
string.h:90:51: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lex.cpp:793:21
```
After:
```
lex.cpp:793:21: runtime error: null pointer passed as argument 1, which is declared to never be null
string.h:90:51: note: nonnull attribute specified here
#0 0x5572db22a680 in Carbon::Lex::Lexer::MakeLines(llvm::StringRef) /proc/self/cwd/toolchain/lex/lex.cpp:793:14
#1 0x5572db227ee4 in Lex /proc/self/cwd/toolchain/lex/lex.cpp:738:3
(etc)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior lex.cpp:793:21
```
Interestingly, even though this is labelled as UB, it's using the
ASAN_SYMBOLIZER_PATH (specifically not LLVM_SYMBOLIZER_PATH). But
canonically UBSAN_SYMBOLIZER_PATH may also be used per
https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/ubsan/ubsan_flags.cpp#L53,
so I'm adding it to the list out of an excess of caution.
Also doing some small related cleanup:
- Removing `ASAN_SYMBOLIZER_PATH` from `--test_env` because it should
now be getting overridden by these settings (also was a little
inconsistent in that `LLVM_SYMBOLIZER_PATH` was not included).
- Improving the environment construction and documentation.
The canonical location of the instruction may be an entirely different
instruction, which the instruction in question was not imported from. In
particular, we shouldn't assume that we can use the constant value of an
instruction that the *location* of an imported instruction refers to as
the constant value of the imported instruction.
The only time we should be looking at the `ImportIRInstId` for a `LocId`
is when determining its location in some other file.
Fixes a crash when importing thunks (which can contain instructions
whose location points to an instruction in a differnt IR).
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
When two non-final impls have the same type structure (neither is a
specialization of the other), it is an error unless they are within a
`match_first` block. For now, we don't have `match_first` implemented,
so it's always an error.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
A final impl must be written in the same file as the root self type or
the interface. This provides tests that should fail but don't yet for
writing a final impl in a third file that defines neither, as well as a
blanket final impl over an interface outside the file that defines the
interface.
Fixes a compile failure with the new version, essentially:
```
external/+llvm_project+llvm-project/llvm/include/llvm/Support/FormatVariadicDetails.h:157:1: error: implicit instantiation of undefined template 'llvm::support::detail::missing_format_adapter<clang::LookupResultKind>'
```
Right now we construct `tree_and_subtrees_getters` a couple different
ways, it's just not obvious because one's abstracted in `check`. But
also, when formatting IR, we'll repeatedly do the `IncludeInDumps`
string check, which felt odd to me since it only needs to be calculated
once per IR.
This also shifts `CheckIRId` selection a little earlier, and in doing so
makes `CheckParseTrees` accept a sparse `units` argument. I actually
think this is a positive: it makes `CheckIRId` a little more stable
across possible command lines, when file loading fails (which is the
only time that a file will have a `CompilationUnit` but not a
`Check::Unit`).
Trying to build on the shared issue between these, I'm adding a
`MultiUnitCache` to store the calculated arrays. For the subtree
getters, this is very minor and avoids at most one incremental array
construction (moving logic out of `CompileSubcommand::Run` might be the
bigger benefit). For `include_in_dumps`, when dumping SemIR, this is
changing a calculation run once per entity (in each IR) to be calculated
once per IR (globally), i.e. O(M*N) -> O(N).
Note this seems to be marginal for performance of file_test:
- Before: Stats over 10 runs: max = 5.3s, min = 4.7s, avg = 4.9s, dev =
0.2s
- After: Stats over 10 runs: max = 4.9s, min = 4.7s, avg = 4.8s, dev =
0.1s
I was mainly thinking about this in the context of dumping SemIR ranges.
There, the impact may actually decrease because a range won't do any
cross-IR printing. But, I'm expecting to add another layer for whether
we're printing IR for a file, and that made the `should_format_entity`
callback stick out for me.
When we fill the witness table with errors, set the witness id to an
error too, which signals to impl lookups to not use the impl.
Make the use of the `Impl` from the store more consistent once it's been
added to the store (or known to be there already).
This consolidates Lex::TokenizedBuffer::DumpSemIRRange and
Parse::TreeAndSubtrees::TokenRange into a single InclusiveTokenRange,
also making the OverlapsWithDumpSemIRRange function take the new struct.
I considered switching to `llvm::iterator_range<Lex::TokenIterator>`,
but we often want to see if the range is size one. Using `TokenIterator`
just looked like it'd add a bunch of offsetting to make it work; I view
that as low-value overhead.
For example:
```
Lex::InclusiveTokenRange token_range = GetSubtreeTokenRange(node_id);
auto begin_loc = tree_->tokens().TokenToDiagnosticLoc(token_range.begin);
if (token_range.begin == token_range.end) {
return begin_loc;
}
auto end_loc = tree_->tokens().TokenToDiagnosticLoc(token_range.end);
```
would become:
```
llvm::iterator_range<Lex::TokenIterator> token_range = GetSubtreeTokenRange(node_id);
auto begin_loc = tree_->tokens().TokenToDiagnosticLoc(*token_range.begin());
if (token_range.begin() + 1 == token_range.end()) {
return begin_loc;
}
auto end_loc = tree_->tokens().TokenToDiagnosticLoc(*(token_range.end() - 1));
```
So I'm keeping the bespoke struct.
- Adds parent information as a single-pass calculation
- Moves off `GetSubtreeTokenRange` because it's O(N)
- Stops using the node's subtree when formatting a single instruction
- This excludes `%F.call` and the following `return`, for example,
because only parameters are marked for formatting
When checking whether we can use the function in an impl directly to
satisfy a signature in an interface, allow the parameter names to differ
between the two declarations.
Instead of building an eval block as a separate pass at the end of a
generic, build the eval block incrementally.
The larger change here is that asking for the type or constant value of
an instruction now always returns an unattached type or constant value,
in order to preserve the behavior that we previously achieved by doing
the rewrite to attached types and constant values at the end of handling
the generic.
This also incidentally fixes some subtle issues where attached types and
constant values would leak out into check and cause it to get confused
about differences between attached and unattached values. Check should
no longer see attached values except where it explicitly asks for them.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Don't import `ImplWitnessTable` into the `constants` block, because we
generally don't put `Unique` constants there. This matches the handling
of the other kinds of `Unique` constants. In order to keep the
instruction visible in formatted SemIR, add it to the `imports` block
instead.
Also fix a bug in the instruction formatter that resulted in
instructions in the `imports` block being omitted from the output if
they were only referenced by earlier instructions in the `imports` block
and by instructions in the `constants` block. This was already resulting
in some referenced instructions being omitted from the output, but also
occurred frequently for `impl_witness_table` instructions after this
change because it is common for the only reference to those instructions
to be from `impl_witness` instructions in the `constants` block.
I was thinking about this while working on parent generation on #5394
(which does a similar loop), it's just a simplification.
Also remove surplus spaces in the preorder print.
This prints instructions that are inside the range, and entities that
overlap with the range. Note this can lead to incomplete printing of
entity contents.
Once a concrete result has been found, it's not legal to write an `impl`
that would change the concrete result afterward.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Trying to improve resilience against failures such as:
```
INFO: Repository rules_jvm_external+ instantiated at:
<builtin>: in <toplevel>
Repository rule http_archive defined at:
/home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl:392:31: in <toplevel>
ERROR: /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl:137:45: An error occurred during the fetch of repository 'rules_jvm_external+':
Traceback (most recent call last):
File "/home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl", line 137, column 45, in _http_archive_impl
download_info = ctx.download_and_extract(
```
https://github.com/carbon-language/carbon-lang/actions/runs/14719625211/job/41311145495?pr=5379
It looks like GitHub currently has a high rate of these, which it
shouldn't, but also maybe we can do a little more to weather these
service issues.
To show flag behavior:
```
╚╡./scripts/run_bazel.py --attempts=5 --retry-all-errors :foo
Command ':foo' not found. Try 'bazel help'.
Retrying exit code 2 because it may be transient...
Command ':foo' not found. Try 'bazel help'.
Retrying exit code 2 because it may be transient...
Command ':foo' not found. Try 'bazel help'.
Retrying exit code 2 because it may be transient...
Command ':foo' not found. Try 'bazel help'.
Retrying exit code 2 because it may be transient...
Command ':foo' not found. Try 'bazel help'.
╚╡./scripts/run_bazel.py --attempts=5 --retry-all-errors query //... | wc -l
INFO: Invocation ID: ffdf9480-0245-442d-885f-ee91a3d86b68
Loading: 0 packages loaded
367
╚╡./scripts/run_bazel.py --attempts=5 :foo
Command ':foo' not found. Try 'bazel help'.
```
On the last run, [test (ubuntu-22.04,
opt)](https://github.com/carbon-language/carbon-lang/actions/runs/14737410033/job/41366888473?pr=5386)
has an example of this working:
```
INFO: Invocation ID: 3f276297-6dc5-4007-a333-dcadd4db55f4
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
WARNING: Download from https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz failed: class java.io.IOException GET returned 618 jwt:jwt-not-provided
INFO: Repository bazel_skylib+ instantiated at:
<builtin>: in <toplevel>
Repository rule http_archive defined at:
/home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl:392:31: in <toplevel>
ERROR: /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl:137:45: An error occurred during the fetch of repository 'bazel_skylib+':
Traceback (most recent call last):
File "/home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_tools/tools/build_defs/repo/http.bzl", line 137, column 45, in _http_archive_impl
download_info = ctx.download_and_extract(
Error in download_and_extract: java.io.IOException: Error downloading [https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz] to /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_skylib+/temp62784[869](https://github.com/carbon-language/carbon-lang/actions/runs/14737410033/job/41366888473?pr=5386#step:5:887)09382546624/bazel-skylib-1.7.1.tar.gz: GET returned 618 jwt:jwt-not-provided
no actions running
no actions running
ERROR: Error loading '@@rules_python+//python/extensions:python.bzl' for module extensions, requested by /home/runner/work/carbon-lang/carbon-lang/MODULE.bazel:147:23: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_python+/python/extensions/python.bzl:48:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_python+/python/private/python.bzl:17:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_features+/features.bzl:3:6: Encountered error while reading extension file 'globals.bzl': no such package '@@bazel_features++version_extension+bazel_features_globals//': no such package '@@bazel_skylib+//lib': java.io.IOException: Error downloading [https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz] to /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_skylib+/temp6278486909382546624/bazel-skylib-1.7.1.tar.gz: GET re
ERROR: Error loading '@@rules_cc+//cc:extensions.bzl' for module extensions, requested by https://bcr.bazel.build/modules/rules_cc/0.1.1/MODULE.bazel:12:29: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_cc+/cc/extensions.bzl:16:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_features+/features.bzl:3:6: Encountered error while reading extension file 'globals.bzl': no such package '@@bazel_features++version_extension+bazel_features_globals//': no such package '@@bazel_skylib+//lib': java.io.IOException: Error downloading [https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz] to /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_skylib+/temp6278486909382546624/bazel-skylib-1.7.1.tar.gz: GET returned 618 jwt:jwt-not-provided: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_cc+/cc/extensions.bzl:16:6: at /h
ERROR: Error loading '@@rules_python+//python/extensions:python.bzl' for module extensions, requested by /home/runner/work/carbon-lang/carbon-lang/MODULE.bazel:147:23: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_python+/python/extensions/python.bzl:48:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/rules_python+/python/private/python.bzl:17:6: at /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_features+/features.bzl:3:6: Encountered error while reading extension file 'globals.bzl': no such package '@@bazel_features++version_extension+bazel_features_globals//': no such package '@@bazel_skylib+//lib': java.io.IOException: Error downloading [https://github.com/bazelbuild/bazel-skylib/releases/download/1.7.1/bazel-skylib-1.7.1.tar.gz] to /home/runner/.cache/bazel/_bazel_runner/8f839eaeb716f9d034eabdfa7ebecdb0/external/bazel_skylib+/temp6278486909382546624/bazel-skylib-1.7.1.tar.gz: GET re
INFO: Invocation ID: e2062912-715f-47ec-a0bc-9d1b4fee9e5d
no actions running
no actions running
no actions running
no actions running
no actions running
no actions running
<root> (carbon@_)
Retrying a failure because it may be transient...
INFO: Invocation ID: 7d4ee250-882f-480d-9c8d-2d92e67462fc
Loading: 0 packages loaded
367
```
Boaz Brickner
Ivana Ivanovska
Dana Jansens
Jon Ross-Perkins
Geoff Romer