Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc updated this revision to Diff 52759.
pcc added a comment.
- New implementation
http://reviews.llvm.org/D18635
Files:
docs/ControlFlowIntegrity.rst
docs/LTOVisibility.rst
docs/UsersManual.rst
docs/index.rst
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Driver/CC1Options.td
include/clang/Driver/Options.td
include/clang/Frontend/CodeGenOptions.def
include/clang/Frontend/CodeGenOptions.h
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGVTables.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/MicrosoftCXXABI.cpp
lib/Driver/SanitizerArgs.cpp
lib/Driver/Tools.cpp
lib/Frontend/CompilerInvocation.cpp
lib/Sema/SemaDeclAttr.cpp
runtime/CMakeLists.txt
runtime/vtables_blacklist.txt
test/CodeGenCXX/bitset-blacklist.cpp
test/CodeGenCXX/bitset-inference.cpp
test/CodeGenCXX/bitsets.cpp
test/CodeGenCXX/cfi-blacklist.cpp
test/CodeGenCXX/cfi-cast.cpp
test/CodeGenCXX/cfi-nvcall.cpp
test/CodeGenCXX/cfi-stats.cpp
test/Driver/cl-runtime-flags.c
test/Driver/fsanitize.c
test/Driver/whole-program-vtables.c
test/Frontend/dependency-gen.c
test/SemaCXX/attr-lto-visibility-default.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2527,6 +2527,7 @@
case ObjCProtocol | ObjCInterface:
return "ExpectedObjectiveCInterfaceOrProtocol";
case Field | Var: return "ExpectedFieldOrGlobalVar";
+case GenericRecord | Namespace: return "ExpectedRecordOrNamespace";
}
PrintFatalError(S.getLoc(),
Index: test/SemaCXX/attr-lto-visibility-default.cpp
===
--- /dev/null
+++ test/SemaCXX/attr-lto-visibility-default.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+
+int i [[clang::lto_visibility_default]]; // expected-warning {{'lto_visibility_default' attribute only applies to struct, union or class}}
+typedef int t [[clang::lto_visibility_default]]; // expected-warning {{'lto_visibility_default' attribute only applies to struct, union or class}}
+[[clang::lto_visibility_default]] void f(); // expected-warning {{'lto_visibility_default' attribute only applies to struct, union or class}}
+void f() [[clang::lto_visibility_default]]; // expected-error {{'lto_visibility_default' attribute cannot be applied to types}}
+
+struct [[clang::lto_visibility_default]] s1 {
+ int i [[clang::lto_visibility_default]]; // expected-warning {{'lto_visibility_default' attribute only applies to struct, union or class}}
+ [[clang::lto_visibility_default]] void f(); // expected-warning {{'lto_visibility_default' attribute only applies to struct, union or class}}
+};
+
+struct [[clang::lto_visibility_default(1)]] s2 { // expected-error {{'lto_visibility_default' attribute takes no arguments}}
+};
Index: test/Frontend/dependency-gen.c
===
--- test/Frontend/dependency-gen.c
+++ test/Frontend/dependency-gen.c
@@ -21,7 +21,7 @@
// RUN: %clang -MD -MF - %s -fsyntax-only -I ./ | FileCheck -check-prefix=CHECK-SIX %s
// CHECK-SIX: {{ }}x.h
// RUN: echo "fun:foo" > %t.blacklist
-// RUN: %clang -MD -MF - %s -fsyntax-only -fsanitize=cfi-vcall -flto -fsanitize-blacklist=%t.blacklist -I ./ | FileCheck -check-prefix=CHECK-SEVEN %s
+// RUN: %clang -MD -MF - %s -fsyntax-only -fsanitize=cfi-vcall -flto -fvisibility=hidden -fsanitize-blacklist=%t.blacklist -I ./ | FileCheck -check-prefix=CHECK-SEVEN %s
// CHECK-SEVEN: .blacklist
// CHECK-SEVEN: {{ }}x.h
#ifndef INCLUDE_FLAG_TEST
Index: test/Driver/whole-program-vtables.c
===
--- test/Driver/whole-program-vtables.c
+++ test/Driver/whole-program-vtables.c
@@ -1,11 +1,2 @@
// RUN: %clang -target x86_64-unknown-linux -fwhole-program-vtables -### %s 2>&1 | FileCheck --check-prefix=NO-LTO %s
// NO-LTO: invalid argument '-fwhole-program-vtables' only allowed with '-flto'
-
-// RUN: %clang -target x86_64-unknown-linux -resource-dir=%S/Inputs/resource_dir -flto -fwhole-program-vtables -### -c %s 2>&1 | FileCheck --check-prefix=BLACKLIST %s
-// BLACKLIST: "-fwhole-program-vtables-blacklist={{.*}}vtables_blacklist.txt"
-
-// RUN: %clang -target x86_64-unknown-linux -fwhole-program-vtables-blacklist=nonexistent.txt -flto -fwhole-program-vtables -### -c %s 2>&1 | FileCheck --check-prefix=NON-EXISTENT-BLACKLIST %s
-// NON-EXISTENT-BLACKLIST: no such file or directory: 'nonexistent.txt'
-
-// RUN: %clang -target x86_64-unknown-linux -fwhole-program-vtables-blacklist=%S/Inputs/resource_dir/vtables_blacklist.txt -flto -fwhole-program-vtables -### -c %s 2>&1 | FileCheck --check-prefix=CUSTOM-BLACKLIST %s
-// CUSTOM-BLACKLIST: "-fwhole-program-vtables-blacklist={{.*}}Inputs/resource_dir/vtables_blacklist.txt"
Index:
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc updated this revision to Diff 52732.
pcc added a comment.
- Update command line flag docs
http://reviews.llvm.org/D18635
Files:
docs/ControlFlowIntegrity.rst
docs/LTOVisibility.rst
docs/UsersManual.rst
docs/index.rst
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Driver/Options.td
include/clang/Frontend/CodeGenOptions.def
include/clang/Frontend/CodeGenOptions.h
include/clang/Sema/AttributeList.h
include/clang/Sema/Sema.h
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGVTables.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/MicrosoftCXXABI.cpp
lib/Driver/SanitizerArgs.cpp
lib/Driver/Tools.cpp
lib/Frontend/CompilerInvocation.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
runtime/CMakeLists.txt
runtime/vtables_blacklist.txt
test/CodeGenCXX/bitset-blacklist.cpp
test/CodeGenCXX/bitset-inference.cpp
test/CodeGenCXX/bitsets.cpp
test/CodeGenCXX/cfi-blacklist.cpp
test/CodeGenCXX/cfi-cast.cpp
test/CodeGenCXX/cfi-nvcall.cpp
test/CodeGenCXX/cfi-stats.cpp
test/Driver/default-class-scope.c
test/Driver/fsanitize.c
test/Driver/whole-program-vtables.c
test/Frontend/default-class-scope.c
test/SemaCXX/attr-linkage-unit-scope.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2527,6 +2527,7 @@
case ObjCProtocol | ObjCInterface:
return "ExpectedObjectiveCInterfaceOrProtocol";
case Field | Var: return "ExpectedFieldOrGlobalVar";
+case GenericRecord | Namespace: return "ExpectedRecordOrNamespace";
}
PrintFatalError(S.getLoc(),
Index: test/SemaCXX/attr-linkage-unit-scope.cpp
===
--- /dev/null
+++ test/SemaCXX/attr-linkage-unit-scope.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -DATTR=linkage_unit_scope %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -DATTR=global_scope %s
+
+int i [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+typedef int t [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+[[clang::ATTR]] void f(); // expected-warning {{attribute only applies to struct, union, class or namespace}}
+void f() [[clang::ATTR]]; // expected-error {{attribute cannot be applied to types}}
+
+struct [[clang::ATTR]] s1 {
+ int i [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+ [[clang::ATTR]] void f(); // expected-warning {{attribute only applies to struct, union, class or namespace}}
+};
+
+struct [[clang::ATTR(1)]] s2 { // expected-error {{attribute takes no arguments}}
+ virtual void f();
+};
+
+struct
+[[clang::linkage_unit_scope]] // expected-error{{'linkage_unit_scope' and 'global_scope' attributes are not compatible}}
+[[clang::global_scope]] // expected-note{{conflicting attribute is here}}
+s3 {};
+
+struct
+[[clang::global_scope]] // expected-error{{'global_scope' and 'linkage_unit_scope' attributes are not compatible}}
+[[clang::linkage_unit_scope]] // expected-note{{conflicting attribute is here}}
+s4 {};
+
+struct [[clang::ATTR]] s5;
+struct s5 {};
+
+struct s6;
+struct [[clang::ATTR]] s6 {};
+
+struct [[clang::ATTR]] s7;
+struct s7;
+struct s7 {};
+
+struct [[clang::ATTR]] s8;
+struct s8;
+struct [[clang::ATTR]] s8 {};
+
+struct s9;
+struct [[clang::ATTR]] s9;
+
+struct [[clang::linkage_unit_scope]] s10; //expected-error{{attributes are not compatible}}
+struct [[clang::global_scope]] s10 {}; // expected-note{{conflicting attribute is here}}
+
+namespace [[clang::ATTR]] ns1 {}
+
+namespace [[clang::linkage_unit_scope]] ns2 {} // expected-error{{attributes are not compatible}}
+namespace [[clang::global_scope]] ns2 {} // expected-note{{conflicting attribute is here}}
Index: test/Frontend/default-class-scope.c
===
--- /dev/null
+++ test/Frontend/default-class-scope.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -fdefault-class-scope=global %s
+// RUN: %clang_cc1 -fdefault-class-scope=attrs %s
+// RUN: %clang_cc1 -fdefault-class-scope=linkage-unit %s
+
+// RUN: not %clang_cc1 -fdefault-class-scope=foo %s 2>&1 | FileCheck %s
+// CHECK: error: invalid value 'foo' in '-fdefault-class-scope=foo'
Index: test/Driver/whole-program-vtables.c
===
--- test/Driver/whole-program-vtables.c
+++ test/Driver/whole-program-vtables.c
@@ -1,11 +1,2 @@
// RUN: %clang -target x86_64-unknown-linux -fwhole-program-vtables -### %s 2>&1 | FileCheck --check-prefix=NO-LTO %s
// NO-LTO: invalid argument '-fwhole-program-vtables' only allowed with '-flto'
-
-// RUN:
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc added a comment. I have updated the documentation for the new design; PTAL before I proceed with implementation. http://reviews.llvm.org/D18635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc updated this revision to Diff 52730.
pcc added a comment.
- Rewrite docs
http://reviews.llvm.org/D18635
Files:
docs/ControlFlowIntegrity.rst
docs/LTOVisibility.rst
docs/UsersManual.rst
docs/index.rst
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Driver/Options.td
include/clang/Frontend/CodeGenOptions.def
include/clang/Frontend/CodeGenOptions.h
include/clang/Sema/AttributeList.h
include/clang/Sema/Sema.h
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGVTables.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/MicrosoftCXXABI.cpp
lib/Driver/SanitizerArgs.cpp
lib/Driver/Tools.cpp
lib/Frontend/CompilerInvocation.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
runtime/CMakeLists.txt
runtime/vtables_blacklist.txt
test/CodeGenCXX/bitset-blacklist.cpp
test/CodeGenCXX/bitset-inference.cpp
test/CodeGenCXX/bitsets.cpp
test/CodeGenCXX/cfi-blacklist.cpp
test/CodeGenCXX/cfi-cast.cpp
test/CodeGenCXX/cfi-nvcall.cpp
test/CodeGenCXX/cfi-stats.cpp
test/Driver/default-class-scope.c
test/Driver/fsanitize.c
test/Driver/whole-program-vtables.c
test/Frontend/default-class-scope.c
test/SemaCXX/attr-linkage-unit-scope.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2527,6 +2527,7 @@
case ObjCProtocol | ObjCInterface:
return "ExpectedObjectiveCInterfaceOrProtocol";
case Field | Var: return "ExpectedFieldOrGlobalVar";
+case GenericRecord | Namespace: return "ExpectedRecordOrNamespace";
}
PrintFatalError(S.getLoc(),
Index: test/SemaCXX/attr-linkage-unit-scope.cpp
===
--- /dev/null
+++ test/SemaCXX/attr-linkage-unit-scope.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -DATTR=linkage_unit_scope %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -DATTR=global_scope %s
+
+int i [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+typedef int t [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+[[clang::ATTR]] void f(); // expected-warning {{attribute only applies to struct, union, class or namespace}}
+void f() [[clang::ATTR]]; // expected-error {{attribute cannot be applied to types}}
+
+struct [[clang::ATTR]] s1 {
+ int i [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+ [[clang::ATTR]] void f(); // expected-warning {{attribute only applies to struct, union, class or namespace}}
+};
+
+struct [[clang::ATTR(1)]] s2 { // expected-error {{attribute takes no arguments}}
+ virtual void f();
+};
+
+struct
+[[clang::linkage_unit_scope]] // expected-error{{'linkage_unit_scope' and 'global_scope' attributes are not compatible}}
+[[clang::global_scope]] // expected-note{{conflicting attribute is here}}
+s3 {};
+
+struct
+[[clang::global_scope]] // expected-error{{'global_scope' and 'linkage_unit_scope' attributes are not compatible}}
+[[clang::linkage_unit_scope]] // expected-note{{conflicting attribute is here}}
+s4 {};
+
+struct [[clang::ATTR]] s5;
+struct s5 {};
+
+struct s6;
+struct [[clang::ATTR]] s6 {};
+
+struct [[clang::ATTR]] s7;
+struct s7;
+struct s7 {};
+
+struct [[clang::ATTR]] s8;
+struct s8;
+struct [[clang::ATTR]] s8 {};
+
+struct s9;
+struct [[clang::ATTR]] s9;
+
+struct [[clang::linkage_unit_scope]] s10; //expected-error{{attributes are not compatible}}
+struct [[clang::global_scope]] s10 {}; // expected-note{{conflicting attribute is here}}
+
+namespace [[clang::ATTR]] ns1 {}
+
+namespace [[clang::linkage_unit_scope]] ns2 {} // expected-error{{attributes are not compatible}}
+namespace [[clang::global_scope]] ns2 {} // expected-note{{conflicting attribute is here}}
Index: test/Frontend/default-class-scope.c
===
--- /dev/null
+++ test/Frontend/default-class-scope.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -fdefault-class-scope=global %s
+// RUN: %clang_cc1 -fdefault-class-scope=attrs %s
+// RUN: %clang_cc1 -fdefault-class-scope=linkage-unit %s
+
+// RUN: not %clang_cc1 -fdefault-class-scope=foo %s 2>&1 | FileCheck %s
+// CHECK: error: invalid value 'foo' in '-fdefault-class-scope=foo'
Index: test/Driver/whole-program-vtables.c
===
--- test/Driver/whole-program-vtables.c
+++ test/Driver/whole-program-vtables.c
@@ -1,11 +1,2 @@
// RUN: %clang -target x86_64-unknown-linux -fwhole-program-vtables -### %s 2>&1 | FileCheck --check-prefix=NO-LTO %s
// NO-LTO: invalid argument '-fwhole-program-vtables' only allowed with '-flto'
-
-// RUN: %clang -target
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc added inline comments.
Comment at: docs/ClassScope.rst:2
@@ +1,3 @@
+===
+Class Scope
+===
rsmith wrote:
> 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`).
1. Deciding to treat `std` specially based on flags like `/MT` makes sense to
me, but visibility isn't a thing on Windows, and dll*port affects the ABI, so
we probably need the in-between state there.
2. Okay, I prefer that over what I proposed. I suppose users could specify
`-fvisibility=default` if that's really what they want.
Let's say we simplify this down to no new driver flags and one new attribute
that disables vtable opt and CFI on hidden visibility and non-dll*port classes
(no namespace attributes). Let me confirm that that's enough for the use cases
I care about.
http://reviews.llvm.org/D18635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc added inline comments.
Comment at: docs/ClassScope.rst:2
@@ +1,3 @@
+===
+Class Scope
+===
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.
http://reviews.llvm.org/D18635
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
rsmith added inline comments. Comment at: docs/ClassScope.rst:2 @@ +1,3 @@ +=== +Class Scope +=== 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? http://reviews.llvm.org/D18635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc added inline comments. Comment at: docs/ControlFlowIntegrity.rst:271 @@ +270,3 @@ +linkage-unit scope. With this flag enabled, the compiler will emit cross-DSO +CFI checks for all classes, except for those which appear in the CFI blacklist +or which use a ``no_sanitize`` attribute. eugenis wrote: > Do we emit a fast non-cross-DSO check for classes with linkage-unit scope? We do not. I reckon we can start doing that in a follow up, this change is large enough as it is. http://reviews.llvm.org/D18635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc added inline comments. Comment at: docs/ClassScope.rst:2 @@ +1,3 @@ +=== +Class Scope +=== 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"? Comment at: docs/ClassScope.rst:13 @@ +12,3 @@ +to a single linkage unit (i.e. the executable or DSO). It is effectively an +ODR violation to declare classes with linkage-unit scope in multiple linkage +units. Classes with global scope may be defined in multiple linkage units, rsmith wrote: > classes -> a class > > Otherwise this reads like you're saying at most one linkage unit per program > can declare classes with linkage-unit scope. Will do Comment at: docs/ClassScope.rst:14 @@ +13,3 @@ +ODR violation to declare classes with linkage-unit scope in multiple linkage +units. Classes with global scope may be defined in multiple linkage units, +but the tradeoff is that the virtual function call optimization and control rsmith wrote: > Classes -> A class Will do Comment at: docs/ClassScope.rst:23 @@ +22,3 @@ + + - ``-fdefault-class-scope=attrs`` indicates that the compiler will infer + class scope based on platform-specific attributes that control the class's eugenis wrote: > Maybe call it "default"? Attrs sounds too specific. Basically this setting > lets clang figure out scope based on the source code. `-fdefault-class-scope=default`? Sounds a little stuttery. Although `attrs` isn't entirely accurate (it's also affected by flags), it's close enough I reckon. Comment at: docs/ClassScope.rst:28 @@ +27,3 @@ + or the ``-fvisibility=hidden -fvisibility-inlines-hidden`` flags) + receive global scope, and all others receive linkage-unit scope. When + targeting Windows, classes with the ``__declspec(dllexport)`` or eugenis wrote: > hidden visibility = linkage-unit scope, not global scope. Yes, I somehow completely screwed up the sense of these, even in the Windows part. Will fix. http://reviews.llvm.org/D18635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
rsmith added inline comments. Comment at: docs/ClassScope.rst:2 @@ +1,3 @@ +=== +Class Scope +=== 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). Comment at: docs/ClassScope.rst:13 @@ +12,3 @@ +to a single linkage unit (i.e. the executable or DSO). It is effectively an +ODR violation to declare classes with linkage-unit scope in multiple linkage +units. Classes with global scope may be defined in multiple linkage units, classes -> a class Otherwise this reads like you're saying at most one linkage unit per program can declare classes with linkage-unit scope. Comment at: docs/ClassScope.rst:14 @@ +13,3 @@ +ODR violation to declare classes with linkage-unit scope in multiple linkage +units. Classes with global scope may be defined in multiple linkage units, +but the tradeoff is that the virtual function call optimization and control Classes -> A class http://reviews.llvm.org/D18635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
eugenis added inline comments. Comment at: docs/ClassScope.rst:23 @@ +22,3 @@ + + - ``-fdefault-class-scope=attrs`` indicates that the compiler will infer + class scope based on platform-specific attributes that control the class's Maybe call it "default"? Attrs sounds too specific. Basically this setting lets clang figure out scope based on the source code. Comment at: docs/ClassScope.rst:28 @@ +27,3 @@ + or the ``-fvisibility=hidden -fvisibility-inlines-hidden`` flags) + receive global scope, and all others receive linkage-unit scope. When + targeting Windows, classes with the ``__declspec(dllexport)`` or hidden visibility = linkage-unit scope, not global scope. Comment at: docs/ControlFlowIntegrity.rst:271 @@ +270,3 @@ +linkage-unit scope. With this flag enabled, the compiler will emit cross-DSO +CFI checks for all classes, except for those which appear in the CFI blacklist +or which use a ``no_sanitize`` attribute. Do we emit a fast non-cross-DSO check for classes with linkage-unit scope? http://reviews.llvm.org/D18635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D18635: Rework interface for bitset-using features to use a notion of class scope.
pcc created this revision.
pcc added reviewers: rsmith, eugenis, aaron.ballman, cfe-commits.
pcc added subscribers: joker.eph, pete.
Bitsets, and the compiler features they rely on (vtable opt, CFI),
only have scope within the linkage unit. Therefore, only enable these
features for classes with linkage unit scope or internal linkage. A new flag,
-fdefault-class-scope=, controls which classes we infer linkage unit scope for.
By default, we infer scope from visibility or dllimport/dllexport attributes.
If the cross-DSO CFI mode is enabled, bitset checks are emitted even for
classes without linkage unit scope, as that mode uses a separate mechanism
to cause bitsets to be exported.
We also provide the [[clang::linkage_unit_scope]] and [[clang::global_scope]]
attributes, which allow users to override the compiler's scope inferences
on a per-class or per-namespace basis.
These attributes replace the whole-program-vtables blacklist, so remove the
-fwhole-program-vtables-blacklist flag.
Because __declspec(uuid()) now implies [[clang::no_linkage_unit_scope]], the
support for the special attr:uuid blacklist entry is removed.
http://reviews.llvm.org/D18635
Files:
docs/ClassScope.rst
docs/ControlFlowIntegrity.rst
docs/UsersManual.rst
docs/index.rst
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Driver/Options.td
include/clang/Frontend/CodeGenOptions.def
include/clang/Frontend/CodeGenOptions.h
include/clang/Sema/AttributeList.h
include/clang/Sema/Sema.h
lib/CodeGen/CGClass.cpp
lib/CodeGen/CGVTables.cpp
lib/CodeGen/CodeGenModule.cpp
lib/CodeGen/CodeGenModule.h
lib/CodeGen/MicrosoftCXXABI.cpp
lib/Driver/SanitizerArgs.cpp
lib/Driver/Tools.cpp
lib/Frontend/CompilerInvocation.cpp
lib/Sema/SemaDecl.cpp
lib/Sema/SemaDeclAttr.cpp
lib/Sema/SemaDeclCXX.cpp
runtime/CMakeLists.txt
runtime/vtables_blacklist.txt
test/CodeGenCXX/bitset-blacklist.cpp
test/CodeGenCXX/bitset-inference.cpp
test/CodeGenCXX/bitsets.cpp
test/CodeGenCXX/cfi-blacklist.cpp
test/CodeGenCXX/cfi-cast.cpp
test/CodeGenCXX/cfi-nvcall.cpp
test/CodeGenCXX/cfi-stats.cpp
test/Driver/default-class-scope.c
test/Driver/fsanitize.c
test/Driver/whole-program-vtables.c
test/Frontend/default-class-scope.c
test/SemaCXX/attr-linkage-unit-scope.cpp
utils/TableGen/ClangAttrEmitter.cpp
Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2527,6 +2527,7 @@
case ObjCProtocol | ObjCInterface:
return "ExpectedObjectiveCInterfaceOrProtocol";
case Field | Var: return "ExpectedFieldOrGlobalVar";
+case GenericRecord | Namespace: return "ExpectedRecordOrNamespace";
}
PrintFatalError(S.getLoc(),
Index: test/SemaCXX/attr-linkage-unit-scope.cpp
===
--- /dev/null
+++ test/SemaCXX/attr-linkage-unit-scope.cpp
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -DATTR=linkage_unit_scope %s
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify -DATTR=global_scope %s
+
+int i [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+typedef int t [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+[[clang::ATTR]] void f(); // expected-warning {{attribute only applies to struct, union, class or namespace}}
+void f() [[clang::ATTR]]; // expected-error {{attribute cannot be applied to types}}
+
+struct [[clang::ATTR]] s1 {
+ int i [[clang::ATTR]]; // expected-warning {{attribute only applies to struct, union, class or namespace}}
+ [[clang::ATTR]] void f(); // expected-warning {{attribute only applies to struct, union, class or namespace}}
+};
+
+struct [[clang::ATTR(1)]] s2 { // expected-error {{attribute takes no arguments}}
+ virtual void f();
+};
+
+struct
+[[clang::linkage_unit_scope]] // expected-error{{'linkage_unit_scope' and 'global_scope' attributes are not compatible}}
+[[clang::global_scope]] // expected-note{{conflicting attribute is here}}
+s3 {};
+
+struct
+[[clang::global_scope]] // expected-error{{'global_scope' and 'linkage_unit_scope' attributes are not compatible}}
+[[clang::linkage_unit_scope]] // expected-note{{conflicting attribute is here}}
+s4 {};
+
+struct [[clang::ATTR]] s5;
+struct s5 {};
+
+struct s6;
+struct [[clang::ATTR]] s6 {};
+
+struct [[clang::ATTR]] s7;
+struct s7;
+struct s7 {};
+
+struct [[clang::ATTR]] s8;
+struct s8;
+struct [[clang::ATTR]] s8 {};
+
+struct s9;
+struct [[clang::ATTR]] s9;
+
+struct [[clang::linkage_unit_scope]] s10; //expected-error{{attributes are not compatible}}
+struct [[clang::global_scope]] s10 {}; // expected-note{{conflicting attribute is here}}
+
+namespace [[clang::ATTR]] ns1 {}
+
+namespace [[clang::linkage_unit_scope]] ns2 {} //
