This is an archive of the discontinued LLVM Phabricator instance.

Use unique_ptr for cached tokens for default arguments in C++.
ClosedPublic

Authored by dtarditi on Nov 8 2016, 5:19 PM.

Details

Summary

This changes pointers to cached tokens for default arguments in C++ from raw pointers to unique_ptrs. There was a fixme in the code where the cached tokens are created about using a smart pointer.

The change is straightforward, though I did have to track down and fix a memory corruption caused by the change. memcpy was being used to copy parameter information. This duplicated the unique_ptr, which led to the cached token buffer being deleted prematurely.

Diff Detail

Repository
rL LLVM

Event Timeline

dtarditi updated this revision to Diff 77289.Nov 8 2016, 5:19 PM
dtarditi retitled this revision from to Use unique_ptr for cached tokens for default arguments in C++..
dtarditi updated this object.
dtarditi added a subscriber: cfe-commits.
malcolm.parsons added inline comments.
include/clang/Sema/DeclSpec.h
1314 ↗(On Diff #77289)

Did you mean reset()?

lib/Parse/ParseDecl.cpp
6064 ↗(On Diff #77289)

Did you mean reset()?

arphaman added inline comments.
include/clang/Sema/DeclSpec.h
1313 ↗(On Diff #77289)

You can elide the curly braces here now that the loop has just one statement.

dtarditi updated this revision to Diff 77373.EditedNov 9 2016, 10:20 AM

Thanks for the code review feedback - I've addressed it. Yes, we should use reset() instead of release(). I also deleted the unnecessary braces. I don't have commit access, so if this looks good, could someone commit this on my behalf?

malcolm.parsons accepted this revision.Nov 11 2016, 10:59 AM
malcolm.parsons added a reviewer: malcolm.parsons.

Your email went in my spam folder.
LGTM.
Will commit later.

This revision is now accepted and ready to land.Nov 11 2016, 10:59 AM
malcolm.parsons requested changes to this revision.Nov 12 2016, 9:24 AM
malcolm.parsons edited edge metadata.
Expected Passes    : 4977
Expected Failures  : 18
Unsupported Tests  : 25
Unexpected Failures: 5037

Needs more work?

This revision now requires changes to proceed.Nov 12 2016, 9:24 AM

I sync'ed to the head of the tree. I tried to reproduce the failures that you saw and couldn't. I'm building on Windows 10 x64 using Visual Studio. What platform/OS did you build on? I built and test both Debug and RelWithDebugInfo.

Here's what I saw for RelWithDebugInfo

Running the Clang regression tests
-- Testing: 10026 tests, 24 threads --

Testing Time: 604.17s
  Expected Passes    : 9922
  Expected Failures  : 21
  Unsupported Tests  : 83

What platform/OS did you build on?

I build release with asserts on 64 bit Linux. My host compiler is gcc.

dtarditi updated this revision to Diff 78375.Nov 17 2016, 9:35 AM
dtarditi edited edge metadata.

The parameter array needed to be initialized so that assignments involving unique pointers work properly. The memory could be uninitialized according to C++ semantics.. Visual C++ was zeroing the memory and GCC was not. This is why the tests passed on Windows and failed on Linux. I updated Sema/DeclSpec.cpp to zero the parameter array before it is used.

Here are the test results from an x64 Linux Ubuntu box:

Testing Time: 465.12s

Expected Passes    : 10017
Expected Failures  : 18
Unsupported Tests  : 27
malcolm.parsons accepted this revision.Nov 17 2016, 9:40 AM
malcolm.parsons edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Nov 17 2016, 9:40 AM
This revision was automatically updated to reflect the committed changes.