[PATCH] D18635: Rework interface for bitset-using features to use a notion of LTO visibility.

Wed Apr 20 16:48:45 PDT 2016

pcc added a comment.

Thanks for the comments. I've also taken another pass over the docs and changed them to specify that classes defined in translation units compiled without LTO receive public LTO visibility. This seems more precise to me, but please let me know what you think.

I have also added an ASCII art diagram with some examples.

I have also updated the implementation to match the documentation. We now correctly implement the case where only some translation units are compiled with CFI or vtable opt by emitting bitsets for translation units where only LTO is enabled. I considered the alternative of adding another type of "unit", but felt that that would add too much complexity.


================
Comment at: docs/LTOVisibility.rst:5-8
@@ +4,6 @@
+
+LTO visibility is a concept used by the compiler to determine which classes
+the virtual function call optimization and control flow integrity features
+apply to. These features use whole-program information, so they require
+visibility of entire class hierarchies to work correctly.
+
----------------
rsmith wrote:
> Please start with a definition of this that says what it means separate from how we're going to use it. Something like:
> 
> """
> //LTO visibility// is a property of an entity that specifies whether it can be referenced from outside the current LTO unit. An //LTO unit// is one or more translation units that are linked together using link-time optimization; in the case where LTO is not being used, each translation unit is a separate LTO unit.
> 
> The LTO visibility of a class is used by the compiler to determine whether certain virtual function call optimizations and control flow integrity features can be applied to the class. These features use whole-program information, so they require the entire class hierarchy to be visible in order to work correctly.
> """
Done, except for:

> in the case where LTO is not being used, each translation unit is a separate LTO unit

This part is inaccurate; the compiler features we're talking about require `-flto` at the moment. If we do decide to allow this, that should probably be a separate discussion.

================
Comment at: docs/LTOVisibility.rst:10-12
@@ +9,5 @@
+
+It is effectively an ODR violation to declare a class with hidden LTO
+visibility in multiple linkage units, or to declare such a class in an
+translation unit not built with LTO. A class with default LTO visibility
+has no such restrictions, but the tradeoff is that the virtual function
----------------
rsmith wrote:
> "[...] or to declare such a class in multiple translation units not built with LTO."
I don't think we need this part as long as we have the LTO requirement.

================
Comment at: docs/LTOVisibility.rst:11
@@ +10,3 @@
+It is effectively an ODR violation to declare a class with hidden LTO
+visibility in multiple linkage units, or to declare such a class in an
+translation unit not built with LTO. A class with default LTO visibility
----------------
rsmith wrote:
> What do you mean by linkage units here? Do you mean DSOs? (In which case this seems imprecise, since I can link multiple LTO units into a single DSO.)
I mean the contents of a DSO or executable. I have added a definition. I thought this term was defined in the standard, but I guess I must have picked it up from a draft proposal [1] or somewhere else.

I have updated the documentation to clarify that a linkage unit may have only a single LTO unit.

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2003/n1428.html

================
Comment at: docs/LTOVisibility.rst:28-30
@@ +27,5 @@
+
+1. If a linkage unit is produced from a combination of LTO object files and
+   non-LTO object files, any classes defined in translation units from which
+   the non-LTO object files were built will require default LTO visibility.
+
----------------
rsmith wrote:
> The LTO'd files may also need the attribute if the classes are referenced by the non-LTO'd files.
The attribute can only have an effect on generated code if a virtual call is made using the class. Since virtual functions can only be called on classes with definitions, I think this restriction is sufficient. In any case, I've rewritten it to try to clarify it.

================
Comment at: docs/LTOVisibility.rst:37-43
@@ +36,8 @@
+
+Classes that fall into either of these categories can be marked up with
+the ``[[clang::lto_visibility_default]]`` attribute. To specifically
+handle the COM case, the ``__declspec(uuid())`` attribute implies
+``[[clang::lto_visibility_default]]``. On Windows platforms, the ``/MT`` and
+``/MTd`` flags link the program against a prebuilt static standard library;
+these flags imply default LTO visibility for every class declared in the
+``std`` and `stdext`` namespaces.
----------------
rsmith wrote:
> "default" is a terrible name for this. It was a mistake for the GNU attribute to use "default" to mean "public"; let's not replicate this mistake for LTO visibility.
Agreed, done.

================
Comment at: docs/LTOVisibility.rst:39-40
@@ +38,4 @@
+the ``[[clang::lto_visibility_default]]`` attribute. To specifically
+handle the COM case, the ``__declspec(uuid())`` attribute implies
+``[[clang::lto_visibility_default]]``. On Windows platforms, the ``/MT`` and
+``/MTd`` flags link the program against a prebuilt static standard library;
----------------
rsmith wrote:
> It seems to me that `__declspec(uuid(...))` should notionally imply that the class is visible to other DSOs, not just that it's visible outside the LTO unit within its DSO. I'm not sure we have a reasonable way to model that, though (dllexport and default visibility both imply other things that we don't really mean here).
> It seems to me that __declspec(uuid(...)) should notionally imply that the class is visible to other DSOs, not just that it's visible outside the LTO unit within its DSO

Yes. I have changed the docs to state that this is exactly what public LTO visibility means ("A class with public LTO visibility may be defined in multiple linkage units...").

> Don't say the attributes is implied here; instead say the semantics are implied

Done.

================
Comment at: docs/UsersManual.rst:1059-1060
@@ -1058,13 +1058,4 @@
    Enable whole-program vtable optimizations, such as single-implementation
-   devirtualization and virtual constant propagation. Requires ``-flto``.
-
-   By default, the compiler will assume that all type hierarchies are
-   closed except those in the ``std`` namespace, the ``stdext`` namespace
-   and classes with the ``__declspec(uuid())`` attribute.
-
-.. option:: -fwhole-program-vtables-blacklist=path
-
-   Allows the user to specify the path to a list of additional classes to
-   blacklist from whole-program vtable optimizations. This list is in the
-   :ref:`CFI blacklist <cfi-blacklist>` format.
+   devirtualization and virtual constant propagation, for classes with internal
+   linkage or :doc:`hidden LTO visibility <LTOVisibility>`. Requires ``-flto``.
 
----------------
rsmith wrote:
> I would think internal linkage should imply hidden LTO visibility.
Agreed. I've updated LTOVisibility.rst.

================
Comment at: include/clang/Driver/CC1Options.td:287
@@ -285,1 +286,3 @@
+    Flag<["-"], "flto-visibility-default-std">,
+    HelpText<"Use default LTO visibility for classes in std and stdext namespaces">;
 
----------------
rsmith wrote:
> Looks like `stdext` is an MS STL thing; can we limit the special-casing to just MS targets?
It is already so limited. This is a -cc1 only flag and is enabled only if the clang-cl driver is being used.


http://reviews.llvm.org/D18635



