Subzero: Add locking to prepare for multithreaded translation.

Subzero: Add locking to prepare for multithreaded translation.

This just gets the locking in place.  Actual multithreading will be added later.

Mutexes are added for accessing the GlobalContext allocator, the constant pool, the stats data, and the profiling timers.  These are managed via the LockedPtr<> helper.  Finer grain locks on the constant pool may be added later, i.e. a separate lock for each data type.

An vector of pointers to TLS objects is added to GlobalContext.  Each new thread will get its own TLS object, whose address is added to the vector.  (After threads complete, things like stats can be combined by iterating over the vector.)

The dump/emit streams are guarded by a separate lock, to avoid fine-grain interleaving of output by multiple threads.  E.g., lock the streams, emit an entire function, and unlock the streams.  This works for dumping too, though dump output for different passes on the same function may be interleaved with that of another thread.  There is an OstreamLocker helper class to keep this simple.

CodeStats is made an inner class of GlobalContext (this was missed on a previous CL).

BUG= none
R=jfb@chromium.org, jvoung@chromium.org, kschimpf@google.com

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

[Jim Stichnoth, 2015-01-15 00:38:55]

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

[Jim Stichnoth, 2015-01-15 00:45:05]

Performance penalty for the extra locking seems to be either 0.5% or in the
noise.

[JF, 2015-01-15 01:31:01]

jfb@chromium.org changed reviewers:
+ jfb@chromium.org

[JF, 2015-01-15 01:31:01]

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:136: AllThreadContexts.push_back(new ThreadContext());
Do you need to lock AllThreadContexts when doing a push_back?

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:387: // All locking is done by the getConstant*()
target function.
Can't all of the above known constants be create at Context creation time, and
locking only occur for the other constants?

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:74: typedef std::recursive_mutex StrLockType;
Why recursive? I'd leave a TODO here to remove the heavy stuff and just rely on
tsan.

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:253: bool IsStrLocked;
If the intent is to detect races then IsStrLocked should be atomic.

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:316: OstreamLocker &operator=(const OstreamLocker &) =
delete;
OStreamLocker() = delete;

[Jim Stichnoth, 2015-01-15 07:16:29]

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:136: AllThreadContexts.push_back(new ThreadContext());
On 2015/01/15 01:31:01, JF wrote:
> Do you need to lock AllThreadContexts when doing a push_back?

No since no other thread would have access to this GlobalContext object until at
least after the ctor runs.  Added that to the comment.

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:387: // All locking is done by the getConstant*()
target function.
On 2015/01/15 01:31:01, JF wrote:
> Can't all of the above known constants be create at Context creation time, and
> locking only occur for the other constants?

It's true that most of the known lowering constants could be looked up at Target
creation time.

I think the best way to reduce contention (if it actually turns out to be a
problem) is keep a thread-local cache of constant pool entries.

Added a TODO about this.

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:74: typedef std::recursive_mutex StrLockType;
On 2015/01/15 01:31:01, JF wrote:
> Why recursive? I'd leave a TODO here to remove the heavy stuff and just rely
on
> tsan.

(I moved this typedef down to where it's actually used, to keep all the stream
locking documentation together.)

I want to be able to enforce a discipline where a thread grabs the stream lock,
then does a whole bunch of work where far-off callees grab the stream and do
something with it, and finally the thread releases the stream lock.  In that
sense, the mutex should be non-recursive.  When someone grabs the stream, I want
to validate that they are already holding the stream lock.  The easiest way I
could think to do that is with the bool IsStrLocked field.  But IsStrLocked can
only be safely read while the mutex is held, so for safety the check ends up
being made while the mutex is recursively locked twice.

I added a big comment to try to explain this.

I plan to use tsan in any case, but since these streams are so integral to
development yet so unused in production, I'd prefer the developer to get
immediate feedback on stream locking protocol violations.

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:253: bool IsStrLocked;
On 2015/01/15 01:31:01, JF wrote:
> If the intent is to detect races then IsStrLocked should be atomic.

I changed it so that IsStrLocked is only read or written while StrLock is held. 
I.e., StrLock guards IsStrLocked, StrDump, and StrEmit.  I think it should be
thread-safe now, but please check...

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:316: OstreamLocker &operator=(const OstreamLocker &) =
delete;
On 2015/01/15 01:31:01, JF wrote:
> OStreamLocker() = delete;

Done.

[JF, 2015-01-15 16:28:39]

