Fix that global delete operator get's assigned to a submodule.

[teemperor]

Summary

In the current local-submodule-visibility mode, as soon as we discover a virtual destructor, we declare on demand a global delete operator. However, this causes that this delete operator is owned by the submodule which contains said virtual destructor. This means that other modules no longer can see the global delete operator which is hidden inside another submodule and fail to compile.

This patch unhides those global allocation function once they're created to prevent this issue.

[teemperor]

I'm not 100% sure if that's the right fix, but I didn't see a obvious way to declare the new/delete outside the current submodule. Maybe Richard knows a better way :)

[rsmith]

This is supposed to be handled by Sema::DeclareGlobalAllocationFunction:

DeclContext::lookup_result R = GlobalCtx->lookup(Name);
for (DeclContext::lookup_iterator Alloc = R.begin(), AllocEnd = R.end();
     Alloc != AllocEnd; ++Alloc) {
  // Only look at non-template functions, as it is the predefined,
  // non-templated allocation function we are trying to declare here.
  if (FunctionDecl *Func = dyn_cast<FunctionDecl>(*Alloc)) {
    if (Func->getNumParams() == Params.size()) {
      llvm::SmallVector<QualType, 3> FuncParams;
      for (auto *P : Func->parameters())
        FuncParams.push_back(
            Context.getCanonicalType(P->getType().getUnqualifiedType()));
      if (llvm::makeArrayRef(FuncParams) == Params) {
        // Make the function visible to name lookup, even if we found it in
        // an unimported module. It either is an implicitly-declared global
        // allocation function, or is suppressing that function.
        Func->setHidden(false);
        return;

... but it looks like we only get there once per compilation, which means that we won't unset the 'hidden' flag if we create the functions in one submodule and then need them from a second one, as in your testcase.

A better approach would be to call Alloc->setHidden(false) (and maybe also Alloc->setLocalOwningModule(nullptr)) after we create the implicit declaration in the CreateAllocationFunctionDecl lambda, rather than waiting until the second time we want it.

[teemperor]

• Now unhiding/unowning the created functions like Richard suggested.
• Extended the test case to stress test the lookup with the new visibility settings.

[rsmith]

lib/Sema/SemaExprCXX.cpp

2661–2669

Let's just unconditionally setHidden(false) and leave the owning module alone.

(On reflection, changing the owning module is problematic because we'll assert if the declaration is not already marked as hidden, which the above checks are not sufficient to ensure, and it doesn't seem necessary, since the owning module doesn't have any visibility impact if the declaration is not hidden.)

I'd also prefer to not restrict this to local visibility mode / to the case where we have a current module if we don't need to (and as far as I can tell, we shouldn't need to do so).

[teemperor]

• Just unconditionally calling setHidden now.

IIRC I had to put the conditions for the module builds because setLocalOwningModule did trigger some tests/asserts, otherwise but the setHidden works fine like this.

[rsmith]

LGTM

[v.g.vassilev]

r305118.

lib/Sema/SemaExprCXX.cpp

   }
 
   auto CreateAllocationFunctionDecl = [&](Attr *ExtraAttr) {
     QualType FnType = Context.getFunctionType(Return, Params, EPI);
     FunctionDecl *Alloc = FunctionDecl::Create(
         Context, GlobalCtx, SourceLocation(), SourceLocation(), Name,
         FnType, /*TInfo=*/nullptr, SC_None, false, true);
     Alloc->setImplicit();
+    // If we're in local visibility mode, we reuse this allocation function
+    // in all submodules of the current module. To make sure that the other
+    // submodules can lookup this function, we unhide it and make sure it's
+    // not owned by the current submodule.
+    if (getLangOpts().ModulesLocalVisibility &&
+        !getLangOpts().CurrentModule.empty()) {
+      Alloc->setLocalOwningModule(nullptr);
+      Alloc->setHidden(false);
+    }
 
     // Implicit sized deallocation functions always have default visibility.
     Alloc->addAttr(
         VisibilityAttr::CreateImplicit(Context, VisibilityAttr::Default));
 
     llvm::SmallVector<ParmVarDecl *, 3> ParamDecls;
     for (QualType T : Params) {
       ParamDecls.push_back(ParmVarDecl::Create(

test/Modules/Inputs/local-submodule-globaldelete/a.h

+#ifndef A_H
+#define A_H
+class A {
+  virtual ~A(){};
+};
+#endif

test/Modules/Inputs/local-submodule-globaldelete/b.h

+#ifndef B_H
+#define B_H
+class B {
+  virtual ~B(){};
+};
+#endif

test/Modules/Inputs/local-submodule-globaldelete/c.h

+#ifndef C_H
+#define C_H
+class C {
+  virtual ~C(){};
+};
+#endif

test/Modules/Inputs/local-submodule-globaldelete/module.modulemap

+module M {
+  module a { header "a.h" export * }
+  module b { header "b.h" export * }
+}
+module Other {
+  module c { header "c.h" export * }
+}

test/Modules/local-submodule-globaldelete.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cp -r %S/Inputs/local-submodule-globaldelete %t/local-submodule-globaldelete
+// RUN: cd %t
+// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility -verify -fmodules-cache-path=%t  -fimplicit-module-maps -I local-submodule-globaldelete %s -H
+
+// expected-no-diagnostics
+
+// This tests that the global delete operator is not by accident assigned
+// to a submodule in local-submodule-visibility mode. This could happen
+// when the operator is created on demand when we discover a virtual destructor
+// inside a module.
+
+// Build a module with submodules that each need the global delete operator.
+#include "a.h"
+// Build another module with another global delete operator to check that
+// we didn't introduced ambiguity in the lookup.
+#include "c.h"
+
+// Require a lookup of the global delete operator.
+class T {
+  virtual ~T() {}
+};