Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update the section on the vague linkage of v-tables and RTTI #171

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Update the section on the vague linkage of v-tables and RTTI #171

merged 2 commits into from
Nov 14, 2023

Conversation

rjmccall
Copy link
Collaborator

@rjmccall rjmccall commented Nov 2, 2023

  • Cover the impact of C++20 modules on v-table emission
  • Clarify the wording in several places
  • Weaken some constraints that should be unnecessary
  • Note some platform divergences from the ABI

Addresses #170.

@rjmccall
Update the section on the vague linkage of v-tables and RTTI:
057b367 
- Cover the impact of C++20 modules on v-table emission
- Clarify the wording in several places
- Weaken some constraints that should be unnecessary
- Note some platform divergences from the ABI
@rjmccall rjmccall requested a review from jicama November 2, 2023 21:23
@rjmccall rjmccall mentioned this pull request Nov 2, 2023
Open
@rjmccall
Copy link
Collaborator Author

rjmccall commented Nov 2, 2023

I intentionally left the RTTI recommendation open-ended; I'll try to come back and fill that in in the fullness of time, because the current state of the world is not great.

@zygoloid, I can't mark you as a reviewer, but you're likely interested in this.

zygoloid
zygoloid reviewed Nov 2, 2023
View reviewed changes
abi.html Outdated Show resolved Hide resolved
abi.html
<p>
The RTTI <code>std::type_info</code> structures for various basic
types as specified by the <a href=#rtti>Run-Time Type Information</a>
section are provided by the runtime library.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth documenting that we expect these to get emitted in the same object as the vtable for __cxxabiv1::__fundamental_type_info?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, I feel like that's a runtime-library implementation detail that doesn't need to be in the ABI.

@rjmccall @zygoloid
Fix typo
33475e6 
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
@rjmccall
Copy link
Collaborator Author

I think this has been open long enough for a mostly editorial change.

@rjmccall rjmccall merged commit f2ce2cf into itanium-cxx-abi:main Nov 14, 2023
@rjmccall rjmccall deleted the vtable-vague-linkage branch November 14, 2023 04:16
@frederick-vs-ja frederick-vs-ja mentioned this pull request Dec 4, 2023
Merged
hubot pushed a commit to gcc-mirror/gcc that referenced this pull request Mar 7, 2024
@wreien
c++: Redetermine whether to write vtables on stream-in [PR114229]
9ccd03d 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 11, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
934a22f 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 11, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
86db105 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 11, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
69ff94a 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 11, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
1800cf7 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 11, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
ab0714e 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 11, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
0ccc3c4 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 11, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
30d55e4 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 11, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
19de442 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 13, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
2ed6ad5 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/ruyi-gcc that referenced this pull request Mar 13, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
9859362 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Liaoshihua pushed a commit to Liaoshihua/gcc that referenced this pull request Mar 19, 2024
@wreien @Liaoshihua
c++: Redetermine whether to write vtables on stream-in [PR114229]
639d364 
We currently always stream DECL_INTERFACE_KNOWN, which is needed since
many kinds of declarations already have their interface determined at
parse time.  But for vtables and type-info declarations we need to
re-evaluate on stream-in as whether they need to be emitted or not
changes in each TU, so this patch clears DECL_INTERFACE_KNOWN on these
kinds of declarations so that they can go through 'import_export_decl'
again.

Note that the precise details of the virt-2 tests will need to change
when we implement the resolution of [1], for now I just updated the test
to not fail with the new (current) semantics.

[1]: itanium-cxx-abi/cxx-abi#171

	PR c++/114229

gcc/cp/ChangeLog:

	* module.cc (trees_out::core_bools): Redetermine
	DECL_INTERFACE_KNOWN on stream-in for vtables and tinfo.
	* decl2.cc (import_export_decl): Add fixme for ABI changes with
	module vtables and tinfo.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_b.C: Update test to acknowledge that we
	now emit vtables here too.
	* g++.dg/modules/virt-3_a.C: New test.
	* g++.dg/modules/virt-3_b.C: New test.
	* g++.dg/modules/virt-3_c.C: New test.
	* g++.dg/modules/virt-3_d.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
hubot pushed a commit to gcc-mirror/gcc that referenced this pull request May 2, 2024
@wreien
c++: Implement modules ABI for vtable emissions
ad30265 
This patch implements the changes described in
itanium-cxx-abi/cxx-abi#171.

One restriction that is lifted in the ABI that hasn't been updated here
is that the ABI no longer requires unique vtables to be emitted with
vague linkage.  I haven't changed this behaviour for this patch, but in
the future we could look into changing the relevant target hook
('class_data_always_comdat') to default to 'false'.  But the current
behaviour is more forgiving to changes in key function identification.

Since the ABI for vtables attached to named modules no longer depends on
key methods, this also resolves the issue described in PR c++/105224.

	PR c++/105224

gcc/cp/ChangeLog:

	* class.cc (finish_struct_1): Also push classes attached to a
	module into the 'keyed_classes' list.
	* decl.cc (record_key_method_defined): Don't push classes
	attached to a named module into the 'keyed_classes' list.
	* module.cc (trees_in::read_class_def): Likewise.
	* decl2.cc (import_export_class): Uniquely emit vtables for
	non-template classes attached to a named module.
	(vtables_uniquely_emitted): New function.
	(import_export_decl): Update comments. Update with knowledge
	about new kinds of uniquely emitted vtables.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/virt-2_a.C: Update linkage requirements.
	* g++.dg/modules/virt-2_b.C: Likewise.
	* g++.dg/modules/virt-2_c.C: Likewise.
	* g++.dg/modules/virt-4_a.C: New test.
	* g++.dg/modules/virt-4_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
Reviewed-by: Jason Merrill <jason@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zygoloid zygoloid zygoloid approved these changes

@jicama jicama Awaiting requested review from jicama

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@rjmccall @zygoloid