[PATCH] D12022: Refactored dtor sanitizing into EHScopeStack

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/CodeGen/CGCXX.cpp:140-147
@@ -131,1 +139,10 @@
 
+  // If sanitizing memory to check for use-after-dtor, do not emit as
+  //  an alias, unless this class owns no members with trivial destructors.
+  const CXXMethodDecl *MD =
+      cast<CXXMethodDecl>(AliasDecl.getDecl())->getCanonicalDecl();
+  if (isa<CXXDestructorDecl>(MD) &&
+      getCodeGenOpts().SanitizeMemoryUseAfterDtor &&
+      HasFieldWithTrivialDestructor(*this, MD->getParent()))
+    return true;
+
----------------
Having checked a bit more, I think this test should go in `TryEmitBaseDestructorAsAlias`: it's OK to emit the complete object destructor for a class as an alias for the base subobject constructor for the same class even if this sanitizer is enabled, but it's not OK to emit the destructor for the class as an alias for a base class's destructor (which is what `TryEmitBaseDestructorAsAlias` is checking).

The check then becomes a lot simpler: with the sanitizer enabled, for each field we will do exactly one of (a) invoke a destructor (which will poison the memory) or (b) poison the memory ourselves. So if the sanitizer is enabled, we can emit an alias only if the derived class has no fields.

================
Comment at: lib/CodeGen/CGClass.cpp:1559-1560
@@ +1558,4 @@
+      // RecordDecl::field_iterator Field;
+      for (const auto F : Dtor->getParent()->fields()) {
+        FieldDecl const *Field = F;
+        // Poison field if it is trivial
----------------
Separately declaring `F` and `Field` seems unnecessary; how about:

  for (const FieldDecl *Field : Dtor->getParent()->fields()) {

================
Comment at: lib/CodeGen/CGClass.cpp:1592-1593
@@ +1591,4 @@
+          Context.getASTRecordLayout(Dtor->getParent());
+      llvm::ConstantInt *OffsetSizePtr;
+      llvm::Value *OffsetPtr;
+      CharUnits::QuantityType PoisonSize;
----------------
Initialize these as you declare them, rather than separately:

  llvm::ConstantInt *OffsetSizePtr = llvm::ConstantInt::get(
    // ...
  llvm::Value *OffsetPtr = // ...

================
Comment at: test/CodeGenCXX/sanitize-dtor-bit-field.cpp:1
@@ +1,2 @@
+// Test -fsanitize-memory-use-after-dtor
+// RUN: %clang_cc1 -O0 -fsanitize=memory -fsanitize-memory-use-after-dtor -disable-llvm-optzns -std=c++11 -triple=x86_64-pc-linux -emit-llvm -o - %s | FileCheck %s
----------------
It'd also be good to test for the case where a bit-field is adjacent to non-sanitized fields:

  struct A { char c; ~A(); };
  struct B {
    A x;
    int n : 1;
    A y;
  };


http://reviews.llvm.org/D12022