This is an archive of the discontinued LLVM Phabricator instance.

[clang] Add the exclude_from_explicit_instantiation attribute
ClosedPublic

Authored by ldionne on Sep 7 2018, 8:03 AM.

Details

Summary

This attribute allows excluding a member of a class template from being part
of an explicit template instantiation of that class template. This also makes
sure that code using such a member will not take for granted that an external
instantiation exists in another translation unit. The attribute was discussed
on cfe-dev at [1] and is primarily motivated by the removal of always_inline
in libc++ to control what's part of the ABI (see links in [1]).

[1]: http://lists.llvm.org/pipermail/cfe-dev/2018-August/059024.html

rdar://problem/43428125

Diff Detail

Repository
rC Clang

Event Timeline

ldionne created this revision.Sep 7 2018, 8:03 AM

This patch is an early WIP -- I'm just uploading it to serve as a support for a message I'm about to post on cfe-dev. No need to review seriously for now.

dexonsmith edited subscribers, added: cfe-commits; removed: llvm-commits.Sep 7 2018, 8:46 AM

+cfe-commits
-llvm-commits

It'd be good to test that [[no_extern_template]] affects instantiation, not just code generation (eg, put something in the body of the entity that will trigger an error if instantiated, and check that the diagnostic is produced at the right times).

ldionne updated this revision to Diff 165507.Sep 14 2018, 7:36 AM

Fix the tests and remove some warnings that I wasn't able to generate properly
(to avoid false positives).

ldionne retitled this revision from [WIP][clang] Add the no_extern_template attribute to [clang] Add the no_extern_template attribute.Sep 14 2018, 7:37 AM

I think now's a good time to bikeshed the name of the attribute if you have other suggestions.

I think now's a good time to bikeshed the name of the attribute if you have other suggestions.

OK, so the semantics of this attribute are "explicit instantiation declarations or definitions applied to the enclosing class do not apply to this member", right? So it opts the member out of both extern template and (non-extern) template, but only when applied to an enclosing class.

Maybe something like exclude_from_explicit_instantiation? That's a little longer than I'd like, but if it's going to be hidden behind a macro most of the time I could live with it.
Or maybe no_transitive_instantiation? That's less explicit about the purpose of the attribute, but is a more comfortable length.

I think now's a good time to bikeshed the name of the attribute if you have other suggestions.

OK, so the semantics of this attribute are "explicit instantiation declarations or definitions applied to the enclosing class do not apply to this member", right? So it opts the member out of both extern template and (non-extern) template, but only when applied to an enclosing class.

Yes, but it also (obviously) opts the member out when applied to the member itself, not only to an enclosing class.

Maybe something like exclude_from_explicit_instantiation? That's a little longer than I'd like, but if it's going to be hidden behind a macro most of the time I could live with it.

I like this a lot too. I thought about that one and was deterred by the length, but it may be acceptable. Realistically, it's also not an attribute we want people to use most of the time.

Or maybe no_transitive_instantiation? That's less explicit about the purpose of the attribute, but is a more comfortable length.

I'm not sure this one describes what the attribute is doing sufficiently closely.

Unless we get a better suggestion soonish, I'll change it to exclude_from_explicit_instantiation and update the diff.

OK, so the semantics of this attribute are "explicit instantiation declarations or definitions applied to the enclosing class do not apply to this member", right? So it opts the member out of both extern template and (non-extern) template, but only when applied to an enclosing class.

Yes, but it also (obviously) opts the member out when applied to the member itself, not only to an enclosing class.

Assuming I'm understanding you correctly, I don't think the current implementation does that (nor does the documentation change say that), and I think that's a feature not a bug. For example, given:

template<typename T> struct A {
  [[clang::no_extern_template]] void f() {}
  void g() {}
};
template struct A<int>; // instantiates A<int> and definition of A<int>::g(), does not instantiate definition of A<int>::f()
template void A<int>::f(); // instantiates definition of A<int>::f() despite attribute

