This is an archive of the discontinued LLVM Phabricator instance.

Fix several problems at the intersection of template instantiations and visibility
AcceptedPublic

Authored by loladiro on Oct 3 2015, 8:09 PM.

Details

Reviewers
rnk
rsmith
Summary

When we were looking at a template instantiation, that itself was a template instantiation
(say a templated member of a templated class), we weren't looking back far enough along
the chain of instantiations to find a VisibilityAttr (which we don't copy when instantiating
templates). This patch attempts to address that as well as adding a few test cases for these
situations.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 36455.Oct 3 2015, 8:09 PM
loladiro retitled this revision from to Fix several problems at the intersection of template instantiations and visibility.
loladiro updated this object.
loladiro added reviewers: aaron.ballman, rsmith, rnk.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: cfe-commits.
rsmith edited reviewers, added: rafael; removed: rsmith.Oct 5 2015, 1:31 PM
rsmith added a reviewer: rsmith.
rsmith added inline comments.Oct 5 2015, 1:42 PM
lib/AST/Decl.cpp
1088–1089

typo "of a some"

lib/AST/DeclCXX.cpp
1263–1269

This loop structure is not very clear. How about:

while (!CTD->isMemberSpecialization()) {
  auto *NewCTD = CTD->getInstantiatedFromMemberTemplate();
  if (!NewCTD)
    break;
  CTD = NewCTD;
}

... or

while (auto *FromCTD = CTD->isMemberSpecialization()
                           ? nullptr
                           : CTD->getInstantiatedFromMemberTemplate())
  CTD = FromCTD;
loladiro updated this revision to Diff 36565.Oct 5 2015, 3:24 PM

Address review comment re loop structure. Reword comment that had a typo to both fix the typo and make the intent of the code more clear.

aaron.ballman resigned from this revision.Oct 13 2015, 11:35 AM
aaron.ballman removed a reviewer: aaron.ballman.

Bumping this again.

rnk accepted this revision.Mar 16 2016, 11:12 AM
rnk edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 16 2016, 11:12 AM

Looks like patch was not committed.

Since this was approved, I'll rebase and commit.

Hmm, the rebased version of this introduces new errors in test/Modules/cxx-templates.cpp. That test didn't exist when I wrote this code and I'm not familiar with templates. @rsmith could you take a look, error is:

error: 'error' diagnostics seen but not expected:
  File /data/keno/llvm/tools/clang/test/Modules/cxx-templates.cpp Line 202: definition of 'nested_cls_t' must be imported from module 'cxx_templates_common.unimported' before it is required
  File /data/keno/llvm/tools/clang/test/Modules/cxx-templates.cpp Line 202: definition of 'nested_cls_t' must be imported from module 'cxx_templates_common.unimported' before it is required
loladiro updated this revision to Diff 76094.Oct 27 2016, 1:52 PM
loladiro edited edge metadata.

Rebased patch

I just noticed that this patch fixes a template crash that I have a patch for at https://reviews.llvm.org/D25942. I will abandon my patch since this patch fixes the other issue. Would you mind adding the test case from my patch to this patch? It's available at https://reviews.llvm.org/differential/changeset/?ref=558279&whitespace=ignore-most .

Hmm, the rebased version of this introduces new errors in test/Modules/cxx-templates.cpp. That test didn't exist when I wrote this code and I'm not familiar with templates. @rsmith could you take a look, error is:

error: 'error' diagnostics seen but not expected:
  File /data/keno/llvm/tools/clang/test/Modules/cxx-templates.cpp Line 202: definition of 'nested_cls_t' must be imported from module 'cxx_templates_common.unimported' before it is required
  File /data/keno/llvm/tools/clang/test/Modules/cxx-templates.cpp Line 202: definition of 'nested_cls_t' must be imported from module 'cxx_templates_common.unimported' before it is required

FWIW, I also found this issue while working on my patch and decided to add expected-error 1+{{definition of}} verified check to that line ( https://reviews.llvm.org/differential/changeset/?ref=558280&whitespace=ignore-most). It seems that the preceding lines have this check as well, so I assumed that this diagnostic was expected and my patch fixed a bug that prevented this diagnostic from showing up.

loladiro updated this revision to Diff 76257.Oct 28 2016, 3:09 PM

Add the expected-error annotation

rsmith edited edge metadata.Oct 28 2016, 3:25 PM

Please factor out the fix for getTemplateInstantiationPattern and commit it separately. D25942 has a testcase for that portion of this change.

lib/AST/Decl.cpp
1054–1055

This doesn't seem to match GCC; GCC seems to do this for all kinds of member specialization, not just for member class specialization:

template<typename T> struct A {
  template<typename U> struct __attribute__((visibility("hidden"))) B;
};
template<> template<typename U> struct A<int>::B { virtual void f() {} };
void g() { A<int>::B<int>().f(); }

Here, A<int>::B<int> gets hidden visibility, even though it was instantiated from the member template specialization A<int>::B which had no visibility attribute.

I don't know what the underlying logic is here -- does GCC look at the visibility on the enclosing class when computing visibility for a member class? -- but I don't think this one special case covers it.

1092–1093

Likewise.

lib/AST/DeclCXX.cpp
1264–1269

The same bug exists in VarDecl::getTemplateInstantiationPattern; can you fix it there too?

loladiro added inline comments.Oct 28 2016, 9:22 PM
lib/AST/DeclCXX.cpp
1264–1269

Do you have a test case that shows a problem with the current definition of that function? Would like to include it if possible. I tried cooking one up, but wasn't particularly successful.

rsmith added inline comments.Oct 29 2016, 10:51 AM
lib/AST/DeclCXX.cpp
1264–1269

Since this is only currently used by the modules visibility code, you'd presumably need something like this:

// submodule a.x
template<typename T> struct A {
  template<typename U> static const int n;
};
// submodule a.y
import a.x
template<typename T> template<typename U> const int A<T>::n = 1;
// submodule a.z
import a.x
template<> template<typename U> const int A<int>::n = 2;
// test
import a.z
// I expect this to fail because we would incorrectly check to see whether the definition from a.y is visible, because we never check whether the definition from a.z is a member specialization.
static_assert(A<int>::n<int> == 2);

I'm really out of my depth in this code, but it looks like that test case is triggering the one code path in that function that is actually correct. Could you adjust it to trigger the code path behind the first if statement?