Fix codegen for virtual methods that are (re-) exported from multiple modules.

[klimek]

Summary

Fixes multiple crashes where a non-canonical decl would be used as
key.

[klimek]

There's (at least) one more getCanonicalDecl() missing; working on a test.

[klimek]

Attack the problem on a higher level; adapted test to catch more problems.

[rsmith]

I'm curious how this was failing -- all four places you changed appear to be walking over the method declarations that are lexically within the class definitions, so they should all see the same set of declarations. Is there some other source of map keys that uses the canonical declaration instead? In any case, using the canonical declaration as the key seems like a good idea.

Is there any reason not to put all the module definitions in the same module map file in your test case? The use directives also seem redundant, since this isn't a test of -fmodules-decluse.

[klimek]

Simplify module map.

[klimek]

Delete old module maps.

[klimek]

In D7830#128657, @rsmith wrote:

I'm curious how this was failing -- all four places you changed appear to be walking over the method declarations that are lexically within the class definitions, so they should all see the same set of declarations. Is there some other source of map keys that uses the canonical declaration instead? In any case, using the canonical declaration as the key seems like a good idea.

Most of the code already uses the canonical decls. When we create the OverridersMap / MethodInfoMap we apparently already use the canonical decl as key. The problem is that when we walk over the methods of a class owned by one module ('b' in this test), we might hit methods whose canonical decl is the method from the other module ('c' in this case).

Is there any reason not to put all the module definitions in the same module map file in your test case? The use directives also seem redundant, since this isn't a test of -fmodules-decluse.

Done. Was just the way I was used to seeing module maps :)

lib/AST/VTableBuilder.cpp

void FinalOverriders::dump(raw_ostream &Out, BaseSubobject Base,
   RD->printQualifiedName(Out);
   Out << ", ";
   Out << Base.getBaseOffset().getQuantity() << ")\n";
 
   // Now dump the overriders for this base subobject.
   for (const auto *MD : RD->methods()) {
     if (!MD->isVirtual())
       continue;
-  
+    MD = MD->getCanonicalDecl();
     OverriderInfo Overrider = getOverrider(MD, Base.getBaseOffset());
 
     Out << "  ";
     MD->printQualifiedName(Out);
     Out << " - (";
     Overrider.Method->printQualifiedName(Out);
     Out << ", " << Overrider.Offset.getQuantity() << ')';
 
if (PrimaryBase && !Layout.isPrimaryBaseVirtual()) {
     AddVCallOffsets(BaseSubobject(PrimaryBase, Base.getBaseOffset()),
                     VBaseOffset);
   }
   
   // Add the vcall offsets.
   for (const auto *MD : RD->methods()) {
     if (!MD->isVirtual())
       continue;
+    MD = MD->getCanonicalDecl();
 
     CharUnits OffsetOffset = getCurrentOffsetOffset();
     
     // Don't add a vcall offset if we already have one for this member function
     // signature.
     if (!VCallOffsets.AddVCallOffset(MD, OffsetOffset))
       continue;
 
void ItaniumVTableBuilder::AddMethods(
 
   typedef llvm::SmallVector<const CXXMethodDecl *, 8> NewVirtualFunctionsTy;
   NewVirtualFunctionsTy NewVirtualFunctions;
 
   // Now go through all virtual member functions and add them.
   for (const auto *MD : RD->methods()) {
     if (!MD->isVirtual())
       continue;
+    MD = MD->getCanonicalDecl();
 
     // Get the final overrider.
     FinalOverriders::OverriderInfo Overrider = 
       Overriders.getOverrider(MD, Base.getBaseOffset());
 
     // Check if this virtual member function overrides a method in a primary
     // base. If this is the case, and the return type doesn't require adjustment
     // then we can just use the member function from the primary base.
void ItaniumVTableBuilder::dumpLayout(raw_ostream &Out) {
   // Compute the vtable indices for all the member functions.
   // Store them in a map keyed by the index so we'll get a sorted table.
   std::map<uint64_t, std::string> IndicesMap;
 
   for (const auto *MD : MostDerivedClass->methods()) {
     // We only want virtual member functions.
     if (!MD->isVirtual())
       continue;
+    MD = MD->getCanonicalDecl();
 
     std::string MethodName =
       PredefinedExpr::ComputeName(PredefinedExpr::PrettyFunctionNoVirtual,
                                   MD);
 
     if (const CXXDestructorDecl *DD = dyn_cast<CXXDestructorDecl>(MD)) {
       GlobalDecl GD(DD, Dtor_Complete);
       assert(MethodVTableIndices.count(GD));

test/Modules/Inputs/merge-vtable-codegen/a.h

+#ifndef A_H
+#define A_H
+
+struct A {
+  virtual void x();
+};
+
+#endif

test/Modules/Inputs/merge-vtable-codegen/b.h

+#ifndef B_H
+#define B_H
+
+#include "a.h"
+
+class B : virtual public A {
+  virtual void x() {}
+};
+
+void b(A* p) {
+  p->x();
+  // Instantiating a class that virtually inherits 'A'
+  // triggers calculation of the vtable offsets in 'A'.
+  B b;
+}
+
+#endif

test/Modules/Inputs/merge-vtable-codegen/c.h

+#ifndef C_H
+#define C_H
+
+#include "a.h"
+
+#endif

test/Modules/Inputs/merge-vtable-codegen/merge-vtable-codegen.modulemap

+module "a" {
+  textual header "a.h"
+}
+
+module "b" {
+  header "b.h"
+}
+
+module "c" {
+  header "c.h"
+}

test/Modules/merge-vtable-codegen.cpp

+// RUN: rm -rf %t
+
+// First, build two modules that both re-export the same header.
+// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules -fmodule-name=b -o %t/b.pcm -fmodule-maps \
+// RUN:     -emit-module %S/Inputs/merge-vtable-codegen/merge-vtable-codegen.modulemap \
+// RUN:     -I %S/Inputs/merge-vtable-codegen
+// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules -fmodule-name=c -o %t/c.pcm -fmodule-maps \
+// RUN:     -emit-module %S/Inputs/merge-vtable-codegen/merge-vtable-codegen.modulemap \
+// RUN:     -I %S/Inputs/merge-vtable-codegen
+
+// Use the two modules in a single compile.
+// RUN: %clang_cc1 -x c++ -std=c++11 -fmodules -fmodule-file=%t/b.pcm -fmodule-file=%t/c.pcm \
+// RUN:     -fmodule-map-file=%S/Inputs/merge-vtable-codegen/merge-vtable-codegen.modulemap \
+// RUN:     -emit-llvm -o %t/test.o %s
+
+// Note that order is important:
+// Module 'c' just reexports A, while module 'b' defines a method that uses a
+// virtual method of A.
+#include "Inputs/merge-vtable-codegen/c.h"
+#include "Inputs/merge-vtable-codegen/b.h"
+
+void t() {
+  b(nullptr);
+}