Overall I think it would be cleaner if each lock were only ever acquired in one
place: the get method should return a unique_ptr that contains the stream and
the lock, the stats and the lock, or the allocator and the lock. The
unique_ptr's dtor then unlocks.

If you make it impossible to obtain the stream/stats/allocator in any other way
then through the get method (which always bundles the object with a lock) then
the code is impossible to mess up and easier to understand.

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:253: bool IsStrLocked;
On 2015/01/15 07:16:29, stichnot wrote:
> On 2015/01/15 01:31:01, JF wrote:
> > If the intent is to detect races then IsStrLocked should be atomic.
> 
> I changed it so that IsStrLocked is only read or written while StrLock is
held. 
> I.e., StrLock guards IsStrLocked, StrDump, and StrEmit.  I think it should be
> thread-safe now, but please check...

The goal of IsStrLocked is to guarantee thread-safety, so using a straight bool
is thread-safe as long as the lock is held. If you assume that it's used in a
thread-unsafe way then you have to use an atomic. When you do get everything
right the atomic is indeed redundant.

https://codereview.chromium.org/848193003/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:73: typedef std::recursive_mutex GlobalLockType;
Could you instead have separate allocator lock and stats lock?

[Karl, 2015-01-15 17:35:43]

I agree (in general) with JF's comments about OstreamLocker. However, I would
not have a problem with creating an instance of OstreamLocker (whose constructor
grabs the context and defines the lock) so long as one uses a getStr() accessor
(of the OstreamLock) to access the stream. Inside that method, it does the
necessary checks to verify it is valid.

[jvoung (off chromium), 2015-01-15 18:20:40]

https://codereview.chromium.org/848193003/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceCfg.cpp#newcode461
src/IceCfg.cpp:461: if (!Ctx->getFlags().UseELFWriter)
Technically, the lock doesn't need to be held this long. The loop through
CfgNodes can be done without the lock, and just the emitTextHeader /
writeFunctionCode and emitIASBytes need it.

It might be okay to do the loop first, and move the emitTextHeader to be after,
since the .s fallback only writes in bulk via emitIASBytes.

https://codereview.chromium.org/848193003/diff/40001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:461: // No locking because only the bitcode parser
thread calls it.
fwiw, chrome has a "base/threading/thread_checker.h" utility which let's you
assert that certain methods are always called by only one specific thread.

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1027: if (Ctx->getFlags().UseELFWriter) {
I think the elfwriter will also write to a stream.

https://codereview.chromium.org/848193003/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:169: // Note: This gives the reader wild access to the
stream.
What does "wild" access mean? =)

[Jim Stichnoth, 2015-01-17 18:44:17]

I've addressed all the comments except those regarding the stream
bracketing/locking.  More on that later...

https://codereview.chromium.org/848193003/diff/40001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceCfg.cpp#newcode461
src/IceCfg.cpp:461: if (!Ctx->getFlags().UseELFWriter)
On 2015/01/15 18:20:40, jvoung wrote:
> Technically, the lock doesn't need to be held this long. The loop through
> CfgNodes can be done without the lock, and just the emitTextHeader /
> writeFunctionCode and emitIASBytes need it.
> 
> It might be okay to do the loop first, and move the emitTextHeader to be
after,
> since the .s fallback only writes in bulk via emitIASBytes.

Done.

https://codereview.chromium.org/848193003/diff/40001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceGlobalContext.cpp...
src/IceGlobalContext.cpp:461: // No locking because only the bitcode parser
thread calls it.
On 2015/01/15 18:20:40, jvoung wrote:
> fwiw, chrome has a "base/threading/thread_checker.h" utility which let's you
> assert that certain methods are always called by only one specific thread.

Acknowledged.

Also, on further inspection, this method and the one below seem unnecessary
because GlobalDeclarations is never actually used.  Added a TODO for me and Karl
to investigate.

https://codereview.chromium.org/848193003/diff/40001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:73: typedef std::recursive_mutex GlobalLockType;
On 2015/01/15 16:28:39, JF wrote:
> Could you instead have separate allocator lock and stats lock?

