Found by fuzz testing, root caused to #7479 - fixing this issue for Var
in a similar way to how it was fixed for Let in that patch (by adding
`RuntimeBindingName` to the possible things in the name binding.
Not sure if this would be better fixed by adding `RuntimeBindingName` to
`AnyRuntimeBindingPatternName` or something else?
Assisted-By: Gemini with Antigravity
rumdl error broke the proposal script execution:
```
rumdl check..............................................................Failed
- hook id: rumdl
- exit code: 1
proposals/p000003-test-proposal.md:15:5: [MD051] Link anchor '#todo-initial-proposal-setup' does not exist in document headings
Issues: Found 1 issues in 1 file (11ms)
```
So update the script to remove the TODO entry in the ToC to match
removing the TODO section.
The specific location's not ideal (rather than the open curly, or
semicolon for a declaration - the two locations should be the `class`
and then the class name), but the same as we do for functions for now &
enough to get by.
This specifically also fixes a crash I found due to dtors being
generated without a location (because implicitly created functions would
use the class's location), creating a function call without a debug
location, which fails the LLVM IR verifier.
This ensures the clang_decls map is used as a cache - without this,
visiting the same entity twice could cause it to be
re-exported/duplicated. See attached test case.
Removing the clang_decl_id on SemIR::Function - using only the
clang_decls map to create the association between SemIR::Function and
clang::FunctionDecls.
This adds an `is_external` flag to ClangDecl to indicate whether the
entity originated from Carbon or was imported from another language.
(I'm open to names - I guess for now we mostly use "is this from C++" to
be more specific than "is this external" - eg: NameScope::is_cpp_scope)
This allows Clang to correctly generate the vtable for the exported
class.
There's still something wrong with new virtual functions in the Carbon
type (left a TODO) - I thought it might be related to not flagging
the CXXMethodDecl as virtual, but my initial experiments don't seem to
back that up, so I'll look into it further separately.
There's also a test regression due to an virtual (well, abstract
specifically, but I think it'd happen with a virtual one too) function
in an abstract class taking `self` by value being rejected since
the abstract class can't be instantiated. Not sure if this is a correct
change - the test's behavior could be preserved by using `ref self`
instnead of `self` in this function. Is that reasonable/expected? Should
we not require a type to be complete when passing by value if we can
compute the value representation without such completeness?
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This correctly renders the vtable in SemIR, including allowing overrides
in
Carbon-derived-from-C++ classes.
It doesn't work in lowering because clang walks the methods of the
CXXRecordDecl - and we currently don't export anything into the
CXXRecordDecl's methods (we do export the fields) - so that's next.
This also doesn't teach Clang to affirmatively emit the vtable
regardless of the types use in C++ code - or to have Carbon use the
vtable in an object's initialization.
Roundtrip (export/reimport) class declarations
The remapping was previously implemented using name_scopes, which aren't
created for class declarations, only definitions - causing the reimport
to import a fresh copy of the type that mismatched with the original (as
seen in the test baseline).
By changing the mapping to use the reverse part of the clang_decls
mapping this should generalize better (& we probably should further
migrate to that mapping). Though it did trip over some issue with
exactly which instruction is used as the key in the clang_decls map -
this change moves towards standardizing on the first decl id of the
class as its map key.
This still only works if the hash of the distinct types are identical
(so it still doesn't address the derived pointer v base pointer case -
well, not in the way we would want to address it, we could use this
change to make derived pointer and base pointer not compare equal, but
that's not very ergonomic)
I think in a follow up maybe I can use a `TranslatingKeyContext` to
translate `Derived*` to `Base*` in general.
No test coverage for this change, since it's a no-compile situation and
we don't seem to generally do no-compile tests.
Discovered while working on #6940
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Start recording the clang::DeclContext* -> InstId mapping for use in
later operations.
The test update includes removing the initial fail_* test because I
hadn't thought about the use of namespace aliases as a way to test for
the presence of a namespace without the failure caused by not finding
the thing inside the namespace.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Add a clang::ExternalASTSource to begin exposing Carbon entities to
Clang - initially only a single `Carbon` top level namespace.
Subsequent work will add Carbon entities to this namespace.
Likely this CarbonExternalASTSource will be refactored into another
file, tie into/reference SemIR::File and CppFile, etc eventually - but
that'll wait for future patches.
If there's mechanical problems with the current implementation - how I'm
creating the new NamespaceDecl, etc - I'm all ears. It's very much in
the "it seems to work" state, not much more than that.
This does break Clang Modules (header modules, C++20 modules,
precompiled headers, etc) since they're implemented as an
ExternalASTSource as well, and Clang's ASTContext only supports one
ExternalASTSource at a time. To fix that regression we'll need to
implement some kind of ExternalASTSource multiplexing support - either
in Clang or Carbon (unclear which).
This regression of modules support can be observed by the following:
`A.h`
```
inline void f1() { }
```
`module.modulemap`
```
module A {
header "A.h"
export *
}
```
`test.carbon`
```
import Cpp inline '''
// Hardcode the pragma to ensure this isn't silently falling back to
// textual inclusion.
void f2() {
f1();
}
''';
```
```
carbon compile test.carbon -- -I . -fmodules -fimplicit-modules -fmodules-cache-path=module_cache
```
I wrote a `file_test` test for this, but it doesn't /quite/ work because
`file_test` provides an in-memory filesystem for tests to make them more
hermetic, but Clang's Filesystem abstrtaction is for reading only - so
the module that's written out successfully can't be found when it needs
to be read back in - so the test doesn't pass as a baseline. Clang does
have support for `llvm::vfs::OutputBackend` which allows virtualizing
output - which I guess we could tie together with the InMemoryFilesystem
we use for input to make such a test work. But I guess that's not worth
the effort here?
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Clang performs the equivalent of Carbon's `lower` progressively,
interleaved with parsing/semantic analysis. This is in conflict with
Carbon's phase-based approach and leads to bugs in missing functionality
in Clang's generated IR during Carbon/C++ interop.
I surveyed other uses of Clang's APIs (originally written up in
[this](https://docs.google.com/document/d/1wi85FRiWh4X9A-gCYMVGKR40-q5fM6-3JaSpePk-XCY/edit?tab=t.0#heading=h.j7j8nwhzao5n)
doc - though the contents in this proposal are now more complete than
the doc) to better understand how Clang's constraints might effect
projects and how they've addressed them. In the mean time, Carbon
changes made more stable approaches viable that were eventually
implemented in #6569.
This proposal then aims to formalize the analysis that lead to #6569 for
posterity in case these design decisions need to be revisited in the
future.
---------
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
In some situations we need to mangle class declarations, which then are
not NameScopes (can't scope anything inside a declaration) - so make the
mangler able to cope with that situation by mangling the name of the
class directly rather than relying on generic NameScope mangling to
handle the class case.
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.
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.
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>
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 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)
```
I was thinking that the incompleteness diagnostic for C++ types would've
been produced by Clang for record types, but seems they're produced by
Carbon & we already /are/ sharing that diagnostic (with #6302), and that
patch only adds an extra note rather than being a whole separate
codepath for effectively the same diagnostic.
This doesn't show up in the raw SemIR dumps (I don't think that's due to
a lack of coverage, but due to the fact that ExprRegionId isn't used as
an operand of any instructions).
I don't /think/ these ids can appear in the raw SemIR dump, so this
change doesn't show up in any test updates - but it should still be
valuable for identifying bugs in the future.
Based on review feedback on
https://github.com/carbon-language/carbon-lang/pull/6215#discussion_r2430177644
There are some intermediate commits with alternatives, finding other
ways (non-templates) to address the layering boundaries between
`ValueStore` construction and `CheckIRId` tagging. But, yeah, template
seems like the way to go - certainly in terms of terseness and probably
in terms of extensibility to other Id tagging as/when needed.
Use the `CheckIRId` as a unique identifier for the scope of an `InstId`
- if an `InstId` is created within the scope of one `CheckIRId` it must
not be used in the scope of a different `CheckIRId`.
This is achieved without extra storage, but with false negatives for
large inputs.
When an `InstId` is created, the original index of the `Inst` is XORed
with a tag derived from the `CheckIRId` to produce the final `InstId`.
When the `InstId` is used, the expected tag is XORed with the `InstId`
to get back to the original index - if the tags don't match, the
resulting index will be corrupted, likely too large - resulting in an
out of bounds index CHECK-failure.
(the tag value is derived as such:
* take the CheckIRId
* left shift one bit (padding zero)
* left shift another bit (padding 1 - used to signify that the resulting
`InstId` has a tag combined into it)
* reverse the bits
In this way, the tag is unlikely to overlap with the index for small
test cases - making it possible to separate out the `CheckIRId` from the
index in these cases to provide more meaningful debugging/CHECK
messages, and more informative `SemIR` textual dumping that can now
include the `CheckIRId` along with the `Inst`'s index in the name of an
`inst`)
The test churn here is improved printing as tagged `InstId`s can now,
with best effort (more likely for small test cases where the `CheckIRId`
and the `Inst` index aren't at risk of overlapping from the high and low
bits), render the `CheckIRId` as part of the inst's name. Going from
`instNN` to `irMM.instNN`.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
I wasn't sure exactly in what way it was problematic, so I was a bit
vague in the justification (though justifications aren't generally
needed/provided here anyway - so I'm not sure it'd be net helpful to add
one anyway).
This is Itanium-specific for now (explicitly downcasting to the itanium
vtable handling code in Clang) - though it doesn't look like it'd be a
big stretch to either have conditional/two codepaths down Itanium and
MSVC in Carbon, or maybe add a virtual function in clang to avoid
needing to conditional+downcast in Carbon.
Here's a working example:
`dynamic_type.h`:
```
#ifndef TEST_H
#define TEST_H
struct A {
virtual auto virt0() -> int;
virtual auto virt1() -> int;
};
auto GetVal() -> A* _Nonnull;
#endif
```
`test.carbon`:
```
library "test";
import Cpp library "dynamic_type.h";
import Core library "io";
fn Run() {
var a: Cpp.A* = Cpp.GetVal();
Core.Print(a->virt0());
Core.Print(a->virt1());
}
```
`dynamic_type.cpp`:
```
#include "dynamic_type.h"
auto A::virt0() -> int {
return 0;
}
auto A::virt1() -> int {
return 1;
}
struct B: A {
auto virt0() -> int override {
return 7;
}
auto virt1() -> int override {
return 42;
}
};
auto GetVal() -> A* _Nonnull {
static B b;
return &b;
}
```
```
$ ./bazel-bin/toolchain/carbon compile test.carbon
$ clang++-tot -g dynamic_type.cpp test.o --output=a.out
$ ./a.out
7
42
```
(linking with `carbon link` failed because we aren't linking to the C++
runtime yet, it seems, so: `ld.lld: error: undefined symbol: vtable for
__cxxabiv1::__class_type_info`)
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Found by WIP validation for this type of issue ongoing in #5997
I'm not entirely sure how the one test update falls out of this change -
but it is from the same test that I originally reduced the problem from,
which is reassuring.
The reduced test case I investigated the issue with was this:
`a.carbon`:
```
library "lib";
interface I1(Other:! type) {
let Result:! type;
}
```
`b.carbon`:
```
import library "lib";
class T1 { }
impl T1 as I1(Self) where .Result = Self { }
```
The SemIR dump diff looked like this:
```
89c89
< %Main.import_ref.b6f = import_ref Main//lib, inst28 [no loc], unloaded
---
> %Main.import_ref.b6f = import_ref Main//lib, inst27 [no loc], unloaded
96c96
< %Main.import_ref.f7b: @I1.%I1.type (%I1.type.e87) = import_ref Main//lib, inst28 [no loc], loaded [symbolic = @I1.%Self (constants.%Self.c47)]
---
> %Main.import_ref.f7b: @I1.%I1.type (%I1.type.e87) = import_ref Main//lib, inst27 [no loc], loaded [symbolic = @I1.%Self (constants.%Self.c47)]
```
Which is a difference, but given the `inst28`/`inst27` don't appear
anywhere else than these two lines, it doesn't give a terribly
meaningful diff/story about what changed - but perhaps it's
sufficient...
Not sure if this test ^ is sufficiently more interesting than the diff
update already in this patch. If so, happy to add the above as a new
test case.
Open to ideas.
This addresses/avoids the duplicate import of vtables.
I went through a few iterations/etc along the way and left them in the
commit
history for the PR in case any of them are useful to illustrate how I
got here,
or worth revisiting.
Essentially I ended up with a circularity in importing - importing the
class
imported the vtable_decl which imported the virtual functions - and then
pending
specifics of the virtual functions needed the self specific of the
enclosing
class which wasn't ready yet.
Adding ImportRef to the vtable_decl to break the cycle caused me trouble
when
naming the vtable_decl instructions - so I tried making the functions in
the
vtable unloaded ImportRefs instead. That worked, but meant that
importing a
class still was doing O(number of vtable entries) even if the vtable
wasn't
used.
So I revisited the lazy vtable_decl - figured out how to make the naming
work
(when building the vtable_ptr, even though the vtable_decl doesn't have
to be
loaded for the vtable_ptr, I force it to be loaded anyway, to load the
vtable so
it's usable by lowering, etc). And then I could go back to the old
non-lazy
loaded vtable entries (using some loaded ImportRefs in the cases where
we needed
them/had already adopted them).
Then thinking about the VtablePtr instruction, went back/forth on
exactly what
it needed - went from VtablePtr's member being a VtableDecl InstId, to a
ClassId, then back to a VtableId as it was before this patch.
Naming the instructions has one oddity, that the VtableDecl and
VtablePtr
instructions seem to need to add the pending name for the VtableId -
despite not
using the VtableId in their own name - should the inst namer be doing
this work
for parameters of instructions rather than requiring the inst to do it
deliberately? (or am I holding it wrong in some way?)
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
With help from Richard Smith debugging/identifying this.
Hmm - looks like maybe the Self type import ref may have the same
problem? (or at least it seems to have the same quirk in the semir dump,
where the inst id is mentioned in the `import_ref` insts, but is not
defined elsewhere, has no name, and says `[no loc]`. I'll look into that
separately. (hmm, maybe this is just an unloaded ImportRef, actually)
Similar to 26ec78ec00 - vtable entries
can't be unattached symbolic constants, because if they are they can't
be `GetValueInSpecific`d because they lack the context for their
generic/specific.
Importing mostly wants to make/import things as unattached constants -
but `ImportRef` supports attached constants, so use those - but we don't
need the laziness, so use `LoadedImportRef`.
Otherwise these end up as unattached constants (see the baseline test
changes) and can't be resolved by `GetConstantValueInSpecific` in
lowering or in further derived vtables.
If the class is non-generic, then it's fine for the vtable entry for
some function inherited from a generic base is represented as an
unattached constant, since the specific in that specific_function is
already fully resolved.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Also update `RequestVtableIfVirtual` to strip `impl` from functions in
classes without a base class - this avoids a duplicate diagnostic where
they're diagnosed as being in a class without a base class, then
diagnosed again because they don't have a matching function in a base
class.
Some specific features:
* Use `SpecificFunction` for vtable entries for generic classes.
* Create specific constants for vtable entries in classes derived from
generic classes to reference the appropriate specific of the function
in the context of such a derived class.
* Create specific constants for vtable_ptrs for uses of specific generic
classes.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Ensure `vtable_ptr`s(and the vtables they refer to) aren't
imported if the type is imported but the vtable isn't
needed (no initialization of a value of that type is required).
Is it worth having a distinct diagnostic or phrasing for non-class types
(like tuples, structs, pointers, etc), also for declared-but-not-defined
class types (where we can't tell if they're final or not)? Happy to add
it, but not sure how much detail to put in here at this stage at least.
I chose "non-final type" as somewhat vague wording so it sort of applies
even to pointers/tuples/structs.
The goal was/is to reduce the overhead for vtables in generics - the
previous representation/prior to this patch caused a new vtable to be
created in every specific which isn't generally what we want for Carbon
generics (the whole specific/generic thing is meant to avoid creating
specific versions for things that can be a generic form parameterized by
a specific instead of manifest as a unique entity per specific)
So this moves vtables to a top level object (like functions, classes,
etc). Each dynamic class will have a vtable in this list.
Classes have a `vtable_ptr` instruction in them that points to the
vtable.
The actual generic support hasn't been implemented in this patch, as
I've been struggling with just getting this part of the migration going
& wanted to get it flushed out before adding the additional
complications.
It's possible more laziness when doing cross-file importing would be
suitable - for instance if we only need to reference the vtable from
another file, but don't need to know its individual contents, it may be
beneficial for the functions in the vtable to be import_refs (or to add
another layer of indirection - so it can be a single import_ref
all-or-nothing for the functions in the vtable).
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This adds something similar to the level of `const` support - that it's
a type, but not the conversions and limitations on usage that are
needed.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Similar to the class change made in
c6f25e9018 but without tests as it doesn't
appear that the same bug is reachable for these cases at the moment -
but seems good to match the approach in case cycles appear in the future
and to make the code consistent.
The impl case didn't seem to be able to use the new common utility
function, since it splits the two pieces of work and only does the
second conditionally.
This would've identified c6f25e9018
earlier/more clearly.
I looked for similar assertions for things like
`GetLocalConstantValueOrPush` but it has the right property by
construction (if it's going to return `None`, it pushes work) so an
assertion didn't seem suitable there.
Perhaps there are other such mapping functions that could get this
treatment? Open to pointers.
Entities, such as `Function`s may be created during phase 2 and need to
read the `Class`'s `scope_id` at that point, so it must be made
available earlier (in phase 2, rather than 3) when importing.
(thanks @zygoloid for explaining this all to me)
I'll look into other instances of this 3 phase lookup to see if they
have
similar bugs/if I can create test cases to tickle them as follow-ups.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Not entirely sure what the SemIR representation for this should be - do
we put the bogus thing in the vtable, and just not lower it later? The
patch currently doesn't add the function to the SemIR vtable - which
then means you could find a virtual function that's not in the vtable,
which seems similarly confusing.
I guess we could make the function non-virtual?
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
These will need to be emitted lazily, as we do for functions - this
addresses the crash/removes the impossible (because we don't have a
specific) non-lazy path.
Adds a `virtual_index` to `SemIR::Function` used to determine which
vtable slot
to use when calling the given function.
Then use that to lower the function call to use the vtable and
specifically the
relative vtable ABI to match the vtable entries.
Adds a mapping to keep track of vtable LLVM IR decls/defs for use.
Adds the vtable_id to the vtable_ptr initialize instruction for lookup.
Adds emission of vtable declarations for use outside the file that
defines the vtable. (this isn't done lazily, it's done for any imported
class - it could be done lazily & maybe eventually has to be lazy to
handle generics)
One of the remaining three steps for proof-of-concept virtual function
lowering (the other two being: initializing vptrs to point to these
tables, and using the vptr+table at call sites).
Introduces a (placeholder?) mangling of vtables as ".$vtable" at the end
of the mangling of the class name.
Uses a scheme similar to clang's relative vtables - though those are
relative to the vtable slot, and this is relative to the start of the
vtable (seemed simpler? though I haven't looked at it in detail, perhaps
in lowering call sites I'll find the relative-to-vtable-slot is nicer,
easy enough to change).
Fixes#5186
With @zygoloid's kind assistance, this generalizes the existing
non-const lowering of ClassInit, that had previously only handled
InitializeFrom, to find other cases - such as a nested ClassInit used to
initialize a class member.
This refactors the `FindReturnSlotArgForInitializer` from
`check/convert.cpp` into `sem_ir/file.{h,cpp}` for use from lowering
(since lower doesn't depend on check, which I assume is an intentional
layering constraint - so figured it made sense to move it to sem_ir, and
found one or two similar-ish utility functions in `sem_ir/file.{h,cpp}`,
so figured that was a good spot)
Seems the instructions got emitted out of order & that caused problems
for lowering. This was because most of the initialization instructions
were added to a PendingBlock, but the vptr initialization instructions
were added to the (non-pending) block directly.
(I don't fully understand the pending stuff (is there a different test I
could/should write that demonstrates the vptr init instructions not
being discarded because they didn't go in the pending block (before this
patch)), or the out of order instruction problem (could we add more
robust checking for instruction ordering?) - but perhaps this is
adequate understanding for this bug fix at least)
Fixes#5094
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
While current examples of this could also be addressed by emitting
declarations on use (by which stage the associated types would have to
be complete by construction) - it's expected that future examples
(vtables, function pointers) will need to work in this case anyway, so
might as well implement this feature.
This tripped over a lowering crash when a member function with self was
declared-but-not-defined, so that's why some test cases were updated to
have (empty) function definitions.
I'll follow-up with/look into a fix for the
self-declared-but-not-defined cases separately.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
This ensures the data is available for more uses (specifically for
diagnosing virtual/abstract/impl functions on non-instance methods).
It still doesn't quite address the TODO to move the Self param search
all the way back to the param walk in all cases. To do that in the case
that still has a separate search loop, I think we'd have to change
`Check::NameComponent` to carry this information (as it carries the
implicit_param_patterns-id) - though there's comments in NameComponent
suggesting it shouldn't carry function-specific things like
`call_params_id` and `return_slot_pattern_id` - so I wasn't sure if it
was suitable to add more there, but I can - possibly in a follow-up
change.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
David Blaikie
