pcc added a comment.

Please upload patches with context. `arc diff` will do this for you.



================
Comment at: clang/docs/ControlFlowIntegrityDesign.rst:277
 
+Forward-Edge CFI for Virtual Calls by Interleaving Virtual Tables
+=================================================================
----------------
I would add this as a subsection of "Forward-Edge CFI for Virtual Calls".


================
Comment at: clang/docs/ControlFlowIntegrityDesign.rst:286
+
+On the high level, the interleaving scheme consists of two steps: 1) order 
virtual tables by a pre-order 
+traversal of the class hierarchy and 2) interleave the virtual tables entry by 
entry.
----------------
On the high level -> At a high level


================
Comment at: clang/docs/ControlFlowIntegrityDesign.rst:322
+This step will arrange the virtual tables for A, B, C, and D in the order of 
*vtable-of-A, vtable-of-B, vtable-of-D, vtable-of-C*.
+In this order, for any class all the compatible virtual tables will appear 
consecutively.
+
----------------
I would move this sentence to the start of the subsection because it isn't 
specific to your example and clarify that although GlobalLayoutBuilder tries to 
place compatible vtables consecutively (but doesn't always succeed because the 
Itanium ABI glues vtables together), this algorithm requires them to appear 
consecutively.


================
Comment at: clang/docs/ControlFlowIntegrityDesign.rst:331
+works under this scheme because the interleaved virtual table has the property 
that for 
+each virtual funtion the distance between an entry of this function and the 
corresponding 
+address point is always the same. 
----------------
funtion -> function


================
Comment at: clang/docs/ControlFlowIntegrityDesign.rst:339
+  
+  A::offset-to-top, B::offset-to-top, D::offset-to-top, C::offset-to-top, 
&A::rtti, &B::rtti, &D::rtti, &C::rtti, &A::f1, &B::f1, &D::f1, &C::f1, &B::f2, 
&D::f2, &C::f3, &D::f4
+
----------------
This layout isn't necessarily going to work with traditional RTTI because the 
`__dynamic_cast` function is allowed to assume that the rtti and offset-to-top 
fields appear at the offsets behind the address point that the ABI says that 
they will appear at. Indeed, the libcxxabi implementation makes that assumption:

https://github.com/llvm-mirror/libcxxabi/blob/master/src/private_typeinfo.cpp#L627

It's probably more something to keep in mind for the implementation, but I 
think we at least need to mention the RTTI incompatibility here.


Repository:
  rC Clang

https://reviews.llvm.org/D50372



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to