Debug stub: associate NaClAppThread with IThread

Debug stub: associate NaClAppThread with IThread

This will be needed for platform-specific threads manipulation, in particular, for suspend/resume.

As a side effect, split IThread::Create from IThread::Acquire, in particular because NaClAppThread is not available in every acquire point.

BUG=http://code.google.com/p/nativeclient/issues/detail?id=2758
TEST=run_debug_stub_test,run_gdb_rsp_tests

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=8569

[eaeltsin, 2012-05-05 08:30:26]

Please take a look.

Please note, that I would like to make another, separate change for reorganizing
IThread implementations, and, in particular, for dealing with posix/win code
duplications.

[Mark Seaborn, 2012-05-07 21:56:49]

LGTM with the changes below.  I'm not keen on changes that add a field that
isn't used -- you could have combined this with de-duplicating the code, maybe.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
File src/trusted/debug_stub/posix/thread_impl.cc (left):

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/posix/thread_impl.cc:133: // TODO(mseaborn): This is
duplicated in the Windows version.
This TODO should be kept with Create(), because it still applies.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
File src/trusted/debug_stub/posix/thread_impl.cc (right):

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/posix/thread_impl.cc:135: IThread*
IThread::Create(uint32_t id, struct NaClAppThread* natp) {
'*' spacing style differs between here and above.  If there's a doubt, I'd
suggest using " *", which is the prevailing NaCl style.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/posix/thread_impl.cc:141: NaClLog(LOG_ERROR,
"IThread::Create: thread 0x%x already exists\n", id);
This should be LOG_FATAL because the callers are not checking for NULL.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/posix/thread_impl.cc:157: NaClLog(LOG_ERROR,
"IThread::Acquire: thread 0x%x does not exist\n", id);
Shouldn't this be LOG_FATAL too?  The callers aren't checking for NULL.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
File src/trusted/debug_stub/win/thread_impl.cc (right):

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/win/thread_impl.cc:379: NaClLog(LOG_ERROR,
"IThread::Create: thread 0x%x already exists\n", id);
Ditto

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/win/thread_impl.cc:385: delete thread;
FYI, this isn't consistent with the posix implementation, which doesn't have a
NULL check.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/gdb_rsp/te...
File src/trusted/gdb_rsp/test.cc (right):

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/gdb_rsp/te...
src/trusted/gdb_rsp/test.cc:102: IThread* IThread::Create(uint32_t id, struct
NaClAppThread *) {
Please use "struct NaClAppThread *natp"; make sure arguments have names.

[eaeltsin, 2012-05-08 11:09:35]

On 2012/05/07 21:56:49, Mark Seaborn wrote:
> LGTM with the changes below.  I'm not keen on changes that add a field that
> isn't used -- you could have combined this with de-duplicating the code,
maybe.

Good point. Removed the field for now.

[eaeltsin, 2012-05-08 11:10:19]

Fixed the style, except unused parameters being not named.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
File src/trusted/debug_stub/posix/thread_impl.cc (left):

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/posix/thread_impl.cc:133: // TODO(mseaborn): This is
duplicated in the Windows version.
On 2012/05/07 21:56:49, Mark Seaborn wrote:
> This TODO should be kept with Create(), because it still applies.

Done.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
File src/trusted/debug_stub/posix/thread_impl.cc (right):

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/posix/thread_impl.cc:135: IThread*
IThread::Create(uint32_t id, struct NaClAppThread* natp) {
On 2012/05/07 21:56:49, Mark Seaborn wrote:
> '*' spacing style differs between here and above.  If there's a doubt, I'd
> suggest using " *", which is the prevailing NaCl style.

But if I change this, it will differ from below (which seems a bit closer as it
is the same function :)) I'll try clean the style in the whole file when
refactoring this in the subsequent changes.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/posix/thread_impl.cc:141: NaClLog(LOG_ERROR,
"IThread::Create: thread 0x%x already exists\n", id);
On 2012/05/07 21:56:49, Mark Seaborn wrote:
> This should be LOG_FATAL because the callers are not checking for NULL.

Done.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/posix/thread_impl.cc:157: NaClLog(LOG_ERROR,
"IThread::Acquire: thread 0x%x does not exist\n", id);
On 2012/05/07 21:56:49, Mark Seaborn wrote:
> Shouldn't this be LOG_FATAL too?  The callers aren't checking for NULL.

Done.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
File src/trusted/debug_stub/win/thread_impl.cc (right):

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/win/thread_impl.cc:379: NaClLog(LOG_ERROR,
"IThread::Create: thread 0x%x already exists\n", id);
On 2012/05/07 21:56:49, Mark Seaborn wrote:
> Ditto

Done.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/debug_stub...
src/trusted/debug_stub/win/thread_impl.cc:385: delete thread;
On 2012/05/07 21:56:49, Mark Seaborn wrote:
> FYI, this isn't consistent with the posix implementation, which doesn't have a
> NULL check.

This is the old code, kept as is.

Anyway, this code will be refactored very soon.

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/gdb_rsp/te...
File src/trusted/gdb_rsp/test.cc (right):

https://chromiumcodereview.appspot.com/10365028/diff/1/src/trusted/gdb_rsp/te...
src/trusted/gdb_rsp/test.cc:102: IThread* IThread::Create(uint32_t id, struct
NaClAppThread *) {
On 2012/05/07 21:56:49, Mark Seaborn wrote:
> Please use "struct NaClAppThread *natp"; make sure arguments have names.

Not naming unused parameters is a very common C++ practice. It combines a note
from C++ standard (for 2005 see 8.4  Function definitions) that "unused
parameters need not be named" and de-facto standard for compiler behavior to
suppress the "unused parameter" warning in this case (which becomes an error
with our settings). 

I'm not keen on including a file just for using a macro just for suppressing a
warning, but if you insist I can use UNREFERENCED_PARAMETER here.

[Mark Seaborn, 2012-05-09 00:06:06]

http://codereview.chromium.org/10365028/diff/1/src/trusted/gdb_rsp/test.cc
File src/trusted/gdb_rsp/test.cc (right):

http://codereview.chromium.org/10365028/diff/1/src/trusted/gdb_rsp/test.cc#ne...
src/trusted/gdb_rsp/test.cc:102: IThread* IThread::Create(uint32_t id, struct
NaClAppThread *) {
On 2012/05/08 11:10:19, eaeltsin wrote:
> On 2012/05/07 21:56:49, Mark Seaborn wrote:
> > Please use "struct NaClAppThread *natp"; make sure arguments
> > have names.
> 
> Not naming unused parameters is a very common C++ practice. It
> combines a note from C++ standard (for 2005 see 8.4 Function
> definitions) that "unused parameters need not be named" and
> de-facto standard for compiler behavior to suppress the "unused
> parameter" warning in this case (which becomes an error with
> our settings).
>
> I'm not keen on including a file just for using a macro just
> for suppressing a warning, but if you insist I can use
> UNREFERENCED_PARAMETER here.

If a parameter is nameless, how do you view its contents in a debugger?

Names are also good for documenting the code.

I'll let this one go because it's in this test file and this particular test
probably isn't very useful.

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/posi...
File src/trusted/debug_stub/posix/thread_impl.cc (right):

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/posi...
src/trusted/debug_stub/posix/thread_impl.cc:128: struct NaClAppThread *natp_;
I'm confused.  You said "Removed the field for now", but it's still in the Linux
file, but not in the Windows file.  Is that what you intended?

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/posi...
src/trusted/debug_stub/posix/thread_impl.cc:143: return NULL;
ditto

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/posi...
src/trusted/debug_stub/posix/thread_impl.cc:159: return NULL;
ditto

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/win/...
File src/trusted/debug_stub/win/thread_impl.cc (right):

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/win/...
src/trusted/debug_stub/win/thread_impl.cc:213: Thread(uint32_t id, struct
NaClAppThread *)
FWIW, the same criticism of unused fields applies to unused arguments.

I'm not against having an unused natp_ field -- you didn't have to remove it --
I'm just saying I'm cautious of changes that add something unused because it's
hard for me to see how it's going to be used.

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/win/...
src/trusted/debug_stub/win/thread_impl.cc:379: return NULL;
You don't need a return after a LOG_FATAL.

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/win/...
src/trusted/debug_stub/win/thread_impl.cc:399: return NULL;
ditto

[eaeltsin, 2012-05-09 06:01:32]

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/posi...
File src/trusted/debug_stub/posix/thread_impl.cc (right):

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/posi...
src/trusted/debug_stub/posix/thread_impl.cc:128: struct NaClAppThread *natp_;
On 2012/05/09 00:06:06, Mark Seaborn wrote:
> I'm confused.  You said "Removed the field for now", but it's still in the
Linux
> file, but not in the Windows file.  Is that what you intended?

Yes, because for Linux there is a pending prototype for thread suspension.
However, I've returned the field back for Windows as well - instead of not
adding the field now I'll refactor this code ASAP.

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/posi...
src/trusted/debug_stub/posix/thread_impl.cc:143: return NULL;
On 2012/05/09 00:06:06, Mark Seaborn wrote:
> ditto

Done.

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/posi...
src/trusted/debug_stub/posix/thread_impl.cc:159: return NULL;
On 2012/05/09 00:06:06, Mark Seaborn wrote:
> ditto

Done.

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/win/...
File src/trusted/debug_stub/win/thread_impl.cc (right):

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/win/...
src/trusted/debug_stub/win/thread_impl.cc:213: Thread(uint32_t id, struct
NaClAppThread *)
On 2012/05/09 00:06:06, Mark Seaborn wrote:
> FWIW, the same criticism of unused fields applies to unused arguments.
> 
> I'm not against having an unused natp_ field -- you didn't have to remove it
--
> I'm just saying I'm cautious of changes that add something unused because it's
> hard for me to see how it's going to be used.

The field is back for now.

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/win/...
src/trusted/debug_stub/win/thread_impl.cc:379: return NULL;
On 2012/05/09 00:06:06, Mark Seaborn wrote:
> You don't need a return after a LOG_FATAL.

Done.

http://codereview.chromium.org/10365028/diff/7001/src/trusted/debug_stub/win/...
src/trusted/debug_stub/win/thread_impl.cc:399: return NULL;
On 2012/05/09 00:06:06, Mark Seaborn wrote:
> ditto

Done.