Done.  I went ahead and made several finer-grain locks: allocator, stats,
timers, and constant pool.  (There's still room to refine the constant pool lock
to be per-type, and/or to keep a thread-local cache.)

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1027: if (Ctx->getFlags().UseELFWriter) {
On 2015/01/15 18:20:40, jvoung wrote:
> I think the elfwriter will also write to a stream.

Good point.  Lifted the locker to cover the whole method.

https://codereview.chromium.org/848193003/diff/40001/src/PNaClTranslator.cpp
File src/PNaClTranslator.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/PNaClTranslator.cpp#...
src/PNaClTranslator.cpp:169: // Note: This gives the reader wild access to the
stream.
On 2015/01/15 18:20:40, jvoung wrote:
> What does "wild" access mean? =)

Clarified the comment - it means the stream is being stashed somewhere and can
be improperly used without locking.  Added a TODO for Karl about possibly
changing the LLVM-side interface.

[JF, 2015-01-18 03:14:20]

https://codereview.chromium.org/848193003/diff/60001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/848193003/diff/60001/src/IceCfg.cpp#newcode473
src/IceCfg.cpp:473: getAssembler<Assembler>()->emitIASBytes(Ctx);
What is this locking exactly? It sounds like it's locking the byte emitter, so
the right approach may be to have getAssembler<Assembler>()->getByteEmitter()
return a {ByteEmitter, ByteEmitterLock}, and then call emitIASBytes on that?

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

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:59: public:
This is already public.

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:60: LockedPtr(T *Value, GlobalLockType &Lock) : Ptr(new
LockedPtrWrapper(Value, Lock)) {}
The allocation here is pretty unfortunate. You want the move and unique
ownership semantics of unique_ptr, but without the extra allocation (you want
unique_ptr to contain the LockedPtrWrapper itself, not a pointer to one).

I think you can use unique_ptr's Deleter class template argument to only call
Ptr.~LockedPtrWrapper? If not it's probably better to replicate unique_ptr's
semantics (move constructible, move assignable, not copy constructible, not copy
assignable) but containing the value/lock pair instead of a pointer.

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:300: bool IsStrLocked;
I would still drop IsStrLocked, since tsan should do the same and be simpler.

[Jim Stichnoth, 2015-01-19 21:44:05]

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

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:60: LockedPtr(T *Value, GlobalLockType &Lock) : Ptr(new
LockedPtrWrapper(Value, Lock)) {}
On 2015/01/18 03:14:19, JF wrote:
> The allocation here is pretty unfortunate. You want the move and unique
> ownership semantics of unique_ptr, but without the extra allocation (you want
> unique_ptr to contain the LockedPtrWrapper itself, not a pointer to one).
> 
> I think you can use unique_ptr's Deleter class template argument to only call
> Ptr.~LockedPtrWrapper? If not it's probably better to replicate unique_ptr's
> semantics (move constructible, move assignable, not copy constructible, not
copy
> assignable) but containing the value/lock pair instead of a pointer.

Yeah, I think trying to use unique_ptr without an actual pointer may be
overkill.  We only want to lock the resource for a lexically scoped region,
never stashing it away or transferring its ownership.

How about this updated version of LockedPtr?  (I think I do not want move
assignment enabled.)

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:300: bool IsStrLocked;
On 2015/01/18 03:14:19, JF wrote:
> I would still drop IsStrLocked, since tsan should do the same and be simpler.

Right, I didn't get to that part yet. :)

Here's what I'm thinking.  Continue with a std::lock_guard scoped locking
pattern for reserving the stream lock.  The dump stream's lock can be recursive
so you can grab the lock anywhere for debugging output as needed.  The emit
stream's lock can be non-recursive if desired.  (Though performance is not an
issue with the textual dump/emit code.)

Agreed that tsan can catch conflicting uses of the streams without taking the
lock, though that depends on running tsan builds with suitably high levels of
debugging output.  It would also be possible to use a second lock along with
LockedPtr<> to make it safer.

[JF, 2015-01-20 01:23:05]

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

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:60: LockedPtr(T *Value, GlobalLockType &Lock) : Ptr(new
LockedPtrWrapper(Value, Lock)) {}
On 2015/01/19 21:44:05, stichnot wrote:
> On 2015/01/18 03:14:19, JF wrote:
> > The allocation here is pretty unfortunate. You want the move and unique
> > ownership semantics of unique_ptr, but without the extra allocation (you
want
> > unique_ptr to contain the LockedPtrWrapper itself, not a pointer to one).
> > 
> > I think you can use unique_ptr's Deleter class template argument to only
call
> > Ptr.~LockedPtrWrapper? If not it's probably better to replicate unique_ptr's
> > semantics (move constructible, move assignable, not copy constructible, not
> copy
> > assignable) but containing the value/lock pair instead of a pointer.
> 
> Yeah, I think trying to use unique_ptr without an actual pointer may be
> overkill.  We only want to lock the resource for a lexically scoped region,
> never stashing it away or transferring its ownership.
> 
> How about this updated version of LockedPtr?  (I think I do not want move
> assignment enabled.)

