This is an archive of the discontinued LLVM Phabricator instance.

Refactored dtor sanitizing into EHScopeStack
ClosedPublic

Authored by nmusgrave on Aug 13 2015, 5:25 PM.

Details

Summary

Dtor sanitization handled amidst other dtor cleanups,
between cleaning bases and fields. Sanitizer call pushed onto
stack of cleanup operations.

Diff Detail

Event Timeline

nmusgrave updated this revision to Diff 32115.Aug 13 2015, 5:25 PM
nmusgrave retitled this revision from to Refactored dtor sanitizing into EHScopeStack.
nmusgrave updated this object.
nmusgrave added reviewers: eugenis, kcc.
nmusgrave updated this revision to Diff 32180.Aug 14 2015, 1:52 PM
  • Refactoring dtor sanitizing emission order
  • support for virtual functions & virtual bases WIP
nmusgrave updated this revision to Diff 32360.Aug 17 2015, 5:14 PM
  • Refactoring dtor sanitizing emission order
  • support for virtual functions & virtual bases WIP
  • Repress dtor aliasing when sanitizing in dtor
eugenis edited edge metadata.Aug 17 2015, 5:31 PM

This patch needs tests.

lib/CodeGen/CGCXX.cpp
35

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
1544

This is no longer true.

1548

extra empty line

1593

Move this line above to group it with related code (Fn assignment).

nmusgrave marked 3 inline comments as done.Aug 19 2015, 11:50 AM

This patch needs tests.

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

This patch needs tests.

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.

eugenis edited edge metadata.Aug 19 2015, 12:14 PM
eugenis added a subscriber: cfe-commits.

+cfe-commits

nmusgrave updated this revision to Diff 32600.Aug 19 2015, 1:03 PM
  • CFE test for dtor aliasing, and repression of aliasing in dtor code generation.
nmusgrave updated this revision to Diff 32608.Aug 19 2015, 1:50 PM
  • 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.
How about:
CHECK-LABEL: define {{.*}}ZN7DerivedD1Ev
CHECK-NOT: ret void
CHECK: call void {{.*}}ZN7DerivedD2Ev
CHECK-NOT: ret void
CHECK: call void {{.*}}ZN11VirtualBaseD2Ev
CHECK: ret void

And the same in ZN7DerivedD2Ev.
And remove all other destructors.

nmusgrave updated this revision to Diff 32873.Aug 21 2015, 3:30 PM
nmusgrave marked 2 inline comments as done.
  • Poisoning on field-by-field basis, with collective poisoning of trivial members when possible.
  • Cleaned up implementation of calculating region to poison in dtor.
eugenis added inline comments.Aug 21 2015, 3:54 PM
lib/CodeGen/CGCXX.cpp
41

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?

nmusgrave added inline comments.Aug 21 2015, 4:27 PM
lib/CodeGen/CGCXX.cpp
41

It's counter-intuitive:
TryEmitBaseDestructorAsAlias returns false when TryEmitDefinitionAsAlias returns false.
TryEmitDefinitionAsAlias returns false when an alias is successfully created (line 216 in this CL)

nmusgrave added inline comments.Aug 21 2015, 4:38 PM
lib/CodeGen/CGCXX.cpp
41

For example: line 3711 of clang/lib/CodeGen/MicrosoftCXXABI.cpp
bool ProducedAlias = !CGM.TryEmitDefinitionAsAlias(

GlobalDecl(dtor, Dtor_Complete), GlobalDecl(dtor, Dtor_Base), true);

^uses the negation of the function's return value

eugenis added inline comments.Aug 21 2015, 5:21 PM
lib/CodeGen/CGClass.cpp
1581

Why do you need to special-case pointers?

nmusgrave added inline comments.Aug 21 2015, 5:23 PM
lib/CodeGen/CGClass.cpp
1581

FieldHasTrivialDestructorBody doesn't catch pointers- it identifies their base type as some class, and returns false

nmusgrave updated this revision to Diff 32930.Aug 23 2015, 5:27 PM
  • 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.
nmusgrave updated this revision to Diff 32986.Aug 24 2015, 11:49 AM
  • Refactored for simpler trivial field checking
eugenis added inline comments.Aug 24 2015, 5:55 PM
lib/CodeGen/CGCXX.cpp
37

This is unused.

40

This is unnecessary:
Layout.getFieldCount() > 0

124

This function is called from TryEmitBaseDestructorAsAlias.
This check is stronger than the one in the calling function, so that one has no effect.
Maybe move the check to this function?

lib/CodeGen/CGClass.cpp
1357

Remove context from arguments, use CGF.getContext()
Consider making this a method of CodegenFunction

1490

extra line

1501

unrelated changes

1708
  1. Only do this if sanitizing.
  2. I think this needs to be NormalAndEHCleanup
nmusgrave updated this revision to Diff 33037.Aug 24 2015, 6:49 PM
nmusgrave marked 4 inline comments as done.
  • Simplify function invocations
nmusgrave updated this revision to Diff 33100.Aug 25 2015, 12:09 PM
nmusgrave marked an inline comment as done.
  • Check flags before dtor sanitizing
eugenis added inline comments.Aug 25 2015, 1:30 PM
lib/CodeGen/CGCXX.cpp
31

