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

[Richard Smith via cfe-commits]

rsmith added a comment.

Generally-speaking, I think this is looking really good. I think we need to be careful and precise in how we document this new visibility notion; most of my comments are on that.

I wonder if we'd benefit from a diagram in the documentation showing the nesting levels between object files (translated translation units in C / C++ parlance), LTO units (or whatever you want to call them), and DSOs (or linkage units or whatever you want to call executable / .so / .dylib / .dll files), and precise definitions of the terms we use for each. The two of us seem to be using different terms for the same entities, so a single set of terms we define and use consistently -- at least within Clang's documentation -- would be great.


================
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.
+
----------------
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.
"""

================
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
----------------
"[...] or to declare such a class in multiple translation units not built with LTO."

================
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
----------------
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.)

================
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.
+
----------------
The LTO'd files may also need the attribute if the classes are referenced by the non-LTO'd files.

================
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.
----------------
"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.

================
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;
----------------
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).

================
Comment at: docs/LTOVisibility.rst:40
@@ +39,3 @@
+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;
----------------
Don't say the attributes is implied here; instead say the semantics are implied -- "classes with the `__declspec(uuid())` attribute receive public LTO visibility" or similar?

================
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``.
 
----------------
I would think internal linkage should imply hidden LTO visibility.

================
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">;
 
----------------
Looks like `stdext` is an MS STL thing; can we limit the special-casing to just MS targets?

================
Comment at: lib/CodeGen/CGVTables.cpp:925-926
@@ +924,4 @@
+      auto *D = cast<Decl>(DC);
+      DC = DC->getParent();
+      if (isa<TranslationUnitDecl>(DC)) {
+        if (auto *ND = dyn_cast<NamespaceDecl>(D))
----------------
The `namespace std` declaration might be in an extern "C" or extern "C++" context; you should check the redecl context of `DC` here (cf `DeclContext::isStdNamespace`).

================
Comment at: lib/CodeGen/CGVTables.cpp:936-939
@@ -928,1 +935,6 @@
+
+  if (getTriple().isOSBinFormatCOFF())
+    return !RD->hasAttr<DLLExportAttr>() && !RD->hasAttr<DLLImportAttr>();
+  else
+    return LV.getVisibility() == HiddenVisibility;
 }
----------------
Maybe perform these checks before the (likely more expensive) check for an enclosing `std` namespace?


http://reviews.llvm.org/D18635