The YAML test helpers didn't use the `Printable` abstraction in one
place and instead directly used `<<` with a `std::ostream`. This matches
the `require`s expression in the `error_test_helpers.h` printing logic
for `ErrorOr`, but fails to provide the necessary implementation for
`llvm::formatv` to succeed with the `Yaml::Value` type.
The main fix is to use `Printable` and to define the `Print` method in
terms of `llvm::raw_ostream`. We already have all the mapping hooks in
place to also support `std::ostream` when needed based on that
definition.
This also adds some constraints to the printing in
`error_test_helpers.h` so it is a bit less under-constrained and more
understandable when it is correctly being used. These are just tidying
though, they aren't what makes these headers work together.
I've added a test to try and make sure these test helpers compose as
well.
If `Self` is not in the self type, then it must be an argument to every
interface required by the declaration. Specifically, this means the
interfaces in the identified facet type, and does not matter if `Self`
appears in the arguments of named constraints.
Fix the diagnostic to stop saying "constraint" incorrectly. And improve
clarity by including in the diagnostic which interface it found without
`Self` as an argument, since it may be found in some other named
constraint, rather than directly in the facet type as written.
Previously we only allowed conversions from `void*` to `U*` this way,
requiring casting via `void*` to get from `T*` to `U*`. That seems like
an unnecessary circumlocution.
OwningArrayRef is being removed upstream, per
https://github.com/llvm/llvm-project/pull/169126. This replaces uses
with `SmallVector`.
I've also made a separate commit which does init changes; these aren't
strictly necessary, but I added to make it a little more idiomatic in
spots.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Add the required facet type as an extended scope of the containing
interface/named constraint, and teach name lookup to look for extended
scopes in named constraints.
This makes name lookup work properly when the facet type does not have a
specific that involves `Self`. Support for `Self` needs further work in
another PR.
Note that when an _interface_ requires another interface, this PR lets
us find the name, but we still fail to find a witness for the interface
named through `extend require`, and this is future work. For a named
constraint, things work correctly as the identified facet type chases
through the named constraint and includes the required interface, so
impl lookup is able to provide a witness.
An interface A requiring another interface B means that an impl of A
must verify that the self-type also impls B. The instructions created
from this can involved a lookup that the self-type impls A, which end up
finding the impl being defined. This is not problematic of itself, but
it is problematic if these lookup instructions become part of the impl's
generic definition. When we find a specific of that `impl as A` during
impl lookup of A, and we resolve the specific definition, those lookup
instructions are replayed. Doing so does another lookup for `impl as A`,
which creates an infinitely recursive loop.
To break this loop we move the lookup instructions done to verify that
the self-type impls B outside of the definition of `impl as A`. This
prevents them from being specialized. But it doesn't prevent us from
diagnosing monomorphization errors properly. They just get diagnosed at
the use of that invalid specific, instead of inside the verification of
`impl as B` in the definition of `impl as A`.
This gets us a step closer toward resolving TODOs in member access
around facets, by making the lookup into a facet value a "lookup in
base" operation instead of a "lookup in type of base". However the base
given to find scopes in still remains the facet type of the facet, which
is still a TODO.
Then we can simplify the "lookup in type of base" case a bit, with a
single code path doing the name lookup step. But we keep a TODO where if
the type of base is a facet, we change the lookup target to be the facet
type of the facet instead.
This is toward having name lookup into an interface that is extending a
named constraint work correctly with a `Self` in its specific. To
perform that name lookup, we will need to tell name lookup what is the
base, so that it can replace `Self` with the base. This change gets us
in a position where we can correctly provide the base in the `T.F()`
(lookup in facet) and `t.F()` (lookup in type of facet) correctly and
straightforwardly.
We provide a marginally improved diagnostic when looking into a facet
with an incomplete facet type, which will move into
AppendLookupScopesForConstant once we are looking into the facet
directly instead of its type.
Some module metadata changed - because rather than linking one module
with one module metadata value (eg: PIC Level 0, or unspecified) and one
module with a different one (PIC level 2, in clang) - we use Clang's
Module as-is, no merging required, so Clang's module metadata sticks
rather than being merged with default values from Carbon.
Also tweaked the name we use for Clang's module name so it matches the
carbon file name.
Otherwise the IR changes seem to be just reorderings - C++ interop goes
first, then Carbon, rather than the other way around.
This requires re-working our config features to be usable in
feature-level `requires` clauses in addition to `with_feature_set` by
always including all of the features, but controlling whether the
features are enabled or disabled based on the target.
This is a little more verbose in the config features, but lets us use
them more widely and is a bit more principled.
This lets us use a single undconditional feature for linking with flag
sets that are enabled based on the underlying OS. While here, tidy up
the feature names a bit.
The diff here may look really bad without aggressive whitespace
ignoring, but none of the contents of the two flag sets changed --
they've just be indented more and placed into a single list.
Now the CPU flags feature can be unconditionally added as part of the
optimization features and another of the conditions in the main
configuration goes away.
The failure to pass these to links was probably harmless, but it's
better to include it there as well.
This removes another chunk of platform-specific feature construction and
simplifies the code further.
Also removes a now-stale comment about adding more platform-specific
features.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This PR merges the OS-specific Clang flags into the main Clang flags
features using feature-based constraints instead of separate features
conditionally added. Similarly for libc++. This also move flags to more
correctly live in the Clang flag set vs. the libc++ flag set as some of
these flags were specific to using libc++.
To make this change, the libc++ feature needs to be computed rather than
being fixed, as we need to add search paths based on the installed
location of LLVM and Clang.
All of this only works when the OS-config flags work. The earlier PR
adding these had a bug -- _none_ of the OS features would ever be
enabled. This didn't result in a problem as the initial use was only to
_disable_ flags on the wrong OS. Now that we're enabling flags, we have
to get it right by marking all of these as `enabled`.
The goal is to clarify that tool-generated submissions are fine, but
emphasize the requirements we have on the operators of these tools. The
inspiration for the two aspects emphasized comes from the discussion
around an update to LLVM's policy in
https://github.com/llvm/llvm-project/pull/154441, and in Fedora's
policy:
https://docs.fedoraproject.org/en-US/council/policy/ai-contribution-policy/
I've not used those policies _exactly_, as I think we may want somewhat
simpler and less formal guidance, but the goal is to remain
directionally aligned.
That said, I'm not attached to the current iteration of the wording, it
still feels a bit excessively formal or wordy to me. Suggestions on
wording improvements very welcome in addition to thoughts and feedback
on the overall direction.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Dana Jansens <danakj@orodu.net>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Members of `std::string_view` can't be accessed directly, because that
type maps into Carbon's `str` type (`Core.String`), so member access
doesn't find the C++ members. But they can be named via qualified name
lookup into a derived type. That crashed because we didn't expect the
non-Cpp type `Core.String` to be the parent of a Cpp-imported member.
Plus add some more test coverage for related cases (not involving `str`)
that already worked.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This leaves behind project-specific features such as the system header
management of our dependencies and the fancy cache management string.
No expected changes here, but yet another slightly different order of
flags.
This introduces the first pieces of a cleaner way to configure toolchain
components on target dimensions: dedicated features for those target
dimensions.
With that, we extract a `libcxx_feature` that can always be present but
disables its flags on unsupported targets.
With `-stdlib` in its own feature, move `-std=c++20` to not require
a variable but directly live in the flags.
This should enable us to extract the largest remaining feature into its
own file cleanly by removing dynamic configuration of it, along with
libcxx.
Further refactoring of target-specific logic will follow in its
footsteps.
The key changes are:
- Function output parameters are now prefixed with `out`, and more
consistently formatted as named parameters.
- Function and inst output arguments are now written as part of the inst
form, rather than as one of the inst arguments.
As a drive-by fix, this also changes `Temporary::storage_id` from
`DestInstId` to `InstId`, because it doesn't represent an output
parameter of the `Temporary` inst itself.
See the review of
[#6532](https://github.com/carbon-language/carbon-lang/pull/6532) and
[this Discord
discussion](https://discord.com/channels/655572317891461132/999638000126394370/1458268977020141589)
for additional background.
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.
- Distinguish attached vs. unattached constants.
- Add some missing value stores to the top-level output.
- Add missing fields to various Print methods.
I've had these kicking around for a year but never got around to pushing
them. They seem to cover a few things that previous examples didn't, so
I think we may as well include them.
---------
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Identifying a facet type takes both a self and facet type as a pair, and
then encode the self into the IdentifiedFacetType. This makes a
constraint that requires some _other_ type implements an interface
visible in the IdentifiedFacetType. And it will help to enable facet
types with `where T impls Z` for `T` that is not `.Self` in the future.
IdentifiedFacetTypes are now stored in a CanonicalValueStore instead of
a RelationalValueStore as they key is the combination of self and
(declared) facet type together now.
When the self-type is a facet value (has type FacetType) this is most
straightforward. But when it's a type we need to construct a FacetValue
to construct a specific for a require decl, to replace the generic
binding of the symbolic `Self`, which has type FacetType. To do so, we
make a FacetValue with an empty FacetType (equivalent to TypeType). This
prevents any looking for witnesses through the FacetType, which matches
what you can get from a type directly, requiring witnesses to come from
finding an `impl` decl.
Add additional InstNamer logic for such empty facet types so they print
as `<typename>.type.facet` if possible instead of as just `facet_value`.
This adds the necessary parser infrastructure to recognize and parse
lambda expressions in Carbon.
Key changes:
- Added and Parse Node Kinds.
- Updated to use to accommodate the growing number of node kinds.
- Implemented parser states and handlers for lambda syntax ( or ).
- Added structure to .
- Added diagnostics for missing lambda bodies.
- Added a stub in phase to defer semantic analysis using .
- Added parser tests for lambdas.
This moves the simplest parts of the toolchain config into separate
files. These parts are either unparameterized or trivially parameterized
and so easily extracted from the main file.
I tried to minimize the interesting edits here, but wasn't _completely_
successful I'm afraid. I'll try to describe them.
First, all of the interesting content of the new files is copied and
re-indented, no interesting edits were done.
The main file sees some more significant edits in order to realize this
refactoring:
- Extract the feature array building to a helper method.
- Collapse some extraneous features as there was no where to extract
them.
- Restructure how the array itself is built to support building it using
array fragments from the various files.
The only interesting semantic change I'm aware of here is that this
somewhat changes the order of command line flags in compiles and links.
The previous order was "fine", but not especially logical. I've tried to
more logically have features that should "override" or are "more
specific" come later here. However, that results in a slightly different
ordering. None of the current features had any flags that overlap, so
this should have no behavior change other than the changed flag order.
This is only the first step, however. There remain complex features in
the main configuration that I want to move out. However, to make those
moves simple requires some significant changes to how these remaining
features work and so I wanted to break them out. I've tried to leave
TODOs that can help as breadcrumbs on the parts of this refactoring that
aren't yet complete.
The comments also are mostly what we already had. I'm happy to try and
add some, but not sure how much I can cover as there is a _lot_ of code
here that I'm just moving around. Please let me know if there are
particularly places that would benefit from comments.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
The `install_paths` library depends on the `llvm_tools` library, which
depends on all of LLVM in order to allow _invoking_ the LLVM tools in
addition to listing and manipulating them. The `install_paths` also
depends on the Clang version number which for some reason depends
transitively on a large fraction of LLVM. That should probably be fixed,
but we don't actually need it anyways, we can just prune our dependency.
Because the digest builder is built in the _exec_ configuration, this
was pulling in most of LLVM and Clang to build in the exec configuration
as well, adding about 4000 actions or a roughly 30% overhead to complete
rebuilds. The time impact is likely closer to 2x because many of the
slowest actions are here.
Hopefully this makes our bots take much less time when rebuilding.
Background:
https://docs.google.com/document/d/1wi85FRiWh4X9A-gCYMVGKR40-q5fM6-3JaSpePk-XCY/edit?usp=sharing
And specifically this work is essentially an alternative to #5543
Clang's code generation is implemented through an ASTListener
(clang::CodeGenerator) that is attached throughout Clang's
parsing/sema/code
generation phases and acts on Clang AST incrementally throughout that
process.
Prior to this patch, Carbon has only created the CodeGenerator during
Carbon's
`lower` phase, missing out on key callbacks that would be made by Clang
during
`check`. Some of these issues were addressed by #6237 and #6483 - but
there were
still remaining cases where the delayed processing lead to missing
functionality.
With #6483 much of the Clang code that made multithreaded complexity of
#5543 is
no longer present, and we have access to the point of ASTListener
registration
so we can register the CodeGenerator there and consume its resulting
llvm::Module during lower.
Examples of some of the bugs this addresses are seen in the linked doc,
and
checked in as tests in this change in
`clang_code_generator_callbacks.carbon`
An indicental bug that's also fixed, and caused all the other test case
churn,
is that the `CodeGenerator` created during `lower` wasn't getting passed
the
Clang `CodeGenOpts` and was creating its own default - so, most notably,
optimization flags were not respected. This meant that the LLVM IR from
Clang
was always -O0 style IR (optnone, no inlinehint, no TBAA, etc). With
this
change, now the Clang IRGen gets the real `CodeGenOpts` and respects
optimization/other flags specified there.
This is only meant to be a rough proof of concept - I'm totally open to
reworking this in any way (even quite substantially) if folks have ideas
about
how this should be implemented most generally/elegantly/etc.
The IdTag knows the type of the Id its tagging and the type of the Id
being used as the tag. This prevents mixing up tagged and untagged ids,
and avoids having to work with untyped integers.
Adds an Untagged marker struct that's used as the tag type in IdTag when
no tag is desired.
The complexity of ConstantIds and TypeIds became a bit visible: TypeIds
are concrete ConstantIds. And ConstantIds have two different tagging
schemes, one for concrete and one for symbolic ids. And ConstantIds are
actually re-cast InstIds with the same index. The LoweredTypeStore needs
to work with tagged TypeIds, but the tags actually come from an InstId
store in ConstantValueStore. Now this is expressed in the type system by
getting the tags for TypeIds from the ConstantValueStore.
ValueStores without an TagId type parameter are now visibly untagged.
IdTag is now only default constructible when it does not have a tag,
which means ValueStore is only default constructible when the TagId is
untagged. This forces tagged value stores to be constructed correctly
with a tag at compile time, and untagged ones to be constructed without.
FixedSizeValueStore has overloads for dealing with tagged and untagged
Ids, since it can't default-construct ValueStore for tagged ids, and no
longer requires passing in default-constructed tags when there is no tag
in the ids.
The mangled name of the global init function is the same for all files
in a package, so giving it external linkage results in link errors if
more than one file in a package has global initializers. We never need
to refer to it from outside the file, so give it internal linkage.
This also requires that we stop eagerly emitting a declaration of it --
if it's empty, we don't emit a definition, and LLVM doesn't allow us to
emit an undefined declaration of an internal linkage symbol.
These checks include a full check that a red-black tree satisfies its
invariants on every erase. This leads to
`llvm::DWARFDebugAranges::construct` becoming quadratic in the number of
debug symbols in the binary, which means that in `-c dbg`, symbolization
of backtraces is astronomically slow, and in practice never completes.
(I left it for over 12 hours and it did not finish.)
Reduce the libc++ hardening mode from *debug* to *extensive* to turn off
the checks that have unbounded performance impact.
Previously, we used the FHS "prefix" concept as the basis of the
install, but this makes it hard to integrate an installed toolchain with
Bazel (or similar) build system where it wants the "root" of the
toolchain to have some specific files (`MODULES.bazel` or
`BUILD.bazel`), and cannot reference anything outside that directory
tree.
An easy solution is to make the `lib/carbon` directory the root of the
install and never walking up from it. Then we simply have a `bin/carbon`
symlink to the busybox that is useful for getting the command into the
PATH, but isn't used for anything else. The FHS-constrained install
paths surround a root we fully control the layout and files within.
While initially motivated by trying to make a single toolchain structure
that works both for installation and for Bazel, it actually makes the
paths we end up using in the toolchain much simpler. We no longer have
awkward `.../lib/carbon/../../lib/carbon/...` sequences in the toolchain
which is cleaner and even a (trivial) efficiency gain.
As I was doing this I noticed several out-of-date comments that I tried
to fix, and I tried to improve some code reuse rather than re-computing
paths.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Chandler Carruth
Dana Jansens
Richard Smith
Ivana Ivanovska
Jon Ross-PerkinsandDana Jansens
Geoff Romer
David Blaikie
Burak Emir