Even with `--benchmark_dry_run`, the benchmarks that use _batching_
still do one batch at a minimum as that's inherent to how batching works
in the benchmark framework.
This means that the minimum batch size can (and in practice does)
trigger timeouts by forcing 1k iterations in the test run that is just
trying to ensure the benchmark doesn't _crash_ in some way.
Reduce the minimum size to 128 instead of 1k for this benchmark which
should put it (much) further from any timeout limit. It also still seems
perfectly effective for getting good benchmark data -- I think the
original value was set _much_ too aggressively.
This adds just enough debug info for i32/int parameters and return
values, with a path forward for adding DWARF type metadata for other
types.
As it happens, return type information is carried separately from
parameter information:
* Return type information is carried in the `type` of the `DISubprogram`
(as a `DISubroutineType` - which does carry parameter type information
as well, but that's unused when the DWARF is emitted by LLVM)
* Parameter information is carried by `DILocalVariable`s with a non-zero
`arg` value (representing the order of function parameters)
In the absence of locations for the parameters (future work), nothing
would usually keep the `DILocalVariable` live/reachable when emitting
DWARF - so for cases where this can happen (for clang, this happens in
optimized builds where all references to the parameter variable might be
optimized away) the variables can be "retained" in a list on the
`DISubprogram` - achieved by passing `AlwaysPreserve` parameter to
`createParameterVariable` (adds them to a list, then that list gets
attached to the `DISubprogram` when it's finalized later)
For now, any unsupported types are emitted as `void*` (except void
return, which is implemented as void) as a placeholder.
Given this example:
```
import Core library "io";
class MyClass {
}
fn Unsupported(v: MyClass) {
}
fn Ret() -> i32 {
return 42;
}
fn Arg(x: i32) {
Core.Print(x);
}
fn Run() {
}
```
this is the resulting DWARF:
```
DW_TAG_compile_unit
DW_AT_name ("test.carbon")
DW_TAG_subprogram
DW_AT_name ("Unsupported")
DW_TAG_formal_parameter
DW_AT_type (0x00000066 "void *")
DW_TAG_subprogram
DW_AT_name ("Ret")
DW_AT_type (0x00000062 "int")
DW_TAG_subprogram
DW_AT_name ("Arg")
DW_TAG_formal_parameter
DW_AT_type (0x00000062 "int")
DW_TAG_subprogram
DW_AT_name ("Run")
DW_TAG_base_type
DW_AT_name ("int")
DW_TAG_pointer_type
```
And the debugger:
```
(gdb) p Ret()
$1 = 42
(gdb) p Arg(4)
4
$2 = void
```
I'm not sure if there's a way this logic should be merged with the logic
for making the `llvm::Function` type (which the `DISubroutineType`
building code was inspired by/copied from) - since they're done at
different times/places, I don't think there's an easy way to do it in
one pass, but maybe the code can be shared (even if it's run twice) in
some generic `SemIR::Function` type walker.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Define rules for `extend` declarations (`extend require`, `extend impl
as`, `extend base`, `extend adapt`) that say the target scope they name
must be complete at the point of the declaration. Define completeness
for a facet type to include all interfaces and named constraints that
provide unqualified name lookup through the facet type.
---------
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
We already did this translation in the other direction, but we had no
mapping from `Optional(T)` to anything, so round-tripping a nullable
pointer from C++ through Carbon and back to C++ was previously rejected.
Right now, the impl lookup can both fail to resolve the specific
definition because it's symbolic, and return a "final" constant because
it's a `final impl`. This is adding an instruction to help ensure the
specific is resolved.
The constant evaluation is fully recursive, but I'm not adding a TODO
since that's a known issue with impl lookup in general.
Enum constants in a macro replacement list are recognized only when
prefixed with “::”.
There is a `todo` test to make explicit that this still needs to be
fixed.
When prefixed with a global scope “::”, they are correctly found and
evaluated to a const.
Part of #6303
Don't attempt to defer overload resolution by creating a
`CppOverloadSet`; this was incorrect as we weren't saving the complete
clang::OverloadCandidateSet, resulting in template candidates not being
found. Moreover, saving the overload candidate set would be expensive,
as the representation is surprisingly large, and is unnecessary since
we're about to build a call.
In passing, improve the diagnostics for overload resolution failure to
use Clang's operator overload resolution messages rather than its call
overload resolution messages.
This fixes calls to templated operator overloads, which is the final
piece needed for us to successfully compile an iostream-based "Hello
world" program.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
I was trying to figure out the right way to get specifics to be added to
the work.
Technically, we could keep the pending_specific list; this is taking a
different approach of inserting inside the work stack, which will do
extra work moving entries, although typically that should be expected to
be small. One challenge of `pending_specifics` is that if we would need
to shift them to work after both `Done` (for immediate processing) and
`Retry` (for processing after the current instruction is later revisited
and done). That feels kind of awkward as additional tracking to do.
Also, the common case is probably that there's either 0 or 1 specifics
being added, so an additional vector may be significant overhead. That's
why I leaned more in this direction of just inserting them in the vector
of work.
Identifying a facet type is an operation on a pair of (self type, facet
type). It substitutes that self in as the `Self` of any require
declarations in order to form the set of (self type, SpecificInterface)
pairs that constitute the requirements of the IdentifiedFacetType.
Currently we don't pass around any self type, and assume all require
declarations are written against `Self` but this will change in the
future.
By contrast, type completion is done in the abstract and does not form
specifics for the require declarations. The purpose of type completion
is to enumerate the scopes where name lookup can occur and ensure they
are completed.
With this change, type completion is:
- No longer built on top of identification for facet types.
- Recursively ensures all `extend` scopes are complete since name lookup
can find symbols in them.
We add some test cases that demonstrate consistency between a resolving
the specific of a generic class, and a generic interface/constraint,
both used in a type position. In all cases, an invalid specific is not
materialized for the type completion when the specific's arguments are
used in a non-extend context. But they specific is materialized and
checked for type completion when in an extend context (extend impl or
extend require).
Type completion itself does not need to recurse into named constraints
or interfaces as the `extend require` declarations require the type to
be complete immediately, just as for `extend impl` in a class.
We had a test (`fail_incomplete_where.carbon`) with `impl as J where
.Self impls K` and `J` is incomplete, which used to be diagnosed but no
longer is, because we don't require non-extend interfaces to be complete
in type completion, nor in identification. The test was trying to test
the presence of rewrite constraints though, which it didn't even use. So
we remove the diagnostic that we can't hit anymore and replaced it with
a TODO, and add a test that should reach that TODO once qualified
rewrite constraints work.
This builds the archive and checks relevant symbols are defined. While
here, this refactors the runtimes test to share much more code between
the different runtimes.
Last but not least, this adds a convenience type-def for the libunwind
runtimes builder.
There is an inconsistency between how we spell things as `Libunwind` or
`LibUnwind`. We should canonicalize on the former as it matches the
underscores and other things we will spell in this space. I'm not fixing
existing spellings in this PR but will send follow-ups for those.
For now, treat such classes as being final, since we can't correctly
derive from them.
This removes the last category of C++ class that we are entirely unable
to interop with, and is a prerequisite for interop with C++ iostreams
(which have a virtual base class).
For example, when developing against a checkout of LLVM, it is useful to
be able to consistently pass an override flag to Bazel for that
repository.
This lets:
```console
bazel test --override_repository=+_repo_rules+llvm-raw=$HOME/src/llvm/llvm-project //toolchain/...
```
and
```console
./scripts/create_compdb.py --extra-bazel-flag=--override_repository=+_repo_rules+llvm-raw=$HOME/src/llvm/llvm-project
```
Share the same Bazel cache and use the same flags.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
About the same # of LOC, but maybe less work to analyze correctness?
Versus the template, could also stamp that out in the helper function
and still avoid the duplication of calls before/after HasNewWork.
Similar to how I've left `rewrite_constraints`.
Alternately I'm also kind of tempted to rename GetLocalSpecificInterface
and GetLocalSpecificNamedConstraint to instead be overloaded functions
(or to provide overloaded versions), which would allow this to drop the
function type parameters. But, naming is hard.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This test started failing with #6405, but it wasn't caught by our PR
testing or the merge queue as the test didn't _appear_ to be impacted by
the change (I think).
When run explicitly, as the post-commit actions do, it started failing
because of the new dependency edge.
This clarifies that the CC1 logic is directly extracted from Clang.
There are probably some other places in the toolchain we should extract
code like this where we're replicating and customizing logic from LLVM,
but wanted to start here.
This continues work to eliminate pending generics/specifics and get them
to be interleaved with instruction imports. I'm trying to use
`FinishGenericOrDone` here as a way to help ensure that code correctly
handles generics, where the simple alternative would be for each
`TryResolveTypedInst` call `SetGenericData` directly (but which might
make it easier to call the wrong `ResolveResult` function, and we do
need the `GenericId`s to be passed).
Avoid using a large switch that needs to be manually extended when
adding a new kind of instruction. Instead, the expression category for
an instruction is now specified when defining the `InstKind`.
In passing, add a distinct expression category value for patterns. This
isn't used for much except some error checking at the moment, but it
keeps the number of instructions that we need to manually classify as
`NotExpr` despite having a type very low.
Replace all unexpected instruction ids in a line, not just the first
one. Otherwise you get something like this:
```
// CHECK:STDOUT: impl @<null name>: <unexpected>.inst{{[0-9A-F]+}}.loc20_6 as <unexpected>.inst6000002E.loc20_11;
```
This is just an incremental step towards removing pending logic. The
rest seems like it'll be more complex due to interdependencies (I've
been poking at behavior).
Following #6357, map C++ `void` to a prelude class type
`Core.CppCompat.VoidBase`, not to a builtin type. This is mostly just
moving logic around, but does notably change `Cpp.void` from being an
incomplete type to being a complete-but-abstract type.
Also change `NullptrT` to be an adapter for `void*` instead of `()*`, to
follow the approved design.
Implicit conversions to `void` and to `void*` are still absent.
Part of #6280.
Since `ValueStore` now separates its id and value types as two template
parameters, we can use a `ValueStore` of `optional<ValueType>` as the
storage instead of a `SmallVector`.
Otherwise the value fails in confusing ways while untagging:
CHECK failure at ./toolchain/base/value_store.h:71:
index >= initial_reserved_ids_: When removing tagging bits,
found an index that shouldn't've been tagged in the first place.
With this change:
CHECK failure at ./toolchain/base/fixed_size_value_store.h:112:
id.index >= 0: instFFFFFFFFFFFFFFFD
This is part of trying to rewrite pending specific/generic code to make
use of the standard constant resolution flow. The LoadImportRef code was
a particular sticking point due to the recursion it does, which makes it
difficult to adapt over.
We add tests showing that `ImplStore::GetOrAddLookupBucket` is doing the
wrong thing for impls of a named constraint, as the impl-file
redeclarations of impls in the api file are not getting flagged as such.
To do the right thing requires us to be able to get the constraint from
a require declaration with the specific of the named
constraint/interface applied, which is future work as described in the
[open discussion
notes](https://docs.google.com/document/d/1Yt-i5AmF76LSvD4TrWRIAE_92kii6j5yFiW-S7ahzlg/edit?tab=t.1ji9ixn9bbnn#heading=h.kijomnov90rz).
Every test that used `addr` before #6283 should be using `ref` after
this PR. In most cases that was done in #6283, but this PR transitions a
few that I missed in that first pass. In addition, #6283 cloned the old
`addr` tests from `foo.carbon` to `foo_addr.carbon` in order to maintain
test coverage during the transition; this PR removes those cloned tests.
The to_array was mainly needed for zip_equal, and the
GetBlockAsTypeInstIds is forming a vector that should also be size two.
But just writing this out should avoid memory allocations.
Of course, then I'm like "but maybe a lambda or function would be
clearer than a for loop"... So the second commit.
This helps at least lldb handle calling functions (currently the debug
info describes every function as `void()`, so no parameters or return
values are supported) - seems gdb and lldb both depend on demangling to
varying degrees in C code (marking a function as "prototyped" in C in
DWARF does seem to also address this problem).
Given:
```
fn PrintThree() {
Core.Print(3);
}
```
Before:
```
(lldb) p PrintThree()
error: Couldn't look up symbols:
PrintThree
Hint: The expression tried to call a function that is not present in
the target, perhaps because it was optimized out by the compiler.
```
After:
```
(lldb) p PrintThree()
3
(lldb)
```
This is the first real step towards building libc++ itself, and fleshes
out both the core runtimes management logic and the archive-based
runtimes logic for a quite simple runtime.
Nothing here causes us to _use_ libunwind, and in fact this doesn't
include even the "on-demand" aspect of building `libunwind`. Instead,
this just wires it up to the explicit `build-runtimes` subcommand for
simple testing. The full integration along side the target directory is
future work.
Previously, the Clang runtimes building only considered building the
target resource directory, and was only _internally_ asynchronous.
Because the asynchrony was only internal, it could use the function
frame as a context object throughout the build of the resource dir. This
is simple but doesn't generalize well to more runtimes: if we want to
add 2 or 3 more runtimes, we want them to _all_ build asynchronously.
That means using some asynchronous builder that maintains the context
and allows them to proceed concurrently with other work.
This also factors all the runtimes building code into a separate set of
files. These aren't separate libraries at this point due to the
`ClangRunner` in some cases wanting to build runtimes on-demand, but it
at least lets us organize the code more cleanly.
Because this splits code between `clang_runner.*` and
`clang_runtimes.*`, it also works to update the `#include`s for both to
be roughly accurate. I used ClangD's include cleaner for this and it
probably also did some latent cleaning as it went, but that's the reason
for the churn of `#include` lines.
The archive building is also factored out into a re-usable helper. This
is a bit "over factored" in this PR, but supports the next PR that uses
the same code to build archives for other runtimes.
This also overhauls the synchronization used -- it uses a simple `Latch`
construct introduced in a previous PR to coordinate between the steps of
building the runtimes.
Last but not least, it factors the "enable leaking" state out of a
boolean in the runner to a parameter. This is important in the face of
concurrent calls as otherwise toggling this boolean can create a race.
The next PR will layer building more runtimes on top of this new
factoring.
---------
Co-authored-by: David Blaikie <dblaikie@gmail.com>
There are two uses I'm not converting here, that seem to want the
"shortest" behavior. For everything else, I'm going to `zip_equal` since
it's more restrictive.
I wish `zip` were named `zip_shortest`.
This fixes the flakiness caused by reuse of inode values when refreshing
stale cache entries by keeping the relevant directory open even as it is
unlinked from the filesystem.
It does this in two places, as technically we had the same flakiness in
two tests. However, the second test was broken and not testing what it
intended to due to confusing off-by-one naming and a typo. I've tried to
improve the naming, removed the typo, and added the parallel flakiness
fix.
This test was also egregiously slow because we ended up building too
many runtimes and trying to prune stale runtimes while holding a file
lock on _all_ runtimes -- a scenario that is not what the code was
designed for in the first place. Fixing that makes the test go from 10s
to 1s in runtime, and makes it much easier to test for flakiness.
Now appears to pass 100% of the 10k runs I did.
Closes#6168
Move `GetWithDefault` into the `ValueStore` base class, and avoid doing
the tag -> index mapping twice.
Call `ValueStore::Get` instead of `ConstantValueStore::GetAttached` in
`GetUnattachedConstant`. This is equivalent, since we never need a
default value here, and should be faster and less surprising.
Chandler Carruth
Ivana Ivanovska
David BlaikieandDana Jansens
Jon Ross-Perkins
Geoff Romer
Boaz Brickner