[ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl

[teemperor]

Summary

Shafik found out that importing a CXXConstructorDecl will create a translation unit that
causes Clang's CodeGen to crash. The reason for that is that we don't copy the OperatorDelete
from the CXXConstructorDecl when importing. This patch fixes it and adds a test case for that.

[teemperor]

Didn't really upstream any non-trivial ASTImporter patches yet, so please point out any style errors.

[a_sidorin]

Hi Rafael,
The change looks mostly fine but I have some comments inline.

lib/AST/ASTImporter.cpp

3258

It's better to move this code to VisitFunctionDecl to keep all related stuff together.

3261

Moving this code to VisitFunctionDecl will also allow us not to create a dtor if this import fails.

[shafik]

Thank you, this looks good but perhaps some refactoring would help improve the change.

lib/AST/ASTImporter.cpp

3258

I am not sure I agree. Having everything in VisitFunctionDecl makes this code harder to maintain.

3261

At what point in VisitFunctionDecl would you expect this change to bail out of? A possible refactor would be to move this code to a separate function and then call that function at that point in VisitFunctionDecl. This would prevent VisitFunctionDecl from becoming even larger and I think address your concern.

[a_sidorin]

Hi Shafik,
Please find my answers inline.

lib/AST/ASTImporter.cpp

3258

That's true, but I think it's better to put FunctionDecl refactoring as a separate commit - it would be bigger than just putting some related stuff to the original function. We have the same problem with VisitRecordDecl, but, unfortunately, I still had no time to rearrange this stuff.

3261

Just before line 3096, where a new dtor decl is being created.
If correct import of operator delete is required for successful import of dtor decl, it's better to check if it can be imported before a new dtor decl is created. So, it's better to put operator delete import before the creation of a new declaration which is located in VisitFunctionDecl(). Otherwise, we will be able to create a declaration both invalid and available by AST lookup (because DeclContexts are already set).
Please correct me if I'm wrong.

[teemperor]

• Moved code into the VisitFunctionDecl function.

I don't mind if we clean up that function in a later commit. Thanks for the feedback!

[a_sidorin]

LGTM, thank you!
I'm sorry if the change I requested doesn't look good, but I think the correctness is our priority.

[martong]

The updated version looks good to me! Thank you!

lib/AST/ASTImporter.cpp

ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   FunctionDecl *ToFunction = nullptr;
   if (auto *FromConstructor = dyn_cast<CXXConstructorDecl>(D)) {
     if (GetImportedOrCreateDecl<CXXConstructorDecl>(
         ToFunction, D, Importer.getToContext(), cast<CXXRecordDecl>(DC),
         ToInnerLocStart, NameInfo, T, TInfo,
         FromConstructor->isExplicit(),
         D->isInlineSpecified(), D->isImplicit(), D->isConstexpr()))
       return ToFunction;
-  } else if (isa<CXXDestructorDecl>(D)) {
+  } else if (CXXDestructorDecl *FromDtor = dyn_cast<CXXDestructorDecl>(D)) {
+
+    auto Imp =
+        importSeq(const_cast<FunctionDecl *>(FromDtor->getOperatorDelete()),
+                  FromDtor->getOperatorDeleteThisArg());
+
+    if (!Imp)
+      return Imp.takeError();
+
+    FunctionDecl *ToOperatorDelete;
+    Expr *ToThisArg;
+    std::tie(ToOperatorDelete, ToThisArg) = *Imp;
+
     if (GetImportedOrCreateDecl<CXXDestructorDecl>(
         ToFunction, D, Importer.getToContext(), cast<CXXRecordDecl>(DC),
         ToInnerLocStart, NameInfo, T, TInfo, D->isInlineSpecified(),
         D->isImplicit()))
       return ToFunction;
+
+    CXXDestructorDecl *ToDtor = cast<CXXDestructorDecl>(ToFunction);
+
+    ToDtor->setOperatorDelete(ToOperatorDelete, ToThisArg);
   } else if (CXXConversionDecl *FromConversion =
                  dyn_cast<CXXConversionDecl>(D)) {
     if (GetImportedOrCreateDecl<CXXConversionDecl>(
             ToFunction, D, Importer.getToContext(), cast<CXXRecordDecl>(DC),
             ToInnerLocStart, NameInfo, T, TInfo, D->isInlineSpecified(),
             FromConversion->isExplicit(), D->isConstexpr(), SourceLocation()))
       return ToFunction;
   } else if (auto *Method = dyn_cast<CXXMethodDecl>(D)) {

 
 ExpectedDecl ASTNodeImporter::VisitCXXConstructorDecl(CXXConstructorDecl *D) {
   return VisitCXXMethodDecl(D);
 }
 
 ExpectedDecl ASTNodeImporter::VisitCXXDestructorDecl(CXXDestructorDecl *D) {
   return VisitCXXMethodDecl(D);
 }
 
 ExpectedDecl ASTNodeImporter::VisitCXXConversionDecl(CXXConversionDecl *D) {
   return VisitCXXMethodDecl(D);
 }
 
 ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
   // Import the major distinguishing characteristics of a variable.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))

test/Import/destructor/Inputs/F.cpp

+struct B {
+  virtual ~B() {}
+};

test/Import/destructor/test.cpp

+// RUN: clang-import-test -dump-ast -import %S/Inputs/F.cpp -expression %s
+
+// Triggers the deserialization of B's destructor.
+B b1;
+
+// CHECK: CXXDestructorDecl
+
+// CHECK-NEXT: ~B 'void () noexcept' virtual
+// CHECK-SAME: 'void () noexcept'
+// CHECK-SAME: virtual