Dtor sanitization handled amidst other dtor cleanups,
between cleaning bases and fields. Sanitizer call pushed onto
stack of cleanup operations.
Details
Diff Detail
Event Timeline
- Refactoring dtor sanitizing emission order
- support for virtual functions & virtual bases WIP
- Refactoring dtor sanitizing emission order
- support for virtual functions & virtual bases WIP
- Repress dtor aliasing when sanitizing in dtor
This patch needs tests.
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
50 | We should still emit as alias if the derived class has no new fields. Effectively, you need to check the same conditions as in SanitizeDtor class, to avoid pointlessly suppressing alias generation in cases when we end up not poisoning any memory. Also, this should take the msan function attribute into account. Don't fold it into the same statement as CXXCtorDtorAliases, and add a comment explaining why this is done. | |
lib/CodeGen/CGClass.cpp | ||
1542 | This is no longer true. | |
1546 | extra empty line | |
1591 | Move this line above to group it with related code (Fn assignment). |
The runtime test dtor-derived-class.cpp fails without this patch. I'll write a cfe test as well to verify the virtual function table is properly initialized
A good test in cfe would involve a class with a base class, a virtual base class, and a non-trivial member, and would check the order of calls in the derived class destructor.
- More complex testing for destruction order. Tests class with base, virtual base, trivial, and nontrivial member to ensure destruction order is correct.
The test for virtual base that you added effectively disables the aliasing test, because Derived now has non-trivial members.
Move it to a separate file.
test/CodeGenCXX/sanitize-dtor-repress-aliasing.cpp | ||
---|---|---|
39 | I don't think this is interesting. | |
48 | That's a lot of checks. And the same in ZN7DerivedD2Ev. |
- Poisoning on field-by-field basis, with collective poisoning of trivial members when possible.
- Cleaned up implementation of calculating region to poison in dtor.
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
49 | I'm not sure this is correct. It says not to use an alias if D has trivial body. Should not it be the other way around? |
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
49 | It's counter-intuitive: |
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
49 | For example: line 3711 of clang/lib/CodeGen/MicrosoftCXXABI.cpp GlobalDecl(dtor, Dtor_Complete), GlobalDecl(dtor, Dtor_Base), true); ^uses the negation of the function's return value |
lib/CodeGen/CGClass.cpp | ||
---|---|---|
1579 | Why do you need to special-case pointers? |
lib/CodeGen/CGClass.cpp | ||
---|---|---|
1579 | FieldHasTrivialDestructorBody doesn't catch pointers- it identifies their base type as some class, and returns false |
- Refactoring dtor sanitizing emission order
- support for virtual functions & virtual bases WIP
- Repress dtor aliasing when sanitizing in dtor
- CFE test for dtor aliasing, and repression of aliasing in dtor code generation.
- More complex testing for destruction order.
- Poison trivial members one-by-one.
- Poisoning on field-by-field basis, with collective poisoning of trivial members when possible.
- Cleaned up implementation of calculating region to poison in dtor.
- Checking for existence of a single trivial field.
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
45 | This is unused. | |
48 | This is unnecessary: | |
133 | This function is called from TryEmitBaseDestructorAsAlias. | |
lib/CodeGen/CGClass.cpp | ||
1355 | Remove context from arguments, use CGF.getContext() | |
1488 | extra line | |
1499 | unrelated changes | |
1722 |
|
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
31 | I think this should be called "HasFieldWithTrivialDestructor". |
test/CodeGenCXX/sanitize-dtor-repress-aliasing.cpp | ||
---|---|---|
31 | Its checking that my aliasing is repressed when appropriate.Should I keep or get rid of this test case? |
test/CodeGenCXX/sanitize-dtor-repress-aliasing.cpp | ||
---|---|---|
31 | Hm, it looks like my comment was for some older version of this code. |
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
42–44 | 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? | |
45–47 | This check seems to be redundant, since TryEmitDefinitionAsAlias checks the same thing. Remove this copy? | |
135 | 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. | |
136–138 | Please reorder this after the quicker, higher-level checks below. (Maybe move it to just before we create the mangled name?) | |
lib/CodeGen/CGClass.cpp | ||
1334–1335 | This appears redundant; we'd return true for pointers on line 1341 anyway. (getBaseElementType only strips off array types, not pointer types). | |
1549 | Add full stop. | |
1554 | You're only counting the direct fields here, not the inherited ones. | |
1555–1558 | unsigned totalFields = std::distance(Dtor->getParent()->field_begin(), Dtor->getParent()->field_end()); | |
1567 | todo -> TODO (or FIXME) | |
1621 | 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. | |
1644–1645 | Maybe do this once in Emit (if you poison anything) rather than doing it every time you emit a poison call? | |
1722 | Please add a comment here. "Mark the lifetime of fields as ending after field destructors run and before we destroy base classes." or similar. |
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
42–44 | Yes, each (sub-)object poisons its storage in the base destructor, and only does it for fields that don't have their own destructors to avoid double-poisoning. Poisoning vptr would be interesting. Yes, current implementation fails to do this. Padding - less so, but it would make the tool a little bit more predictable. |
- Simplified implementation of class field poisoning, to avoid redundant counting and conditional checks. Expensive checks delayed.
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
42–44 | vptr poisoning will be implemented in another CL after this is approved |
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
45 | This simply suppresses all dtor alias under UseAfterDtor, effectively disabling the second check below. | |
131–140 | formatting | |
157 | ... with trivial destructors | |
lib/CodeGen/CGClass.cpp | ||
1550 | Probably better check Dtor->getParent()->field_empty() for consistency. | |
1554 | disable tail calls only when !field_empty() |
LGTM but please wait for Richard's answer
lib/CodeGen/CGClass.cpp | ||
---|---|---|
1554 | oh, right, that's already the case. |
I'd like to see some tests for poisoning bitfields, and particularly anonymous bitfields (I think the code works fine in those cases, but they're a bit special so explicitly testing them is useful).
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
161 | && goes on previous line. | |
lib/CodeGen/CGClass.cpp | ||
1550 | This check is equivalent and more efficient. | |
1562 | Can you use a range for? | |
1590 | Doxygenize this comment (use ///, \param). |
lib/CodeGen/CGCXX.cpp | ||
---|---|---|
156–163 | 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. | |
lib/CodeGen/CGClass.cpp | ||
1562–1563 | Separately declaring F and Field seems unnecessary; how about: for (const FieldDecl *Field : Dtor->getParent()->fields()) { | |
1595–1596 | Initialize these as you declare them, rather than separately: llvm::ConstantInt *OffsetSizePtr = llvm::ConstantInt::get( // ... llvm::Value *OffsetPtr = // ... | |
test/CodeGenCXX/sanitize-dtor-bit-field.cpp | ||
1 ↗ | (On Diff #33959) | 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; }; |
I think this should be called "HasFieldWithTrivialDestructor".
Also, pass D->getParent() to this function instead of D.