Subzero. Fixes memory leaks.

Subzero. Fixes memory leaks.

Adds named constructors to initialzers. Removes destructor from Inst.

BUG= None
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=1bec8bcd64da0cd4855a357d740d3ad45bbef3b9

[John, 2015-06-18 20:19:42]

Patchset #3 (id:40001) has been deleted

[John, 2015-06-18 20:19:48]

Patchset #2 (id:20001) has been deleted

[John, 2015-06-19 17:59:53]

jpp@chromium.org changed reviewers:
+ jvoung@chromium.org, kschimpf@google.com, stichnot@chromium.org

[John, 2015-06-19 17:59:54]

This is a big CL dealing with several sources of memory leaks in subzero.

It tries to standardize memory management by either allocating memory managed by
the allocators, or returning unique_ptrs to heap-allocated memory.

It also changes the GlobalContext::allocate<> methods to register destructors
for allocations the require a destructor to be invoked. Elements allocated in
GlobalContext should be few (in comparison to what's allocated in Cfgs), so
allowing destructors to be invoked allows IceGlobalContext::allocate<> clients
to allocate more complicated types (e.g., ElfSections.) The call is optimized so
that destructors are only invoked for types that require.

TODO: Cfg::allocate<> should only allow trivially destructible types to be
allocated. This seems rather draconian, but we want Cfg locals to be quickly
erased (i.e., without having to invoked a destructor.) This is currently **not**
the case. In particular, Ice::Variables hold an std::vector<> in them, and
because of that they are not trivially destructible. The current solution of
allocating the std::vector's memory using Subzero allocators work, but there
could be problems as it suppresses destructor calls.

TODO: the <<type>>::create() methods should never return a raw pointer. They
should either return a unique_ptr (if they are head allocated with new); a
CfgLocalPtr<> (new type for holding pointers to data allocated using placement
new with data allocated from the cfg local pool); or GlobalContextPtr<> (almost
like CfgLocalPtr, but for allocations using GlobalContext::allocate.)

[Jim Stichnoth, 2015-06-19 23:09:25]

btw, Variable contains a std::vector with a nontrivial dtor but the vector
allocates its storage from the Cfg allocator, so Variable's vector shouldn't
need its dtor to be run unless the dtor is doing something other than reclaiming
memory.

But if this is not a valid assumption, then we could make our own simple vector
class for that purpose.

https://codereview.chromium.org/1181013016/diff/160001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1181013016/diff/160001/src/IceDefs.h#newcode126
src/IceDefs.h:126: } // namespace Internal
"end of namespace Internal" for code base consistency

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h...
src/IceGlobalContext.h:216: // Allocate data of type T using the global
allocator. We allow entites
entities

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h...
src/IceGlobalContext.h:217: // allocated from this global allocator to be either
trivially and
either ... or

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h...
src/IceGlobalContext.h:222: typename
std::enable_if<std::is_trivially_destructible<T>::value, T>::type *
This is a cool trick, but I feel that the user might unknowingly allocate lots
of objects with nontrivial dtors.

What do you think of calling them something like allocate<>() and
allocateWithDtor<>()?  This forces the user to make a conscious choice.

Since misuse would cause such horrible error messages, maybe use static_assert
instead of enable_if<>?

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalInits.h#n...
src/IceGlobalInits.h:23: #include "llvm/Bitcode/NaCl/NaClBitcodeParser.h" // for
NaClBitcodeRecord.
be consistent about ending or not ending the comment with a period

[John, 2015-06-20 00:10:34]

https://codereview.chromium.org/1181013016/diff/160001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1181013016/diff/160001/src/IceDefs.h#newcode126
src/IceDefs.h:126: } // namespace Internal
On 2015/06/19 23:09:25, stichnot wrote:
> "end of namespace Internal" for code base consistency

Done.

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h...
src/IceGlobalContext.h:217: // allocated from this global allocator to be either
trivially and
On 2015/06/19 23:09:25, stichnot wrote:
> either ... or

Done.

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h...
src/IceGlobalContext.h:222: typename
std::enable_if<std::is_trivially_destructible<T>::value, T>::type *
On 2015/06/19 23:09:25, stichnot wrote:
> This is a cool trick, but I feel that the user might unknowingly allocate lots
> of objects with nontrivial dtors.
And that's fine. This allocator, essentially, "replaces" the heap so that we
don't have to keep track of memory management (which is bad in my opinion, but
we already have allocators, so...)

> 
> What do you think of calling them something like allocate<>() and
> allocateWithDtor<>()?  This forces the user to make a conscious choice.
>
This makes sense in the CfgLocalAllocator as those objects need to be deleted
fast. This global pool should simply run destructors when memory is being
reclaimed. If you don't want you type do have a destructor, you're better off
saying

~Foo() = delete

This way, the compiler will yell if you try to destroy the object (including
implicit stack allocations when the go out of scope, and unique_ptrs.)


> Since misuse would cause such horrible error messages, maybe use static_assert
> instead of enable_if<>?
It would not. clang++ says "something something std::enable_if<>", and it
highlights enable_if. clang++'s gone a long way since g++ cryptic error
messages. :)

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalInits.h
File src/IceGlobalInits.h (right):

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalInits.h#n...
src/IceGlobalInits.h:23: #include "llvm/Bitcode/NaCl/NaClBitcodeParser.h" // for
NaClBitcodeRecord.
On 2015/06/19 23:09:25, stichnot wrote:
> be consistent about ending or not ending the comment with a period

Done.

[Jim Stichnoth, 2015-06-21 00:15:49]

lgtm

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/1181013016/diff/160001/src/IceGlobalContext.h...
src/IceGlobalContext.h:222: typename
std::enable_if<std::is_trivially_destructible<T>::value, T>::type *
On 2015/06/20 00:10:33, John wrote:
> On 2015/06/19 23:09:25, stichnot wrote:
> > What do you think of calling them something like allocate<>() and
> > allocateWithDtor<>()?  This forces the user to make a conscious choice.
> >
> This makes sense in the CfgLocalAllocator as those objects need to be deleted
> fast. This global pool should simply run destructors when memory is being
> reclaimed. If you don't want you type do have a destructor, you're better off
> saying
> 
> ~Foo() = delete

OK, I buy that.

[John, 2015-06-22 17:51:31]

Committed patchset #8 (id:180001) manually as
1bec8bcd64da0cd4855a357d740d3ad45bbef3b9 (presubmit successful).