... the attribute affects the first explicit instantiation but not the second (I think you're saying it should affect both, and make the second explicit instantiation have no effect). I think the current behavior in this patch is appropriate: making the attribute also affect the second case makes it strictly less useful. If you didn't want to actually explictly instantiate A<int>::f(), you can just leave out that explicit instantiation. But the current implementation gives you extra capabilities:

// A<float>::g() is instantiated somewhere else, A<float>::f() still needs to be implicitly instantiated when used
extern template struct A<float>;

// A<int>::f() and A<int>::g() are both instantiated somewhere else
extern template struct A<int>;
extern template void A<int>::f();

You can't do that if an explicit instantiation of a member with the attribute has no effect.

OK, so the semantics of this attribute are "explicit instantiation declarations or definitions applied to the enclosing class do not apply to this member", right? So it opts the member out of both extern template and (non-extern) template, but only when applied to an enclosing class.

Yes, but it also (obviously) opts the member out when applied to the member itself, not only to an enclosing class.

Assuming I'm understanding you correctly, I don't think the current implementation does that (nor does the documentation change say that), and I think that's a feature not a bug. For example, given:

template<typename T> struct A {
  [[clang::no_extern_template]] void f() {}
  void g() {}
};
template struct A<int>; // instantiates A<int> and definition of A<int>::g(), does not instantiate definition of A<int>::f()
template void A<int>::f(); // instantiates definition of A<int>::f() despite attribute

... the attribute affects the first explicit instantiation but not the second (I think you're saying it should affect both, and make the second explicit instantiation have no effect). I think the current behavior in this patch is appropriate: making the attribute also affect the second case makes it strictly less useful. [...]

Sorry, that's a misunderstanding. We're on the same page. What I meant is that if you apply the attribute to the entity itself but explicitly instantiate the enclosing class, then the entity is not instantiated. It's very obvious, but your previous comment:

So it opts the member out of both extern template and (non-extern) template, but only when applied to an enclosing class.

suggested that it only applied in the following case:

template <class T> struct Foo {
  struct __attribute__((no_extern_template)) nested { static void f() { } };
};

template struct Foo<int>; // Foo<int>::nested::f not instantiated
ldionne updated this revision to Diff 165569.Sep 14 2018, 12:37 PM

Change no_extern_template to exclude_from_explicit_instantiation

ldionne retitled this revision from [clang] Add the no_extern_template attribute to [clang] Add the exclude_from_explicit_instantiation attribute.Sep 14 2018, 12:38 PM

Looks good other than the warning, which I don't yet understand.

clang/include/clang/Basic/DiagnosticSemaKinds.td
4683–4686 ↗(On Diff #165569)

Diagnostics should start with a lowercase letter and not end with a period.

That said, I'm not sure I see why this diagnostic is correct / useful. If the entity is never used, then there's no link error. And if it is ever used, then you should get an implicit instantiation like normal, and we already have a diagnostic for the case where an entity is implicitly instantiated and no definition is available.

clang/lib/Sema/SemaTemplateInstantiate.cpp
2581–2582 ↗(On Diff #165569)

Nit: we prefer to left-align continuation lines (clang-format will do that for you).

ldionne added inline comments.Sep 17 2018, 7:18 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
4683–4686 ↗(On Diff #165569)

Diagnostics should start with a lowercase letter and not end with a period.

Done.

That said, I'm not sure I see why this diagnostic is correct / useful. If the entity is never used, then there's no link error. And if it is ever used, then you should get an implicit instantiation like normal, and we already have a diagnostic for the case where an entity is implicitly instantiated and no definition is available.

This is not what happens right now. If you don't provide a definition but you try to call the function, an extern call will be produced (and that will result in a link error because any potential explicit instantiation won't provide the function). For example:

cat <<EOF | ./install/bin/clang++ -cc1 -stdlib=libc++ -xc++ -emit-llvm -o - -
template <class T>
struct Foo {
  __attribute__((exclude_from_explicit_instantiation)) static void static_member_function();
};

extern template struct Foo<int>;

int main() {
  Foo<int>::static_member_function();
}
EOF

Results in the following LLVM IR:

; Function Attrs: noinline norecurse nounwind optnone
define i32 @main() #0 {
entry:
  call void @_ZN3FooIiE22static_member_functionEv()
  ret i32 0
}

declare void @_ZN3FooIiE22static_member_functionEv() #1

I guess we should be getting a warning or an error on the point of implicit instantiation instead, or is this behavior acceptable?

clang/lib/Sema/SemaTemplateInstantiate.cpp
2581–2582 ↗(On Diff #165569)

Thanks for the heads up. I ran clang-format on all the lines I touched in this file.

ldionne updated this revision to Diff 165766.Sep 17 2018, 7:19 AM

Reformat warning message and run clang-format on changed lines.

aaron.ballman added inline comments.
clang/include/clang/Basic/AttrDocs.td
2990 ↗(On Diff #165766)

An example that demonstrates how the attribute behaves would be appreciated. Having some exposition that explains why a user might want to write this attribute themselves would also be good.

ldionne updated this revision to Diff 165768.Sep 17 2018, 7:49 AM

Add example of how to use the attribute, per Aaron's comment.

ldionne marked an inline comment as done.Sep 17 2018, 7:49 AM

I'm thinking about this some more, and I'm wondering whether it would be a viable solution to just exclude members marked with hidden visibility from explicit template instantiations (and declarations thereof). I thought I had been convinced by Hubert that visibility and this attribute should be left separate, but now I'm not sure anymore. I don't think there's a single case of a member function with hidden visibility that we don't want to exclude from explicit instantiations. @rsmith Thoughts?

That may work for libc++'s purposes, but it's clearly inappropriate as a compiler rule. There are good reasons why something with hidden visibility would need to be explicitly instantiated. For many programmers, hidden visibility means "this is private to my library", not "this is actually public to my library, but I'm walking an ABI tightrope".

That may work for libc++'s purposes, but it's clearly inappropriate as a compiler rule. There are good reasons why something with hidden visibility would need to be explicitly instantiated.

I take your word for it, but I can't think of any example. For my education, do you have a specific example in mind?

For many programmers, hidden visibility means "this is private to my library", not "this is actually public to my library, but I'm walking an ABI tightrope".

In libc++'s case, the functions we will annotate with exclude_from_explicit_instantiation are private to libc++ too (in the sense that we don't want them part of the ABI and they are not exported from the dylib). Those functions were previously marked with __always_inline__ to make sure they were not part of the ABI.

Note that I'm quite happy with exclude_from_explicit_instantiation as it solves libc++'s problem -- I'm trying to see whether another solution would serve people better while still solving libc++'s problem. (Appart from explicitly exporting functions, typeinfos and vtables like we've talked about on cfe-dev, which is a superior solution to everything else but is left as a future improvement for the time being).

That may work for libc++'s purposes, but it's clearly inappropriate as a compiler rule. There are good reasons why something with hidden visibility would need to be explicitly instantiated.

I take your word for it, but I can't think of any example. For my education, do you have a specific example in mind?

I mean, most code doesn't use explicit instantiations to begin with, but — the general idea would be someone using an explicit instantiation, either for compile-time or seperate-compilation reasons, for some type that's entirely private to their library.

Here's an example of it from Swift that happened to be easy to find:

https://github.com/apple/swift/blob/master/lib/SILOptimizer/ARC/RCStateTransitionVisitors.h

The entire template being instantiated there is private to the SILOptimizer library. Swift doesn't use explicit visibility attributes much, preferring to globally assume -fvisibility=hidden, but if we used them, there would definitely be an attribute on that template.

For many programmers, hidden visibility means "this is private to my library", not "this is actually public to my library, but I'm walking an ABI tightrope".

In libc++'s case, the functions we will annotate with exclude_from_explicit_instantiation are private to libc++ too (in the sense that we don't want them part of the ABI and they are not exported from the dylib). Those functions were previously marked with __always_inline__ to make sure they were not part of the ABI.

Yeah, I understand the use case. That's what I was calling an ABI tightrope. The functions you're annotating are still part of libc++'s *logical* interface, they're just not exported by the dylib.

Note that I'm quite happy with exclude_from_explicit_instantiation as it solves libc++'s problem -- I'm trying to see whether another solution would serve people better while still solving libc++'s problem. (Appart from explicitly exporting functions, typeinfos and vtables like we've talked about on cfe-dev, which is a superior solution to everything else but is left as a future improvement for the time being).

Understood.

John.

rsmith added inline comments.Sep 21 2018, 5:04 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
4683–4686 ↗(On Diff #165569)

I don't think your example is fundamentally any different from:

template <class T>
struct Foo {
  static void static_member_function();
};

int main() {
  Foo<int>::static_member_function();
}

which likewise produces a declaration of _ZN3FooIiE22static_member_functionEv. That case produces a -Wundefined-func-template warning; your example should do the same.

ldionne updated this revision to Diff 166590.Sep 21 2018, 6:32 PM

Fix warnings on undefined entities

rsmith accepted this revision.Oct 3 2018, 6:06 PM
rsmith added inline comments.
clang/lib/Sema/Sema.cpp
648 ↗(On Diff #166590)

What's the purpose of this change?

clang/test/SemaCXX/attr-exclude_from_explicit_instantiation.diagnose_on_undefined_entity.cpp
29 ↗(On Diff #166590)

I generally prefer to use relative line numbers (@-5) rather than absolute ones, so that the test doesn't break if unrelated things are changed earlier in the file.

This revision is now accepted and ready to land.Oct 3 2018, 6:06 PM
ldionne marked an inline comment as done.Oct 4 2018, 8:50 AM
ldionne added inline comments.
clang/lib/Sema/Sema.cpp
648 ↗(On Diff #166590)

It's so that we diagnose a member function marked with exclude_from_explicit_instantiation that is used but not defined just like we do for an inline function. This causes us to push_back the declaration (line 668 below) into the vector of Undefined-but-used declarations, which we use to trigger a diagnostic somewhere else.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.