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