[PATCH] D15367: Cross-DSO control flow integrity (Clang part)

[Peter Collingbourne via cfe-commits]

pcc accepted this revision.
pcc added a comment.
This revision is now accepted and ready to land.

LGTM modulo some wordsmithing in the documentation.


================
Comment at: docs/ControlFlowIntegrity.rst:31
@@ +30,3 @@
+enabled, and are statically linked into the program. This may preclude
+the use of shared libraries in some cases. An experimental support for
+:ref:`cross-DSO control flow integrity <cfi-cross-dso>` existst that
----------------
"Experimental support for". Please also mention that the ABI is unstable.

================
Comment at: docs/ControlFlowIntegrity.rst:32
@@ +31,3 @@
+the use of shared libraries in some cases. An experimental support for
+:ref:`cross-DSO control flow integrity <cfi-cross-dso>` existst that
+does not have these requirements.
----------------
"exists"

================
Comment at: docs/ControlFlowIntegrityDesign.rst:375
@@ +374,3 @@
+
+Basic CFI mode described above assumes that the application is a
+monolithic binary; at least that all possible virtual/indirect call
----------------
"The basic CFI mode"

================
Comment at: docs/ControlFlowIntegrityDesign.rst:377
@@ +376,3 @@
+monolithic binary; at least that all possible virtual/indirect call
+targets and the entire class hierarchies are known at the link
+time. The cross-DSO mode, enabled with
----------------
"hierarchy"; "at link time"

================
Comment at: docs/ControlFlowIntegrityDesign.rst:389
@@ +388,3 @@
+  -  Calls between different instrumented DSOs are also protected with
+     performance penalty compared to the monolithic CFI.
+  -  Calls from instrumented DSO to an uninstrumented one are unchecked
----------------
"comparable"

================
Comment at: docs/ControlFlowIntegrityDesign.rst:390
@@ +389,3 @@
+     performance penalty compared to the monolithic CFI.
+  -  Calls from instrumented DSO to an uninstrumented one are unchecked
+     and just work, with performance penalty.
----------------
"from an instrumented DSO"

================
Comment at: docs/ControlFlowIntegrityDesign.rst:392
@@ +391,3 @@
+     and just work, with performance penalty.
+  -  Calls from instrumented DSO outside of any known DSO are detected
+     as CFI violations.
----------------
same

================
Comment at: docs/ControlFlowIntegrityDesign.rst:426
@@ +425,3 @@
+
+Possible, but unlikely, collisions in the ``CallSiteTypeId`` hashing
+will result in weaker CFI checks that would still be conservatively
----------------
"It is possible, but unlikely, that collisions"

================
Comment at: docs/ControlFlowIntegrityDesign.rst:446
@@ +445,3 @@
+
+The basic implementation is be a large switch statement over all
+values of CallSiteTypeId supported by this DSO, and each case is
----------------
"is a"

================
Comment at: docs/ControlFlowIntegrityDesign.rst:450
@@ +449,3 @@
+
+For performance reasons (see the following section), the address of
+__cfi_check is aligned by 4096.
----------------
I wouldn't claim this is "for performance reasons" unless we have data to back it up. I would remove this paragraph and mention the alignment in the next section.

================
Comment at: include/clang/Driver/Options.td:628
@@ -623,1 +627,3 @@
+                                 Group<f_clang_Group>, Flags<[CC1Option]>,
+                                 HelpText<"Enable control flow integrity (CFI) checks for cross-DSO calls.">;
 def funsafe_math_optimizations : Flag<["-"], "funsafe-math-optimizations">,
----------------
"Disable"


Repository:
  rL LLVM

http://reviews.llvm.org/D15367