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

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: docs/ClassScope.rst:2
@@ +1,3 @@
+===========
+Class Scope
+===========
----------------
pcc wrote:
> rsmith wrote:
> > pcc wrote:
> > > rsmith wrote:
> > > > Can you use some word other than "scope" here? "Class scope" is already a term of art in C++, meaning something completely different. I think what you're referring to is exactly the visibility of the class (in the ELF sense).
> > > Yes, this is pretty much visibility. I wanted to avoid using the term "visibility" because I'm introducing flags and attributes which can make scope mean something different to object file visibility, so I wanted to avoid the overload to avoid confusion.
> > > 
> > > Maybe the overloading would help with understanding though if I add a qualifying adjective. This is all about whether all derived classes are visible, so maybe the right term is something like "derived visibility"?
> > We already have attributes that can set the visibility of a class (which in turn affects the visibility of the vtable etc.) In what way is that different from what you're proposing? Is this a valuable difference, given the complexity of having two similar-but-different ways of describing the cross-DSO visibility of a class?
> Yes, ideally I'd like to just use visibility for this. There were a couple of things that motivated a design that separated the concepts, though:
> 
> 1. The part of the executable or DSO that we're LTOing may be a subset of the whole executable or DSO. If the user has prebuilt object files that they need to link into their executable or DSO, we need to exclude those classes from our analysis. One example of this would be the standard library. On most platforms this is marked up with default visibility attributes, so we're fine, but on Windows the platform ships an un-marked up standard library as a static library, and allows users to link to it. The idea is that Windows users who use the static standard library would be able to say
> ```
> namespace [[clang::global_scope]] std {}
> ```
> in a `-include`d header and get global scope on the standard library.
> 
> 2. I wanted `-fsanitize=cfi*` to turn on CFI checks on classes by default. It would be surprising to users if that flag on its own did not enable CFI on classes declared in the normal way, and it could easily lead to users shipping unprotected binaries. I suppose an alternative thing we could do would be to make `-fsanitize=cfi*` imply `-fvisibility=hidden`. Then if things break, they would at least be likely to break loudly.
I'm really not happy about having a third granularity for entity uniqueness (LTO granularity) between DSO granularity (visibility) and module granularity (linkage). But if we need that to accurately represent the state of the world, then perhaps there's no way to escape it (although if that is the intent, then the documentation here should be talking about something like LTO units rather than linkage units).

 1) Would it be inappropriate for entities in `namespace std` on Windows to be given default visibility when people do this?

 2) I don't think it makes sense for the `-fsanitize=cfi*` flag to enable "types can only be declared within their own LTO unit by default" semantics, whether those semantics are related to devirtualization, CFI, or symbol visibility. Sanitizer flags are supposed to turn on checks for the ambient configuration, not change what the code means; I'd prefer to say that it's an error if you enable `-fsanitize=cfi*` and don't *also* enable `fvisibility=hidden` (or a hypothetical `-fclass-visibility=hidden`).


http://reviews.llvm.org/D18635