Subzero: Manage each Cfg as a std::unique_ptr<Cfg>.

Subzero: Manage each Cfg as a std::unique_ptr<Cfg>.

The Cfg::create() method now returns a unique_ptr.  Once the parser fully builds the Cfg, it is released onto the work queue, and then acquired and ultimately deleted by the translator thread.

BUG= none
R=jfb@chromium.org

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

[Jim Stichnoth, 2015-02-02 00:00:53]

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

[Jim Stichnoth, 2015-02-02 00:07:12]

Sorry for the size of this CL.  After getting unique_ptr<> in place, I thought
it was bad to have so many .get() operations when creating new instructions, and
I thought it would be relatively minor to add some new Cfg::makeInst*() methods,
but the cascading changes ended up being more than I expected.

If anyone wants, I can try to separate this into two CLs.

[Jim Stichnoth, 2015-02-02 06:06:41]

On 2015/02/02 00:07:12, stichnot wrote:
> Sorry for the size of this CL.  After getting unique_ptr<> in place, I thought
> it was bad to have so many .get() operations when creating new instructions,
and
> I thought it would be relatively minor to add some new Cfg::makeInst*()
methods,
> but the cascading changes ended up being more than I expected.
> 
> If anyone wants, I can try to separate this into two CLs.

I came to my senses and reverted the Cfg::makeInst*() part.

The following CL can add that back separately.

[Jim Stichnoth, 2015-02-02 16:51:15]

https://codereview.chromium.org/892063002/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/892063002/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:280: void cfgQueueBlockingPush(Cfg *Func) {
CfgQ.blockingPush(Func); }
I've designed this such that the underlying queue contains raw Cfg pointers
rather than unique_ptr<Cfg> values.

1. This may allow a queue implementation involving atomics to insert/remove work
items to/from the queue.

2. The owner of the Cfg also has the Cfg installed in TLS for container arena
allocation.  Releasing the Cfg pointer from its unique_ptr should also remove it
from TLS.  I'm thinking maybe there should be a UniqueCfg class that wraps
unique_ptr<Cfg> and updates TLS on acquire and release.

[JF, 2015-02-02 17:11:47]

> I came to my senses and reverted the Cfg::makeInst*() part.
> 
> The following CL can add that back separately.

Ha! I almost reviewed the CL yesterday and gave up because it was too long. Good
things come to those who wait!

https://codereview.chromium.org/892063002/diff/60001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/892063002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:108: return std::move(Func);
This invalidates the Func data member contained in LLVM2ICEFunctionConverter.
Why does this class contain it, and then give up its ownership?

https://codereview.chromium.org/892063002/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/892063002/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:280: void cfgQueueBlockingPush(Cfg *Func) {
CfgQ.blockingPush(Func); }
On 2015/02/02 16:51:15, stichnot wrote:
> I've designed this such that the underlying queue contains raw Cfg pointers
> rather than unique_ptr<Cfg> values.
> 
> 1. This may allow a queue implementation involving atomics to insert/remove
work
> items to/from the queue.
> 
> 2. The owner of the Cfg also has the Cfg installed in TLS for container arena
> allocation.  Releasing the Cfg pointer from its unique_ptr should also remove
it
> from TLS.  I'm thinking maybe there should be a UniqueCfg class that wraps
> unique_ptr<Cfg> and updates TLS on acquire and release.

I'm not sure I understand, someone pushes by doing:
  cfgQueueBlockingPush(myPtr.release());

And pops with:
  unique_ptr<Cfg> myPtr = cfgQueueBlockingPop();

?

That's weirdly asymmetric. Push and pop should have the same API so that the
ownership is clear. The container can be raw pointers underneath, but as long as
I can see the unique_ptr going in and out then I can reliably say that there's
no leak and the ownership is clear.

In other words: unique_ptr goes in, unique_ptr goes out, you can't explain that.

https://codereview.chromium.org/892063002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/892063002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1857: // The translator now has ownership of UniqueFunc.
The comment isn't right if there were errors! I should probably be in the if ()
scope :-)

Also, what happens if there are errors?

[Karl, 2015-02-02 18:14:10]

https://codereview.chromium.org/892063002/diff/60001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/892063002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:887: translateFcn(std::move(Fcn));
Should you leave the ownership of Func inside FunctionConvert, and have it
explicitly release the pointer to pass to TranslateFcn (cleaning up how owns the
unique_ptr?).

[Jim Stichnoth, 2015-02-02 18:48:48]

https://codereview.chromium.org/892063002/diff/60001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/892063002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:108: return std::move(Func);
On 2015/02/02 17:11:47, JF wrote:
> This invalidates the Func data member contained in LLVM2ICEFunctionConverter.
> Why does this class contain it, and then give up its ownership?

Yeah, I agree that's kind of weird.  The class contains it so that the class's
private methods can operate on it, but then deliberately gives it up to the
caller.

I took Karl's suggestion and made ownership/release more explicit.

This class structure should be cleaned up, but it's not that high a priority
since the code is not part of the production build.

https://codereview.chromium.org/892063002/diff/60001/src/IceConverter.cpp#new...
src/IceConverter.cpp:887: translateFcn(std::move(Fcn));
On 2015/02/02 18:14:10, Karl wrote:
> Should you leave the ownership of Func inside FunctionConvert, and have it
> explicitly release the pointer to pass to TranslateFcn (cleaning up how owns
the
> unique_ptr?).

Done.

