This is an archive of the discontinued LLVM Phabricator instance.

test case for poisoning trivial members
ClosedPublic

Authored by nmusgrave on Aug 10 2015, 10:28 AM.

Details

Summary

A virtual base class and derived class should only poison their
respective members upon destruction. In particular, trivial members should
be poisoned directly, non-trivial members should be poisoned by their
respective destructors, and references to non-trivial members should be
poisoned.

Diff Detail

Event Timeline

nmusgrave updated this revision to Diff 31686.Aug 10 2015, 10:28 AM
nmusgrave retitled this revision from to test case for poisoning trivial members.
nmusgrave updated this object.
nmusgrave added reviewers: eugenis, kcc.
nmusgrave updated this revision to Diff 31688.Aug 10 2015, 10:38 AM
  • Test case avoids casting to access members
eugenis added inline comments.Aug 10 2015, 10:42 AM
test/msan/dtor-base-access.cc
6

I think this is usually expressed with XFAIL: on a separate line to show that this is not a desired behavior. The test will show up on the "expected failures" line then, and not as a "passing" test.

33

I don't like this cast. Please change B to accept the address of y as a constructor argument.

44

Heap allocation to avoid undefined behavior.

eugenis added inline comments.Aug 10 2015, 10:44 AM
test/msan/dtor-base-access.cc
6

Wait, does this expect a compilation failure?

nmusgrave updated this revision to Diff 31692.Aug 10 2015, 11:13 AM
  • run configurations to reflect expected runtime failure on assertions
eugenis edited edge metadata.Aug 10 2015, 11:20 AM

Don't call Base "a virtual base class" - it's not. It only has a virtual destructor, which is btw not used in this test and can be omitted.

test/msan/dtor-base-access.cc
2

-Xclang -disable-llvm-optzns is used to suppress all LLVM optimization passes when the test needs to examine LLVM IR immediately after the frontend. It should not be used in the end-to-end tests in compiler-rt.

18

That's not a reference, that's a value.

nmusgrave updated this revision to Diff 31715.Aug 10 2015, 1:24 PM
nmusgrave marked 6 inline comments as done.
nmusgrave edited edge metadata.
  • simplified access to internal members
eugenis added inline comments.Aug 10 2015, 2:00 PM
test/msan/dtor-base-access.cc
18

What I meant was store the address of "y" into some data member of Base so that you can later check "y" in ~Base without casting to Derived.
Sorry I was not clear enough.

41

now this line check a member of Base, and not one of its subclass

nmusgrave updated this revision to Diff 31728.Aug 10 2015, 2:41 PM
  • updated internal member structure of base
eugenis added inline comments.Aug 10 2015, 2:46 PM
test/msan/dtor-base-access.cc
53

Not clear what this (and the following assert) does. Maybe replace them with a check for d->x_ptr, which should be good here and poisoned after the destructor?

nmusgrave updated this revision to Diff 31730.Aug 10 2015, 2:49 PM
  • revised assert in main to verify successful poisoning after dtor
eugenis added inline comments.Aug 10 2015, 2:51 PM
test/msan/dtor-base-access.cc
53

Hm, I think you want to check that x_ptr itself is poisoned/unpoisoned.
*x_ptr is already checked in ~Base.

nmusgrave updated this revision to Diff 31732.Aug 10 2015, 3:00 PM
  • verify address of pointer is poisoned
nmusgrave updated this revision to Diff 31733.Aug 10 2015, 3:14 PM
  • fixed assert err
eugenis accepted this revision.Aug 10 2015, 3:15 PM
eugenis edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 10 2015, 3:15 PM
nmusgrave updated this revision to Diff 31736.Aug 10 2015, 3:32 PM
nmusgrave edited edge metadata.
  • cleaned up test
nmusgrave closed this revision.Aug 10 2015, 3:40 PM