Page MenuHomePhabricator

Implement CWG496: Is a volatile-qualified type really a POD?
AbandonedPublic

Authored by K-ballo on Jan 19 2015, 1:04 PM.

Details

Reviewers
rsmith
Summary

Implement CWG496, volatile qualified types are not trivially copyable, special copy/move member functions are not trivial for classes with volatile qualified non-static data members. Fixes PR21070. Breaks CodeGenCXX/no-opt-volatile-memcpy.cpp, which specifically tests assignment for a struct with a volatile member resulting in memcpy.

Diff Detail

Event Timeline

K-ballo updated this revision to Diff 18399.Jan 19 2015, 1:04 PM
K-ballo retitled this revision from to Implement CWG496: Is a volatile-qualified type really a POD?.
K-ballo updated this object.
K-ballo edited the test plan for this revision. (Show Details)
K-ballo added a reviewer: rsmith.
K-ballo added a subscriber: Unknown Object (MLST).
rsmith accepted this revision.Jan 20 2015, 3:34 PM
rsmith edited edge metadata.

Looks great, thanks!

test/CXX/drs/dr4xx.cpp
1187

dr496: 3.7, please.

test/SemaCXX/type-traits.cpp
1863–1865

Please commit this test update and the bugfix for the array case of QualType::isTriviallyCopyableType separately from the rest of the patch.

This revision is now accepted and ready to land.Jan 20 2015, 3:34 PM

Any suggestions on what to do with the failing testcase (CodeGenCXX/no-opt-volatile-memcpy.cpp)? This test checks that the issued @llvm.memcpy has isvolatile=true, but after this patch memcpy is no longer issued as those operations are no longer considered trivial. This could suggest that memcpy with isvolatile=true should still be used in those cases (albeit the docs say "it is unwise to depend on it"), or that there is dead code out there looking for volatile members in trivial operations.

test/SemaCXX/type-traits.cpp
1863–1865

I don't have commit rights, but I will take this out and prepare a new revision.

Some possible approaches:

  • update the tests to check that we don't memcpy in this case
  • change FieldMemcpyizer (in CGClass) to allow volatile fields and create a volatile memcpy call

The test was added for PR9027, which is just trying to make sure we emit a volatile load/store in this case, and either approach satisfies its requirements, but checking for the absence of a memcpy seems like a somewhat better approach to me.

K-ballo updated this revision to Diff 18683.Jan 23 2015, 11:16 AM
K-ballo edited edge metadata.

Addressed review comments. Added test for the newly introduced diagnostic.

CodeGenCXX/no-opt-volatile-memcpy.cpp still fails. I tried checking for the absence of @llvm.memcpy, but the non-volatile array member of the struct is still memcpy-ed (good!). I have little to no experience with codegen tests, let alone llvm ir, I could use some help to adjust the offending test case.

K-ballo abandoned this revision.Jan 14 2018, 1:45 PM