https://codereview.chromium.org/892063002/diff/60001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/892063002/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:280: void cfgQueueBlockingPush(Cfg *Func) {
CfgQ.blockingPush(Func); }
On 2015/02/02 17:11:47, JF wrote:
> On 2015/02/02 16:51:15, stichnot wrote:
> > I've designed this such that the underlying queue contains raw Cfg pointers
> > rather than unique_ptr<Cfg> values.
> > 
> > 1. This may allow a queue implementation involving atomics to insert/remove
> work
> > items to/from the queue.
> > 
> > 2. The owner of the Cfg also has the Cfg installed in TLS for container
arena
> > allocation.  Releasing the Cfg pointer from its unique_ptr should also
remove
> it
> > from TLS.  I'm thinking maybe there should be a UniqueCfg class that wraps
> > unique_ptr<Cfg> and updates TLS on acquire and release.
> 
> I'm not sure I understand, someone pushes by doing:
>   cfgQueueBlockingPush(myPtr.release());
> 
> And pops with:
>   unique_ptr<Cfg> myPtr = cfgQueueBlockingPop();
> 
> ?
> 
> That's weirdly asymmetric. Push and pop should have the same API so that the
> ownership is clear. The container can be raw pointers underneath, but as long
as
> I can see the unique_ptr going in and out then I can reliably say that there's
> no leak and the ownership is clear.
> 
> In other words: unique_ptr goes in, unique_ptr goes out, you can't explain
that.

Heh, I was afraid you'd catch that before I fixed it.

Done.

https://codereview.chromium.org/892063002/diff/60001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/892063002/diff/60001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1857: // The translator now has ownership of UniqueFunc.
On 2015/02/02 17:11:47, JF wrote:
> The comment isn't right if there were errors! I should probably be in the if
()
> scope :-)
> 
> Also, what happens if there are errors?

Done.

[JF, 2015-02-02 20:59:20]

https://codereview.chromium.org/892063002/diff/80001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/892063002/diff/80001/src/IceCfg.h#newcode34
src/IceCfg.h:34: Cfg *Func = new Cfg(Ctx);
It's slightly better to make this a unique_ptr too. In this case it doesn't
change much, but idiomatic C++ likes exception-safe code and what if
TLS_SET_FIELD throws oh my.

https://codereview.chromium.org/892063002/diff/80001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/892063002/diff/80001/src/IceConverter.cpp#new...
src/IceConverter.cpp:109: std::unique_ptr<Ice::Cfg> release() { return
std::move(Func); }
That's nicer. Could you leave a TODO to fix?

https://codereview.chromium.org/892063002/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/892063002/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:165: Cfg::updateTLS(Func.get());
Hmm, now TLS is asymmetric. Maybe updateTLS(nullptr) should move to the end of
the loop instead of being in the dtor?

https://codereview.chromium.org/892063002/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:544: }
It would be nice to explain that the cfgQueue takes ownership of the Cfg, and
that these two functions make it look like it's unique_ptr all the way down, but
as an implementation detail it isn't because concurrency.

https://codereview.chromium.org/892063002/diff/80001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/892063002/diff/80001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:57: Cfg::updateTLS(nullptr);
Comment on why you need this updateTLS.

https://codereview.chromium.org/892063002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/892063002/diff/80001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1859: Func.reset();
This currently does updateTLS(nullptr). Is that bad?

[Jim Stichnoth, 2015-02-03 00:48:51]

https://codereview.chromium.org/892063002/diff/80001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/892063002/diff/80001/src/IceCfg.h#newcode34
src/IceCfg.h:34: Cfg *Func = new Cfg(Ctx);
On 2015/02/02 20:59:19, JF wrote:
> It's slightly better to make this a unique_ptr too. In this case it doesn't
> change much, but idiomatic C++ likes exception-safe code and what if
> TLS_SET_FIELD throws oh my.

Done.

https://codereview.chromium.org/892063002/diff/80001/src/IceConverter.cpp
File src/IceConverter.cpp (right):

https://codereview.chromium.org/892063002/diff/80001/src/IceConverter.cpp#new...
src/IceConverter.cpp:109: std::unique_ptr<Ice::Cfg> release() { return
std::move(Func); }
On 2015/02/02 20:59:19, JF wrote:
> That's nicer. Could you leave a TODO to fix?

Done.

https://codereview.chromium.org/892063002/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/892063002/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:165: Cfg::updateTLS(Func.get());
On 2015/02/02 20:59:19, JF wrote:
> Hmm, now TLS is asymmetric. Maybe updateTLS(nullptr) should move to the end of
> the loop instead of being in the dtor?

Done.

https://codereview.chromium.org/892063002/diff/80001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:544: }
On 2015/02/02 20:59:19, JF wrote:
> It would be nice to explain that the cfgQueue takes ownership of the Cfg, and
> that these two functions make it look like it's unique_ptr all the way down,
but
> as an implementation detail it isn't because concurrency.

Done.

https://codereview.chromium.org/892063002/diff/80001/src/IceTranslator.cpp
File src/IceTranslator.cpp (right):

https://codereview.chromium.org/892063002/diff/80001/src/IceTranslator.cpp#ne...
src/IceTranslator.cpp:57: Cfg::updateTLS(nullptr);
On 2015/02/02 20:59:19, JF wrote:
> Comment on why you need this updateTLS.

Gone now, so no comment needed. :)

https://codereview.chromium.org/892063002/diff/80001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/892063002/diff/80001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:1859: Func.reset();
On 2015/02/02 20:59:19, JF wrote:
> This currently does updateTLS(nullptr). Is that bad?

Done.

[JF, 2015-02-03 00:55:57]

lgtm after typo fix.

https://codereview.chromium.org/892063002/diff/120001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/892063002/diff/120001/src/PNaClTranslator.cpp...
src/PNaClTranslator.cpp:1105: // translation of all remaining functions. This
allows use to see
s/use/developers/

[Jim Stichnoth, 2015-02-03 01:03:13]

Committed patchset #8 (id:140001) manually as
8e92838b8179d0b4a3022889c3446100b7ef831e (presubmit successful).