This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Fix import of class templates partial specialization
ClosedPublic

Authored by martong on Aug 8 2018, 8:01 AM.

Details

Summary

Currently there are several issues with the import of class template
specializations. (1) Different TUs may have class template specializations
with the same template arguments, but with different set of instantiated
MethodDecls and FieldDecls. In this patch we provide a fix to merge these
methods and fields. (2) Currently, we search the partial template
specializations in the set of simple specializations and we add partial
specializations as simple specializations. This is bad, this patch fixes it.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.Aug 8 2018, 8:01 AM
martong retitled this revision from Fix import of class templates partial specialization to [ASTImporter] Fix import of class templates partial specialization.Aug 8 2018, 9:48 AM

Hi Gabor,

While importing methods looks harmless, importing fields can be a breaking change. Do you have any test results on this?

lib/AST/ASTImporter.cpp
2872 ↗(On Diff #159724)

Honestly speaking, I wonder about this behaviour because it doesn't look similar to instantiation of only methods that are used. Is it a common rule?

4550 ↗(On Diff #159724)

Importing additional fields can change the layout of the specialization. For CSA, this usually results in strange assertions hard to debug. Could you please share the results of testing of this change?
This change also doesn't seem to have related tests in this patch.

4551 ↗(On Diff #159724)

The result of import is unchecked here and below. Is it intentional?

unittests/AST/ASTImporterTest.cpp
2722 ↗(On Diff #159724)

3-space indentation.

2763 ↗(On Diff #159724)

Broken indent.

martong marked 5 inline comments as done.Aug 14 2018, 4:21 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
2872 ↗(On Diff #159724)

Yes, this is a common rule. The instantiation of an initializer is similar to the instantiation of default arguments in a sense that both are instantated only if they are being used.

To be more precise, quoting from Vandevoorde - C++ Templates The Complete Guide / 14.2.2 Instantiated Components:

... Default function call arguments are considered separately when instantiating
templates. Specifically, they are not instantiated unless there is a call to that
function (or member function) that actually makes use of the default argument.
If, on the other hand, the function is called with explicit arguments that override
the default, then the default arguments are not instantiated.
Similarly, exception specifications and default member initializers are not
instantiated unless they are needed.

4550 ↗(On Diff #159724)

TLDR; We will not create additional fields.

By the time when we import the field, we already know that the existing specialization is structurally equivalent with the new one.
Since a ClassTemplateSpecializationDecl is the descendant of RecordDecl, the structural equivalence check ensures that they have the exact same fields.
When we import the field of the new spec and if there is an existing FieldDecl in the "To" context, then no new FieldDecl will be created (this is handled in VisitFieldDecl by first doing a lookup of existing field with the same name and type).
This patch extends VisitFieldDecl in a way that we add new initializer expressions to the existing FieldDecl, if it didn't have and in the "From" context it has.

For the record, I added a new test to make sure that a new FieldDecl will not be created during the merge.

4551 ↗(On Diff #159724)

Yes, that is intentional. We plan to refactor all ASTImporter functions to provide a proper error handling mechanism, and in that change we would like to enforce the check of all import functions.
Unfortunately, currently we already have many places where we do not check the return value of import.

martong marked 3 inline comments as done.Aug 14 2018, 4:24 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
4551 ↗(On Diff #159724)

Actually, having proper error handling is just one thing, the other more important reason why we don't check the result of the import here, because we don't want to stop merging other fields/functions if one merge failed. If one merge failed, other merge operations still could be successful.

martong updated this revision to Diff 160552.Aug 14 2018, 4:32 AM
  • Add new test
  • Fix indentation
martong added inline comments.Aug 14 2018, 4:37 AM
lib/AST/ASTImporter.cpp
4550 ↗(On Diff #159724)

This is the new test: ODRViolationOfClassTemplateSpecializationsShouldBeReported. It checks that it is not possible to add new fields to a specialization, rather an ODR violation is diagnosed.

a_sidorin added inline comments.Aug 19 2018, 8:08 AM
lib/AST/ASTImporter.cpp
4550 ↗(On Diff #159724)

Thank you for the explanation. However, I find the comment very misleading. It tells:

// Check and merge those fields which have been instantiated
// in the "From" context, but not in the "To" context.

Would it be correct to change it to "Import field initializers that are still not instantiated", or do I still misunderstand something?

martong marked an inline comment as done.Aug 21 2018, 5:04 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
4550 ↗(On Diff #159724)

Yes, I agree that this comment is not precise and could be misleading, thank you for pointing this out. I changed this comment and others too.

martong updated this revision to Diff 161698.Aug 21 2018, 5:05 AM
martong marked an inline comment as done.
  • Change comments
martong updated this revision to Diff 161699.Aug 21 2018, 5:08 AM
  • Add one more TODO to import instantiated exception specifications
a_sidorin accepted this revision.Aug 21 2018, 11:33 PM

Thank you!

This revision is now accepted and ready to land.Aug 21 2018, 11:33 PM
This revision was automatically updated to reflect the committed changes.