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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