Much better.

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:300: bool IsStrLocked;
On 2015/01/19 21:44:04, stichnot wrote:
> On 2015/01/18 03:14:19, JF wrote:
> > I would still drop IsStrLocked, since tsan should do the same and be
simpler.
> 
> Right, I didn't get to that part yet. :)
> 
> Here's what I'm thinking.  Continue with a std::lock_guard scoped locking
> pattern for reserving the stream lock.  The dump stream's lock can be
recursive
> so you can grab the lock anywhere for debugging output as needed.  The emit
> stream's lock can be non-recursive if desired.  (Though performance is not an
> issue with the textual dump/emit code.)

Is recursive locking of the stream actually needed? It seems like passing a
reference to the already-locked stream would be better.

> Agreed that tsan can catch conflicting uses of the streams without taking the
> lock, though that depends on running tsan builds with suitably high levels of
> debugging output.  It would also be possible to use a second lock along with
> LockedPtr<> to make it safer.

A lot of this discussion is hinging on having a tsan build tested
systematically. Maybe Subzero should have that build? :-)

https://codereview.chromium.org/848193003/diff/80001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/80001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:39: LockedPtr &operator=(const LockedPtr &) = delete;
`LockedPtr() = delete;`

https://codereview.chromium.org/848193003/diff/80001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:45: Other.Value = nullptr;
It's probably better to hold Lock as a pointer instead, so that this method sets
`Other.Lock = nullptr` and catches any unlock on a non-owned mutex.

[Jim Stichnoth, 2015-01-20 16:22:54]

I think I've addressed all the comments, but please double-check that I haven't
missed anything.

I'll update the commit message after the dust has settled.

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/20001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:253: bool IsStrLocked;
On 2015/01/15 16:28:39, JF wrote:
> On 2015/01/15 07:16:29, stichnot wrote:
> > On 2015/01/15 01:31:01, JF wrote:
> > > If the intent is to detect races then IsStrLocked should be atomic.
> > 
> > I changed it so that IsStrLocked is only read or written while StrLock is
> held. 
> > I.e., StrLock guards IsStrLocked, StrDump, and StrEmit.  I think it should
be
> > thread-safe now, but please check...
> 
> The goal of IsStrLocked is to guarantee thread-safety, so using a straight
bool
> is thread-safe as long as the lock is held. If you assume that it's used in a
> thread-unsafe way then you have to use an atomic. When you do get everything
> right the atomic is indeed redundant.

I removed the IsStrLocked field.  The stream lock is now only acquired for
bracketing purposes, and acquiring the stream itself doesn't do any locking or
checking.

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1027: if (Ctx->getFlags().UseELFWriter) {
On 2015/01/17 18:44:17, stichnot wrote:
> On 2015/01/15 18:20:40, jvoung wrote:
> > I think the elfwriter will also write to a stream.
> 
> Good point.  Lifted the locker to cover the whole method.

On second thought, I went back to the original change, for the same reason we
only lock the emit stream in Cfg::emitIAS() when Ctx->getFlags().UseELFWriter is
false.

https://codereview.chromium.org/848193003/diff/60001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/848193003/diff/60001/src/IceCfg.cpp#newcode473
src/IceCfg.cpp:473: getAssembler<Assembler>()->emitIASBytes(Ctx);
On 2015/01/18 03:14:19, JF wrote:
> What is this locking exactly? It sounds like it's locking the byte emitter, so
> the right approach may be to have getAssembler<Assembler>()->getByteEmitter()
> return a {ByteEmitter, ByteEmitterLock}, and then call emitIASBytes on that?

emitTextHeader() and emitIASBytes() are called when we're not emitting to an ELF
file.  In that situation, code bytes, .text directives, and the like are emitted
to the textual asm file, and so the emit stream needs to be locked.

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

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:59: public:
On 2015/01/18 03:14:19, JF wrote:
> This is already public.

I don't think so -- the only thing public above was the public portion of the
private inner class.  It's moot though since I redid LockedPtr.

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:300: bool IsStrLocked;
On 2015/01/20 01:23:05, JF wrote:
> On 2015/01/19 21:44:04, stichnot wrote:
> > On 2015/01/18 03:14:19, JF wrote:
> > > I would still drop IsStrLocked, since tsan should do the same and be
> simpler.
> > 
> > Right, I didn't get to that part yet. :)
> > 
> > Here's what I'm thinking.  Continue with a std::lock_guard scoped locking
> > pattern for reserving the stream lock.  The dump stream's lock can be
> recursive
> > so you can grab the lock anywhere for debugging output as needed.  The emit
> > stream's lock can be non-recursive if desired.  (Though performance is not
an
> > issue with the textual dump/emit code.)
> 
> Is recursive locking of the stream actually needed? It seems like passing a
> reference to the already-locked stream would be better.

