[PATCH] D12022: Refactored dtor sanitizing into EHScopeStack

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/CodeGen/CGCXX.cpp:42-44
@@ -33,1 +41,5 @@
 bool CodeGenModule::TryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
+  // If sanitizing memory to check for use-after-dtor, do not emit as
+  // an alias, unless it has no fields or has only fields with non-trivial
+  // destructors.
+  if (getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
----------------
I assume the rationale here is that the field and base class destructors will mark their storage as destroyed, so there's nothing else to mark? This may mean that vptrs and padding bytes don't get marked as destroyed, is that OK?

================
Comment at: lib/CodeGen/CGCXX.cpp:45-47
@@ +44,5 @@
+  // destructors.
+  if (getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+      HasFieldWithTrivialDestructor(*this, D->getParent()))
+    return true;
+
----------------
This check seems to be redundant, since `TryEmitDefinitionAsAlias` checks the same thing. Remove this copy?

================
Comment at: lib/CodeGen/CGCXX.cpp:135
@@ +134,3 @@
+      cast<CXXMethodDecl>(AliasDecl.getDecl())->getCanonicalDecl();
+  assert(isa<CXXDestructorDecl>(MD));
+  if (getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
----------------
This function isn't (or wasn't) specific to destructors; I think you should be checking for this as part of your `if` below rather than asserting it.

================
Comment at: lib/CodeGen/CGCXX.cpp:136-138
@@ +135,5 @@
+  assert(isa<CXXDestructorDecl>(MD));
+  if (getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+      HasFieldWithTrivialDestructor(*this, MD->getParent()))
+    return true;
+
----------------
Please reorder this after the quicker, higher-level checks below. (Maybe move it to just before we create the mangled name?)

================
Comment at: lib/CodeGen/CGClass.cpp:1334-1335
@@ -1338,1 +1333,4 @@
 {
+  if (Field->getType()->isPointerType())
+    return true;
+
----------------
This appears redundant; we'd return `true` for pointers on line 1341 anyway. (`getBaseElementType` only strips off array types, not pointer types).

================
Comment at: lib/CodeGen/CGClass.cpp:1549
@@ +1548,3 @@
+
+      // Nothing to poison
+      if (Layout.getFieldCount() == 0)
----------------
Add full stop.

================
Comment at: lib/CodeGen/CGClass.cpp:1554
@@ +1553,3 @@
+      // Iterate over fields declared in this class, and count all fields,
+      // including inherited from base classes.
+      unsigned totalFields = 0;
----------------
You're only counting the direct fields here, not the inherited ones.

================
Comment at: lib/CodeGen/CGClass.cpp:1555-1558
@@ +1554,6 @@
+      // including inherited from base classes.
+      unsigned totalFields = 0;
+      for (auto *Field : Dtor->getParent()->fields()) {
+        (void)Field;
+        totalFields += 1;
+      }
----------------
    unsigned totalFields = std::distance(Dtor->getParent()->field_begin(),
                                         Dtor->getParent()->field_end());

================
Comment at: lib/CodeGen/CGClass.cpp:1567
@@ +1566,3 @@
+      unsigned inheritedFields = totalFields - Layout.getFieldCount();
+      // todo: don't use iterator for accessing fields
+      RecordDecl::field_iterator Field;
----------------
todo -> TODO (or FIXME)

================
Comment at: lib/CodeGen/CGClass.cpp:1621
@@ +1620,3 @@
+
+      if (layoutEndOffset >= Layout.getFieldCount() - 1) {
+        PoisonSize = Layout.getNonVirtualSize().getQuantity() -
----------------
I think this is off by one. `layoutEndOffset` is the index of the first field that is not poisoned, so this will trigger if we're poisoning up to, but not including, the last field, as well as triggering if we're poisoning the last field.

================
Comment at: lib/CodeGen/CGClass.cpp:1644-1645
@@ +1643,4 @@
+          CGF.CGM.CreateRuntimeFunction(FnType, "__sanitizer_dtor_callback");
+      // Prevent the current stack frame from disappearing from the stack trace.
+      CGF.CurFn->addFnAttr("disable-tail-calls", "true");
+
----------------
Maybe do this once in `Emit` (if you poison anything) rather than doing it every time you emit a poison call?

================
Comment at: lib/CodeGen/CGClass.cpp:1722
@@ -1660,1 +1721,3 @@
 
+  if (CGM.getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+      SanOpts.has(SanitizerKind::Memory))
----------------
Please add a comment here. "Mark the lifetime of fields as ending after field destructors run and before we destroy base classes." or similar.


http://reviews.llvm.org/D12022