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