This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by teemperor on May 19 2017, 1:35 PM.

Details

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.

Diff Detail

Event Timeline

teemperor created this revision.May 19 2017, 1:35 PM

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 edited edge metadata.May 19 2017, 3:39 PM

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 updated this revision to Diff 99728.May 22 2017, 2:54 AM
teemperor edited the summary of this revision. (Show Details)
  • Now unhiding/unowning the created functions like Richard suggested.
  • Extended the test case to stress test the lookup with the new visibility settings.
rsmith added inline comments.May 23 2017, 12:46 PM
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 updated this revision to Diff 101877.Jun 8 2017, 3:33 AM
  • 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.

teemperor marked an inline comment as done.Jun 8 2017, 3:33 AM
rsmith accepted this revision.Jun 8 2017, 10:56 AM

LGTM

This revision is now accepted and ready to land.Jun 8 2017, 10:56 AM
v.g.vassilev closed this revision.Jun 9 2017, 2:37 PM
v.g.vassilev edited edge metadata.

r305118.