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.
Although this focused on `Destroy` support, some choices here around
`implicit_type_impls` are because copy/move will likely follow a similar
approach. I'm trying not to predict too much about how we'll structure
those, but I'm putting `Destroy` impl logic in a file that could perhaps
be shared with those. They'd likely be interested in similar things,
e.g. traversing members of types (particularly class, struct literal,
tuple literal).
At present this sets the destroy function as `no_op` which is consistent
with current logic, but has a TODO to correctly define.
Constant importing for functions changes slightly due to some issues I
was having with `GetFunctionType`. zygoloid suggested this approach to
avoid `EvalInst` logic.
Adds a flag for controlling whether to generating these impls. While
this does generation for `class`, as noted above this'll also need to be
done for tuples and struct literals, which would leave the `none.carbon`
min_prelude unable to use any types. Note if destruction *would* occur,
it'll still look up `Core.Destroy` for that and fail, but that's already
true of any test using `none.carbon`. I'm trying to use the flag to see
if we can keep `none.carbon` working mostly-consistently.
I'd tried separating out the flag to #5852, but that got a lot of
pushback over whether the behavior was appropriate. I'm hoping that the
interactions here make it clearer why the particular approach -- the
goal is not to enable advanced testing, or create some new end-user
behavior that we really support, it's just to keep no-prelude tests
functional. The main question raised there was why not just keep
generating `impl T as Core.Destroy` if `fn destroy` is present -- but I
think here it should be apparent that would require additional
complexity, as the generation of `impl T as Core.Destroy` is not
currently conditioned based on the implementation of `fn destroy`. I'd
rather add complexity to this flag only if it's enabling interesting
test functionality.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This is trying to make it clearer when vectors are being indexed with
`CheckIRId`.
The only one that I still kind of want to change is the
`SmallVector<std::unique_ptr<CompilationUnit>>`, but because it's a
`unique_ptr` that's a little more complex. I may not bother.
Note, some of the changes around nuanced `SmallVector` interactions were
based on trying to copy the way `SmallVector` itself takes arguments,
like with range passing.
Stop using the clang tooling library to build an ASTUnit; that library
is set up to process clang frontend arguments, assuming that something
has already built frontend arguments from the compiler arguments. It is
also too encapsulated and doesn't let us inspect and modify the compiler
invocation before it's executed.
Instead, build the AST unit directly in two phases:
* FIrst, take a list of clang driver arguments and convert them into a
list of compiler arguments, using `clang::createInvocation`. Internally,
this uses the clang driver to build a frontend invocation, including
building system-specific include paths as needed.
* Then, directly build an ASTUnit from this compiler invocation.
I've factored this so that we can split out the `createInvocation` step,
with the intention that we may want to move it out of check and into the
carbon driver with the rest of the driver-level argument handling, and
we may want to customize some of the clang options before we invoke the
clang frontend with that set of options.
In order to make the invocation reusable, it no longer depends on the
name of the carbon file importing the C++ code. In place of synthesizing
a header file name as `<foo.carbon>.generated.cpp_imports.h`, we now
insert line marker directives into the generated header so that errors
in that header cause Clang to point a diagnostic back at the Carbon
source file itself. This results in a minor improvement in the
diagnostic output: we no longer refer to a nonexistent generated file.
But the snippet still contains text that doesn't match the source code,
so it remains imperfect.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
By moving dumping, we can have dumping occur before verification that
might CHECK-fail (e.g. parse tree and llvm IR verification).
I'm dropping vlogging of raw semir. It was only done when dumping, so
`-v` would print zero copies and `-v --dump-raw-sem-ir` would print two
copies. The lack of complaints about this suggests it's not needed.
I'm making a small change to drop newlines between textual and raw
semir. This is an edge case so I don't expect people to really notice in
general, but it seemed unusually aware of what's on a stream, and it
made it harder to do the dump_stream/raw_dump_stream approach, which I
felt would be decent in general, since check is the only phase that can
emit two different things (which I could also just drop -- we don't
really use raw semir anymore, it doesn't seem like a big need to be able
to print it with textual semir, but I'm assuming to just maintain
existing behavior).
In parse, we were previously dumping the tree on verification errors.
I'm removing that because now `--dump-parse-tree` should work fine,
where previously it wouldn't.
I've been mulling this mainly for the parameter complexity of
check/lower, but doing lex/parse for symmetry.
I'm motivated by the plan to move dumping for all of them into the
respective functions, because of discussion about llvm-verifier. That
basically would add another bool parameter (or more) to each of these.
My instinct is we're going to probably accrue a little more over time,
so I'm suggesting this as maybe adding the boundary a little simpler
and/or easier to read.
Note it may make sense to refactor a little further, e.g. maybe
Lower::Context could receive the full set of options and pick out what
it wants, but I figured creating the struct itself would be a decent
start.
I'm trying to put things into options when we can produce a reasonable
default if the user doesn't assign a value. I'm using an explicit
constructor so that values can be added without affecting every caller.
A different factoring would be to pass in everything through the param
struct, but that just felt weird when I was trying it out.
Removing `inst_namer` and `module_name` from `LowerToLLVM` params --
both of these can be inferred from `sem_ir`, and I'm not seeing a
particular reason to maintain them at the call site.
Suggested by zygoloid while looking at #5678
```
CHECK failure at toolchain/lower/context.cpp:62: !llvm::verifyModule(*llvm_module_, &errs): Verifier errors: Instruction does not dominate all uses!
%.loc17_46.1.temp = alloca { i1, i32, i32 }, align 8, !dbg !13
%tuple.elem0.loc17_46.2.tuple.elem = getelementptr inbounds nuw { i1, i32, i32 }, ptr %.loc17_46.1.temp, i32 0, i32 0, !dbg !13
Instruction does not dominate all uses!
%.loc17_46.1.temp = alloca { i1, i32, i32 }, align 8, !dbg !13
%tuple.elem1.loc17_46.2.tuple.elem = getelementptr inbounds nuw { i1, i32, i32 }, ptr %.loc17_46.1.temp, i32 0, i32 1, !dbg !13
Instruction does not dominate all uses!
%.loc17_46.1.temp = alloca { i1, i32, i32 }, align 8, !dbg !13
%tuple.elem2.loc17_46.2.tuple.elem = getelementptr inbounds nuw { i1, i32, i32 }, ptr %.loc17_46.1.temp, i32 0, i32 2, !dbg !13
```
Adds a `--llvm-verifier` flag to be able to turn this off easily,
particularly for debugging the LLVM IR.
The call workaround is due to a verifier requirement `inlinable function
call in a function with debug info must have a !dbg location`. It
specifically comes up for the `++x` case, with `%1 = call i32
@"_CConvert.8b3d5d6a6c17be04:ImplicitAs.Core.b88d1103f417c6d4"(i32
%other)`. I think #5397 is in the direction of a fix for that, but #5397
was set aside because it puts the debug info in too many places.
Instead, address this by adding a stub location for calls that don't
have a good location. I'm deliberately putting this next to the TODO so
that it's easier to understand the association.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
The `full.carbon` prelude just sets a flag indicating an explicit intent
to include the full prelude. Once all tests include some prelude file,
an error can be enabled (currently it's commented out) that requires an
`INCLUDE-FILE` of some min-prelude to be present in all `check/` and
`lower/` file tests.
Factor out the logic for mapping from a `LocId` into a diagnostic
location from check into sem_ir so it can be reused by lowering. Include
the function and instruction being lowered in the pretty stack trace.
Example stack trace:
```carbon
2. filename: examples/sieve.carbon
3. core/prelude/types/int.carbon:213:3: lowering function Core.Op(Core.IntLiteral as Core.ImplicitAs(i32))
fn Op[addr self: Self*](other: Self) = "int.sadd_assign";
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4. core/prelude/operators/arithmetic.carbon:22:27: lowering call
fn Op[addr self: Self*](other: Other);
^~~~~~~~~~~~
```
To make it easier to identify crashing files when testing multiple.
```
(elided)
3. Check::Context
filename: duplicate_name_same_line.carbon
NodeStack:
(elided)
```
In preparation for lowering information from multiple `SemIR::File`s
into a single `llvm::Module`. The primary purpose of this is to support
lowering a local specific for an imported generic function, where the
instructions for the generic function are in a different file than the
instructions for the specific. See #5475 for a draft PR implementing
that functionality on top of this.
The per-`llvm::Module` state now lives in `Lower::Context`, and
`Lower::FileContext` tracks only the per-`SemIR::File` information.
`Lower::Context` should not mention any `SemIR` IDs that are
file-specific. For now, the C++ lowering and the specific coalescing
logic are kept per-file for simplicity.
This requires:
* Making `FunctionDecl` mutable since generating code
(`HandleTopLevelDecl()`) requires a mutable declaration and since we
manually add `used` attribute to force code generation.
* Passing the file system to `Lower` since it's needed by Clang code
generation.
* Creating an internal Clang LLVM module and link it against the Carbon
LLVM module.
Demo:
```c++
// hello_world.h
extern int puts;
inline void hello_world() {
((int (*)(const char*))&puts)("hello world");
}
```
```carbon
// main.carbon
library "Main";
import Cpp library "hello_world.h";
fn Run() -> i32 {
Cpp.hello_world();
return 0;
}
```
```shell
$ bazel-bin/toolchain/carbon compile main.carbon
$ bazel-bin/toolchain/carbon link main.o --output=demo
$ ./demo
hello world
```
Based on https://github.com/carbon-language/carbon-lang/pull/5406.
Part of #5405.
Right now, a lot of tests have started setting `--no-dump-sem-ir`. My
thought is that we can look at:
1. Put ranges in a bunch more files.
2. Shift more towards `--dump-sem-ir-ranges=only` instead of
`--no-dump-sem-ir`, because it allows mixing fail-tests with no IR
alongside tests that contain IR.
3. Evaluate switching the default to `--dump-sem-ir-ranges=only`, and
instead set `--dump-sem-ir-ranges=if-present` only in files that want to
typically show all IR (particularly import-related tests, where ranges
don't work well).
In real-world use, my thought is also that it'd be helpful to be able to
add the dump range comments to files, see the output (i.e., the default
behavior of `if-present`) but then also be able to pass `ignore` in
order to see the full IR without modifying the file (possibly also
useful in tests). That model is why I went for tri-state handling.
Note `only` can also have an interesting side-effect. Because core files
(including min_prelude versions) typically won't have ranges, they'd be
implicitly excluded.
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.
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.
Trying to make it easier to see the API at a glance. The class has
become really long, and this doesn't fundamentally change that, but
hopefully makes it easier to navigate. The entry structure also had some
cruft that I'm removing.
I'm trying to keep functions in the same order as they currently are.
The delta still looks unhappy because of the churn, but hopefully this
at least explains the ordering in formatter.h. You can try using the
"Add indent" commit on the PR to see a better before-after delta.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
The main goal of this is to collapse the LocId and SemIRLoc types into a
single type, eliminating the need for APIs to decide which to use. This
originated from discussion about UnwrapSemIRLoc in #5169. Although that
was removed in #5202, it's probably still a good direction for LocId.
This changes the packing of LocId to allow adding InstId, making it
tri-modal: ImportIRInstId, InstId, or NodeId. This has a side-effect of
reducing the available space for ImportIRInstId, although not by much
due to the pre-existing `ImplicitBit` behavior. If needed, we could also
probably play with packing a bit more since `ImplicitBit` really only
applies to `NodeId`, but I was trying to keep the logic a little
simpler. Note `TokenOnlyBit` can still apply to `ImportIRInstId`.
This leaves in place a typedef for SemIRLoc -- I intend to clean that up
separately.
Some Discord discussion is
[here](https://discord.com/channels/655572317891461132/655578254970716160/1353755830058745959).
What this really does is avoids shadowing names, so that we can
comfortable have things like `Check::DiagnosticEmitter` or
`Check::DiagnosticLoc` without shadowing being a concern.
Note, down this path I'm also thinking about:
- Renaming misc DiagnosticConsumer/DiagnosticEmitter classes, possibly
just to DiagnosticConsumer/DiagnosticEmitter (so
`Check::DiagnosticEmitter` instead of `SemIRLocDiagnosticEmitter`).
- Dropping `Diagnostic` from `Emitter::DiagnosticBuilder`.
- But not for `Check::DiagnosticBuilder`, because `Check::Builder` would
be ambiguous.
- Renaming diagnostics/diagnostic_* to drop "diagnostic".
[Discussion about SemIRLoc ->
DiagnosticLoc](https://discord.com/channels/655572317891461132/655578254970716160/1353771570463768698)
reminded me of this (in particular the older [Check::DiagnosticBuilder
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1344363562608627763)),
but I'd only do that rename if there's matching consensus about a path
forward where we keep SemIRLoc, and in a way that it's only ever used
for diagnostics (the divergence from which is at the root of current
LocId discussion).
I'm trying to keep that separate from a namespace addition for clarity.
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
ASTUnit is owned by `CompileSubcommand`, passed through `Unit` to be
populated in `ImportCppFiles()` and used via `SemIR::File`.
When generating the AST, pass `-x c++` args to compile C++ (temporary
until we pass args properly).
`Cpp` namespace is marked as a special namespace and has dedicated logic
in `LookupNameInExactScope()`.
The logic for importing declarations from C++ to Carbon is in
`import_cpp.cpp`, but we're likely to want to refactor this
signfiicantly over time as it grows (perhaps a dedicated directory?).
Part of #4666.
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.
At present, we typically define a DiagnosticConverter, then store an
instance of it and a DiagnosticEmitter that wraps it. This is relatively
minor in general, but I've been trying to create more self-contained
DiagnosticEmitter classes (which hold their own DiagnosticConverter,
similar to NullDiagnosticEmitter), and there it just gets in the way.
Since we don't reuse DiagnosticConverter instances, this combines the
definition into DiagnosticEmitter. Mainly this means we don't have a
separate object in play, and less to carry around.
The most impact is probably to SemIRDiagnosticConverter, which was also
the most complex. Now `SemIRLocDiagnosticEmitter`, this gets some
different construction flow. Note in the PR I've split the file rename
to its own commit, to try to help delta views. However, the most
substantial parts of the refactoring are split into #4876, which this
depends upon.
At present, lower depends on `Check::SemIRDiagnosticConverter` for debug
info. That was to support a quick implementation of debug info, but
isn't great because it's both an unusual dependency on check's
implementation, and relying on diagnostic structures for debug info.
This cleans that up by splitting relevant logic out to a library in
sem_ir, and having lowering use sem_ir's library instead of check's.
Additionally, a small refactoring of `Parse::TreeAndSubtrees` to allow
getting locations in lowering without going through a `DiagnosticLoc`.
I'm adding `Parse::GetTreeAndSubtreesFn` in because it's a complex
signature to have in so many spots.
I chose to have `ResolveNodeId` return a `SmallVector` because it seemed
likely to be fairly compact, but that could also be using an optional
callback to handle resolved node IDs, possibly just returning the last
entry. This could be switched if preferred.
Note this change shouldn't affect behavior, it's just moving code
around.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
I'm looking at eliminating `DiagnosticConverter`. This change removes
`NodeLocConverter` (albeit adding `UnitAndImportsDiagnosticConverter`),
and in doing so, refactors lex conversion functions to extract them out
from the `DiagnosticConverter` functions.
I'll be following up with changes that collapse `DiagnosticConverter`
logic into `DiagnosticEmitter` locations. The intent is that we
shouldn't need separate ownership of both types.
This switches most error printing to use diagnostics instead of direct
stream writes, even when not a specific file diagnostic. I'm allowing
empty filenames for this use-case.
This allows a little more specific testing to validate coverage of
output using the diagnostic coverage test. I'm adding a few tests to
cover things that weren't previously tested.
Separately, this also forces a little more standardization in format...
considering how changes like #4568 show effort being spent to _mirror_
diagnostic style, my thought is now to just use diagnostic code where
possible.
Note this also allows incrementally better testing of the language
server; I'm changing the crash fix from #4847 in favor of diagnostic
testing.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
The language server needs stdin, and for tests we should be passing it
around. My intent is to pass in a faux stdin to Driver for language
server tests.
As long as I'm adding a new parameter, I was looking at also changing
the way streams are passed in to Driver for style (pointers since
they're held past construction lifetime). Since these are all stored in
DriverEnv, I thought it might be a net improvement to use the struct
directly, getting more explicit parameter names and also removing the
need for `SetFuzzing`.
I'm trying here to avoid functional changes, but there are a couple
additional fixes like removing an obsolete `find_insensitive` and
refactoring how `ValidateOptions` handles errors (because it reduces the
number of spots that operate on error_stream).
Hi,
I fixed a small issue that I found inside the "compile subcommand"
component:
The program can be crashed by running ```bazel run -- toolchain:carbon
compile --dump-mem-usage "non-existing-file.carbon"``` - i.e. by
activating the memory usage dump flag and passing a non-existing file.
Best regards,
oz
The offsets were originally added to deal with churn from builtins in
the raw semir. In textual semir, we mostly see instruction IDs for
imports, and builtins have also settled down more.
On imports, where possible, use the `EntityNameId` for an import instead
of printing an instruction. Next, show the source location if we have a
node. Only show the instruction if there's no location.
This also exposes `Parse::Tree` and `TokenizedBuffer`, so that we can
pass a `SemIR::File` without the component parts. In particular this
allows us to get the `TokenizedBuffer` for import IRs without
substantial structural modifications. We may want to make these optional
for serialized `SemIR` later, but the nodes/tokens contain source
location, which we'd need for debug information -- so it's not clear how
much we can really make them optional without substantial information
loss.
Reduce arguments to just `File` in a few spots, as a result of the
accompanying `TokenizedBuffer` and `Parse::Tree`. Also updates style to
pass around `const File*` where the reference is maintained, instead of
`const File&`.
I was considering keeping a direct reference to the tree and tokens on
`Context`, but initially my thought was it wouldn't make much
difference. I can re-add those if desired, just as direct caching of the
`File` fields.
The current representation has two nested presence indicatators (the
optional bool, the null pointer). Currently the pointer is never null,
but the style guide suggests that T* should be used for parameters that
may or may not be present, so we do not need the optional here.
> When passing an object's address as an argument, use a reference
> unless one of the following cases applies:
>
> - If the parameter is optional, use a pointer and document that it
> may be null.
Once the parameter is just a pointer, the ScopedTiming field does not
need an optional either, and can just store the pointer.
It would be more preferable to have an optional representation of a
sometimes-null pointer like optional<T&> to describe a sometimes-null
pointer, as this would allow clearer runtime diagnostics when used
incorrectly (a check failure in unwrapping) and would be better
self-documenting through syntax instead of a comment. But we do not
currently have such a primitive.
If they were called twice for a CompilationUnit, they would destroy
objects that they created and returned a pointer to, leaving a dangling
pointer somewhere else.
The switch defaults to true, so it's displayed as `--no-debug-info` in
the help. The description should be agnostic about whether the flag will
enable or disable the behaviour.
This is really a set of closely related changes:
1) We really shouldn't be creating a Check::Unit for _Lower_. This fixes
that by storing the diagnostic converter for reuse.
2) Rather than passing in node converters to Check as their own array,
pass diagnostic converters as part of Check::Unit.
3) To support creating the converters early, pass SemIR::File
pre-constructed.
4) Since SemIR::File construction was used to track "checked", add
`is_checked` for that.
5) Clarifies a subtle edge case around `input_filename_` use with `-`.
Note the key consequence of this change, where I actually started, is
that `Check` only has one array of `Check::Unit` instead of receiving
`NodeLocConverter` as a separate array.
As discussed in #4530 . This required switching to using
`llvm::IntrusiveRefCntPtr` in a number of places.
---------
Co-authored-by: Josh L <josh11b@users.noreply.github.com>
We do not intend to use std::ranges in the Carbon implementation due to
concerns of compile time cost, largely due to implicit instantiation of
types involved in calling and typechecking the functions and their
requires clauses.
In #4539, we converted std::any_of to std::ranges::any_of, but this
replaces that with llvm::any_of from llvm/ADT/STLExtras.h.
This conversion was suggested by the modernize-use-ranges clang-tidy
check. We can keep the check on, and use it to guide conversion to llvm
helpers that do similar things (as was done in this CL now). If it's
being too confusing, then it can be disabled as well.
- Do not include entities imported from files that we are not dumping.
- Do not include constants and import_refs that are not referenced by
something that we are including in the formatted output.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Boaz Brickner
Jon Ross-PerkinsandGeoff Romer
Richard SmithandChandler Carruth
Thomas Köppe
Calvin
ottmar-zittlau
josh11bandJosh L