I think this should be called "HasFieldWithTrivialDestructor".
Also, pass D->getParent() to this function instead of D.

nmusgrave updated this revision to Diff 33115.Aug 25 2015, 1:45 PM
  • Simplify parameters, rename function, for examining fields of class to destroy.

Richard, would you mind taking a look a this change?

nmusgrave updated this revision to Diff 33436.Aug 28 2015, 9:49 AM
  • Checking for existence of fields to poison in alias emission.
nmusgrave marked 3 inline comments as done.Aug 28 2015, 9:51 AM
nmusgrave added inline comments.Aug 28 2015, 9:52 AM
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?

eugenis added inline comments.Aug 28 2015, 1:50 PM
test/CodeGenCXX/sanitize-dtor-repress-aliasing.cpp
31

Hm, it looks like my comment was for some older version of this code.
Keep it.
You can make it a bit more resistant against unrelated changes. We only need to check that both D1 and D2 destructors appear in the vtable, and don't care about any of the i8* casts. Smth like this:
@_ZTV7Derived = {{.*}}@_ZN7DerivedD1Ev{{.*}}@_ZN7DerivedD0Ev

nmusgrave updated this revision to Diff 33463.Aug 28 2015, 2:05 PM
nmusgrave marked an inline comment as done.
  • Alias-repressing test case ignores casting of pointers.
rsmith added inline comments.Sep 1 2015, 3:03 PM
lib/CodeGen/CGCXX.cpp
34–36

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?

37–39

This check seems to be redundant, since TryEmitDefinitionAsAlias checks the same thing. Remove this copy?

126

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.

127–129

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
1339–1340

This appears redundant; we'd return true for pointers on line 1341 anyway. (getBaseElementType only strips off array types, not pointer types).

1551

Add full stop.

1556

You're only counting the direct fields here, not the inherited ones.

1557–1560
unsigned totalFields = std::distance(Dtor->getParent()->field_begin(),
                                     Dtor->getParent()->field_end());
1569

todo -> TODO (or FIXME)

1623

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.

1646–1647

Maybe do this once in Emit (if you poison anything) rather than doing it every time you emit a poison call?

1708

Please add a comment here. "Mark the lifetime of fields as ending after field destructors run and before we destroy base classes." or similar.

eugenis added inline comments.Sep 1 2015, 3:12 PM
lib/CodeGen/CGCXX.cpp
34–36

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.

nmusgrave updated this revision to Diff 33752.Sep 1 2015, 4:31 PM
nmusgrave marked 13 inline comments as done.
  • Simplified implementation of class field poisoning, to avoid redundant counting and conditional checks. Expensive checks delayed.
nmusgrave added inline comments.Sep 1 2015, 4:49 PM
lib/CodeGen/CGCXX.cpp
34–36

vptr poisoning will be implemented in another CL after this is approved

eugenis added inline comments.Sep 2 2015, 4:59 PM
lib/CodeGen/CGCXX.cpp
37

This simply suppresses all dtor alias under UseAfterDtor, effectively disabling the second check below.
Remove it.

122

formatting

139

... with trivial destructors

lib/CodeGen/CGClass.cpp
1552

Probably better check Dtor->getParent()->field_empty() for consistency.

1556

disable tail calls only when !field_empty()

nmusgrave updated this revision to Diff 33887.Sep 2 2015, 5:07 PM
nmusgrave marked 4 inline comments as done.
  • Update comments, consistent style for attribute checking.

LGTM but please wait for Richard's answer

lib/CodeGen/CGClass.cpp
1556

oh, right, that's already the case.

rsmith edited edge metadata.Sep 2 2015, 5:23 PM

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
143

&& goes on previous line.

lib/CodeGen/CGClass.cpp
1552

This check is equivalent and more efficient.

1564

Can you use a range for?

1592

Doxygenize this comment (use ///, \param).

nmusgrave updated this revision to Diff 33956.Sep 3 2015, 10:33 AM
nmusgrave marked 4 inline comments as done.
nmusgrave edited edge metadata.
  • Testing sanitizing bit fields.
nmusgrave updated this revision to Diff 33959.Sep 3 2015, 11:18 AM
  • Refined testing for bit fields.
rsmith added inline comments.Sep 3 2015, 2:19 PM
lib/CodeGen/CGCXX.cpp
138–145

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
1564–1565

Separately declaring F and Field seems unnecessary; how about:

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

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
2

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;
};
nmusgrave updated this revision to Diff 33978.Sep 3 2015, 2:53 PM
nmusgrave marked 4 inline comments as done.
  • Simplified fields and checks for aliasing.
rsmith accepted this revision.Sep 3 2015, 3:12 PM
rsmith edited edge metadata.

LGTM with a couple of tweaks.

lib/CodeGen/CGCXX.cpp
44–45

You can just use !D->getParent()->field_empty()

lib/CodeGen/CGClass.cpp
1293

I think you no longer need to make these functions members.

This revision is now accepted and ready to land.Sep 3 2015, 3:12 PM
nmusgrave updated this revision to Diff 33982.Sep 3 2015, 3:33 PM
nmusgrave marked 2 inline comments as done.
nmusgrave edited edge metadata.
  • Clean method headers, style.
nmusgrave closed this revision.Sep 3 2015, 4:08 PM
Via Conduit