Currently Subzero is pretty sparing with its dump output, so there may not be an
example in the code base I can point to.  But what I have in mind is this. 
Suppose I have a method like this:

void Inst::validate() {
  if (something_wrong) {
    OstreamLocker L(...);
    Ostream &Str = getStrDump();
    Str << "Validation error: ";
    this->dumpDecorated();
    Str << "\n";
  }
}

This method might be called as part of Cfg::dump() while the stream lock is
held, or it might be called as part of another analysis/transformation phase
while the stream lock is not held.  Of course one can deal with this by having
separate locking and non-locking versions of validate(), or passing the locked
state through the call chain.  But I think the most user-friendly solution,
especially with textual dumping/emitting where performance is not a concern, is
to use a recursive mutex.

> 
> > Agreed that tsan can catch conflicting uses of the streams without taking
the
> > lock, though that depends on running tsan builds with suitably high levels
of
> > debugging output.  It would also be possible to use a second lock along with
> > LockedPtr<> to make it safer.
> 
> A lot of this discussion is hinging on having a tsan build tested
> systematically. Maybe Subzero should have that build? :-)

Yes, and actual threading.  And blackjack. :)  I'll do that as part of the next
CL.

https://codereview.chromium.org/848193003/diff/80001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/80001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:39: LockedPtr &operator=(const LockedPtr &) = delete;
On 2015/01/20 01:23:05, JF wrote:
> `LockedPtr() = delete;`

Oops.  Done.

https://codereview.chromium.org/848193003/diff/80001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:45: Other.Value = nullptr;
On 2015/01/20 01:23:05, JF wrote:
> It's probably better to hold Lock as a pointer instead, so that this method
sets
> `Other.Lock = nullptr` and catches any unlock on a non-owned mutex.

Done.

[JF, 2015-01-20 17:20:21]

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

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:300: bool IsStrLocked;
On 2015/01/20 16:22:54, stichnot wrote:
> On 2015/01/20 01:23:05, JF wrote:
> > On 2015/01/19 21:44:04, stichnot wrote:
> > > On 2015/01/18 03:14:19, JF wrote:
> > > > I would still drop IsStrLocked, since tsan should do the same and be
> > simpler.
> > > 
> > > Right, I didn't get to that part yet. :)
> > > 
> > > Here's what I'm thinking.  Continue with a std::lock_guard scoped locking
> > > pattern for reserving the stream lock.  The dump stream's lock can be
> > recursive
> > > so you can grab the lock anywhere for debugging output as needed.  The
emit
> > > stream's lock can be non-recursive if desired.  (Though performance is not
> an
> > > issue with the textual dump/emit code.)
> > 
> > Is recursive locking of the stream actually needed? It seems like passing a
> > reference to the already-locked stream would be better.
> 
> Currently Subzero is pretty sparing with its dump output, so there may not be
an
> example in the code base I can point to.  But what I have in mind is this. 
> Suppose I have a method like this:
> 
> void Inst::validate() {
>   if (something_wrong) {
>     OstreamLocker L(...);
>     Ostream &Str = getStrDump();
>     Str << "Validation error: ";
>     this->dumpDecorated();
>     Str << "\n";
>   }
> }
> 
> This method might be called as part of Cfg::dump() while the stream lock is
> held, or it might be called as part of another analysis/transformation phase
> while the stream lock is not held.  Of course one can deal with this by having
> separate locking and non-locking versions of validate(), or passing the locked
> state through the call chain.  But I think the most user-friendly solution,
> especially with textual dumping/emitting where performance is not a concern,
is
> to use a recursive mutex.

