[PATCH] D51905: Front-end of the implementation of the interleaving algorithm

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a subscriber: cfe-commits.
pcc added a reviewer: rsmith.
pcc added inline comments.


================
Comment at: clang/lib/CodeGen/CGCXXABI.h:53
+  // interleaved layout is decided.
+  bool EnableVTableInterleaving;
+
----------------
Why does this need to be stored separately on the CGCXXABI? Can't we always access it via the CodeGenOptions on CGM?


================
Comment at: clang/lib/CodeGen/CGClass.cpp:2769
 llvm::Value *CodeGenFunction::EmitVTableTypeCheckedLoad(
     const CXXRecordDecl *RD, llvm::Value *VTable, uint64_t VTableByteOffset) {
   SanitizerScope SanScope(this);
----------------
Do you need this function as well as the one you're adding? Can't you change the code in MicrosoftCXXABI.cpp to form a constant?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5190
 bool CodeGenModule::NeedAllVtablesTypeId() const {
-  // Returns true if at least one of vtable-based CFI checkers is enabled and
-  // is not in the trapping mode.
+  // Returns true if vtable interleaving is disabled and at least one of
+  // vtable-based CFI checkers is enabled and is not in the trapping mode.
----------------
Comment should say why.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:983
+    // Add offset.type metadata to the newly created offset global variable.
+    OffsetGV->addOffsetTypeMetadata(TypeId, Offset);
+
----------------
I wouldn't add a function to GlobalObject for this, you can add the metadata here directly since this is the only place in the code where you need to add it.


================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1767
 
+  bool InBounds = shouldInterleaveVTables(VTableClass) ? false : true;
   return llvm::ConstantExpr::getGetElementPtr(VTable->getValueType(), VTable,
----------------
Remind me why this needs to be here? (And the explanation needs to be in a comment.)


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1150
 
+  Opts.EnableVTableInterleaving = Args.hasFlag(
+      OPT_enable_vtable_interleaving, OPT_disable_vtable_interleaving, false);
----------------
Use hasArg here instead, there's no need to support a flag for disabling interleaving in the frontend.


https://reviews.llvm.org/D51905