As we discussed offline: recursive mutexes make me sad, I don't think you need
them here if the code follows some form of standard for locking.

> > > Agreed that tsan can catch conflicting uses of the streams without taking
> the
> > > lock, though that depends on running tsan builds with suitably high levels
> of
> > > debugging output.  It would also be possible to use a second lock along
with
> > > LockedPtr<> to make it safer.
> > 
> > A lot of this discussion is hinging on having a tsan build tested
> > systematically. Maybe Subzero should have that build? :-)
> 
> Yes, and actual threading.  And blackjack. :)  I'll do that as part of the
next
> CL.

Let me get my fur coat.

https://codereview.chromium.org/848193003/diff/100001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/100001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:274: return LockedPtr<ArenaAllocator<> >(&Allocator,
&AllocLock);
Y  u  s p a c e ?

[jvoung (off chromium), 2015-01-20 17:34:30]

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1027: if (Ctx->getFlags().UseELFWriter) {
On 2015/01/20 16:22:54, stichnot wrote:
> On 2015/01/17 18:44:17, stichnot wrote:
> > On 2015/01/15 18:20:40, jvoung wrote:
> > > I think the elfwriter will also write to a stream.
> > 
> > Good point.  Lifted the locker to cover the whole method.
> 
> On second thought, I went back to the original change, for the same reason we
> only lock the emit stream in Cfg::emitIAS() when Ctx->getFlags().UseELFWriter
is
> false.

Hmm, right... and this is only called once after joining all the threads (if the
elf-writer thread is also joined)?

[Jim Stichnoth, 2015-01-20 18:14:54]

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/848193003/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1027: if (Ctx->getFlags().UseELFWriter) {
On 2015/01/20 17:34:30, jvoung wrote:
> On 2015/01/20 16:22:54, stichnot wrote:
> > On 2015/01/17 18:44:17, stichnot wrote:
> > > On 2015/01/15 18:20:40, jvoung wrote:
> > > > I think the elfwriter will also write to a stream.
> > > 
> > > Good point.  Lifted the locker to cover the whole method.
> > 
> > On second thought, I went back to the original change, for the same reason
we
> > only lock the emit stream in Cfg::emitIAS() when
Ctx->getFlags().UseELFWriter
> is
> > false.
> 
> Hmm, right... and this is only called once after joining all the threads (if
the
> elf-writer thread is also joined)?

I think this structure will have to be redone when I add threading.  There will
be 3 ways to emit code:

1. ELF emission.  The ELF emitter thread takes assembler buffers from the work
queue, and emits to the stream/file without needing a lock.

2. Old-style "readable" asm.  The translator thread grabs the emit stream lock
and executes Cfg::emit().  The ELF emitter thread does nothing.

3. Text asm with the integrated assembler (".byte" style asm).  Here I think the
ELF emitter thread will handle the text asm emission.  Since it's the only
thread doing emission, there will be no need to grab the emitter lock.

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

https://codereview.chromium.org/848193003/diff/60001/src/IceGlobalContext.h#n...
src/IceGlobalContext.h:300: bool IsStrLocked;
On 2015/01/20 17:20:21, JF wrote:
> As we discussed offline: recursive mutexes make me sad, I don't think you need
> them here if the code follows some form of standard for locking.

OK, changed to non-recursive mutex, keeping the separate typedef in case we need
to revisit later.

https://codereview.chromium.org/848193003/diff/100001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/848193003/diff/100001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:274: return LockedPtr<ArenaAllocator<> >(&Allocator,
&AllocLock);
On 2015/01/20 17:20:21, JF wrote:
> Y  u  s p a c e ?

Thanks, clang-format!  Fixed here and in IceDefs.h and IceOperand.h.

[JF, 2015-01-20 19:27:56]

(ﾉ◕ヮ◕)ﾉ*:・ﾟ✧ lgtm

[jvoung (off chromium), 2015-01-20 19:51:05]

< lgtm >
 ------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||

[Karl, 2015-01-20 20:00:36]

lgtm

https://codereview.chromium.org/848193003/diff/120001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/848193003/diff/120001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:453: // seems to be unused.  If so, remove that field
and this method.
Feel free to remove.

https://codereview.chromium.org/848193003/diff/120001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:467: // seems to be unused.  If so, remove that field
and this method.
Feel free to remove.

[Jim Stichnoth, 2015-01-20 20:52:55]

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