Enable --nacl-gdb flag on Linux.

Enable --nacl-gdb flag on Linux. 

I changed --nacl-gdb meaning on Linux. On Windows it means path to nacl-gdb, 
but on linux it means path to nacl-gdb and options/wrappers. Options and paths 
can not contain spaces on Linux.

BUG=117010
TEST= browser_tests --gtest_filter=NaClGdb*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130972

[halyavin, 2012-04-02 15:07:37]

>Can you explain the bigger picture here?  What is all this stuff for
> synchronising via a pipe for?
I need to wait until debugger attaches to the program before I let untrusted
code to start. 

>Why does this require Linux-specific stuff?  Why can't you do all of this from
>nacl_process_host.cc?

Done.

[Mark Seaborn, 2012-04-03 21:57:44]

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:275: #if defined(OS_LINUX)
#elif for neatness.

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:423: cmd_line->AppendArg("-ex");
Please stick with --eval-command since it's more obvious what it does [from
previous review].

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:429: cmd_line->AppendArg("-ex");
ditto

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:447: fds_write_(fds_write),
Why are you passing fds_write through?  It's not used.  Keeping it open in the
browser means you will never see an EOF on fd_read.

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:451: close(fds_read_);
Please do LOG(ERROR) on error from close(), otherwise mistakes in use of file
descriptors will go unnoticed (such mistakes are potentially as dangerous as
memory unsafety).

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:459: int fds_read_;
Should be "fd" not "fds".

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:467: LOG(ERROR) << "Failed to sync
with nacl-gdb";
Suggestion: you might also want to sanity check that you receive the same byte
that you send from gdb (and send a char other than 0).

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:477: base::SplitString(nacl_gdb,
static_cast<CommandLine::CharType>(' '), &argv);
Can you put a comment in here to say that we don't support spaces inside
arguments?

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:479: cmd_line.AppendArg("-ex");
Please use --eval-command here and below.

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:481:
NaClBrowser::GetInstance()->GetIrtFilePath().value();
Indent -2

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:510: return
base::LaunchProcess(cmd_line, base::LaunchOptions(), NULL);
This seems to suggest that all FDs, including fds[1], are inherited, which would
be bad.  But process_util.h seems to suggest that only stdin/stdout/stderr are
left open, in which case how does this work?

Anyway, you should use fds_to_remap in the launch options.

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.cc:727: LOG(ERROR) << "Cannot launch
debugger";
"Failed to launch debugger" might be better

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_process_host.h (right):

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.h:70: #if defined(OS_LINUX)
Use #elif, since this is the Linux version of the previous #if block.

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_process_host.h:99: #if defined(OS_LINUX)
It'd be neater to use #elif here too.

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/tes...
File chrome/browser/nacl_host/test/mock_nacl_gdb.cc (right):

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/tes...
chrome/browser/nacl_host/test/mock_nacl_gdb.cc:16: static const char
kEvalCommand[] = "-ex";
Keep it as --eval-command

http://codereview.chromium.org/9950055/diff/4021/chrome/browser/nacl_host/tes...
chrome/browser/nacl_host/test/mock_nacl_gdb.cc:60: if (strncmp(argv[i - 1],
kDump, sizeof(kDump) - 1) == 0) {
FYI, you're mixing sizeof() and strlen() in this function.  Not a big deal
though.

[halyavin, 2012-04-04 09:23:15]

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:275: #if defined(OS_LINUX)
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> #elif for neatness.

Done.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:423: cmd_line->AppendArg("-ex");
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> Please stick with --eval-command since it's more obvious what it does [from
> previous review].

Done.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:447: fds_write_(fds_write),
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> Why are you passing fds_write through?  It's not used.  Keeping it open in the
> browser means you will never see an EOF on fd_read.
It is used by nacl-gdb. It opens fds_write in our process.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:451: close(fds_read_);
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> Please do LOG(ERROR) on error from close(), otherwise mistakes in use of file
> descriptors will go unnoticed (such mistakes are potentially as dangerous as
> memory unsafety).

Done.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:459: int fds_read_;
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> Should be "fd" not "fds".

Done.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:467: LOG(ERROR) << "Failed to sync
with nacl-gdb";
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> Suggestion: you might also want to sanity check that you receive the same byte
> that you send from gdb (and send a char other than 0).
I added sanity check but I don't think using non-zero char is necessary.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:477: base::SplitString(nacl_gdb,
static_cast<CommandLine::CharType>(' '), &argv);
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> Can you put a comment in here to say that we don't support spaces inside
> arguments?

Done.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:481:
NaClBrowser::GetInstance()->GetIrtFilePath().value();
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> Indent -2

Done.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:510: return
base::LaunchProcess(cmd_line, base::LaunchOptions(), NULL);
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> This seems to suggest that all FDs, including fds[1], are inherited, which
would
> be bad.  But process_util.h seems to suggest that only stdin/stdout/stderr are
> left open, in which case how does this work?
> 
> Anyway, you should use fds_to_remap in the launch options.

nacl-gdb opens file descriptor in our process. I experimented with fds_to_remap
and found that a lot of terminals (gnome-terminal for example) close all file
descriptors before launching external processes. Since launching gdb in terminal
is a requirement and user will be confused why some terminals work and some
doesn't, I decided not to use this approach.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:727: LOG(ERROR) << "Cannot launch
debugger";
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> "Failed to launch debugger" might be better

Done.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
File chrome/browser/nacl_host/nacl_process_host.h (right):

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.h:70: #if defined(OS_LINUX)
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> Use #elif, since this is the Linux version of the previous #if block.

Done.

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.h:99: #if defined(OS_LINUX)
On 2012/04/03 21:57:44, Mark Seaborn wrote:
> It'd be neater to use #elif here too.

Done.

[Mark Seaborn, 2012-04-04 15:46:12]

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/9950055/diff/4021/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:510: return
base::LaunchProcess(cmd_line, base::LaunchOptions(), NULL);
On 2012/04/04 09:23:15, halyavin wrote:
> On 2012/04/03 21:57:44, Mark Seaborn wrote:
> > This seems to suggest that all FDs, including fds[1], are
> > inherited, which would be bad.  But process_util.h seems to
> > suggest that only stdin/stdout/stderr are left open, in which
> > case how does this work?
> > 
> > Anyway, you should use fds_to_remap in the launch options.
> 
> nacl-gdb opens file descriptor in our process. I experimented
> with fds_to_remap and found that a lot of
> terminals (gnome-terminal for example) close all file
> descriptors before launching external processes. Since
> launching gdb in terminal is a requirement and user will be
> confused why some terminals work and some doesn't, I decided
> not to use this approach.

I missed that.  It's totally non-obvious.  It definitely needs a comment.

It also means that if something goes wrong and gdb dies, the browser will not
receive EOF on the pipe, and the NaCl process's startup will get stuck, and I
think these data structures in the browser will leak.  This needs a comment too.

I'm a bit uneasy about having another process write to the browser's FDs.  If it
goes wrong, it will mess up some random FD in the browser.

Another solution might be to have a shell script that wraps gdb and does this:

#!/bin/bash
notify_fd="$1"
shift
# Can't use "exec" because we have to stay running for /proc/$$ to work
gnome-terminal -x gdb "$@" --eval-command "dump binary value
/proc/$$/fd/$notify_fd (char)0"

This would make me happier because it would keep the workarounds for others'
code out of Chromium's codebase.  Also, you'd no longer need to split the
--nacl-gdb arg on spaces, so this would be more consistent with the Windows code
path.  You're already providing nacl64-gdb; you could provide example wrapper
scripts with it, such as gdb-gnome-terminal.

This might not work because the gnome-terminal command sometimes returns
immediately.

https://chromiumcodereview.appspot.com/9950055/diff/5020/chrome/browser/nacl_...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/9950055/diff/5020/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:461: int fd_write_;
Please comment here that fd_write_ is not used here, but we hold it open so that
gdb can write to it via /proc/PID/fd/N.

[halyavin, 2012-04-04 16:08:51]

https://chromiumcodereview.appspot.com/9950055/diff/5020/chrome/browser/nacl_...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/9950055/diff/5020/chrome/browser/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:461: int fd_write_;
On 2012/04/04 15:46:13, Mark Seaborn wrote:
> Please comment here that fd_write_ is not used here, but we hold it open so
that
> gdb can write to it via /proc/PID/fd/N.

Done.

[Mark Seaborn, 2012-04-04 16:30:31]

On 2012/04/04 16:08:51, halyavin wrote:
>
https://chromiumcodereview.appspot.com/9950055/diff/5020/chrome/browser/nacl_...
> File chrome/browser/nacl_host/nacl_process_host.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9950055/diff/5020/chrome/browser/nacl_...
> chrome/browser/nacl_host/nacl_process_host.cc:461: int fd_write_;
> On 2012/04/04 15:46:13, Mark Seaborn wrote:
> > Please comment here that fd_write_ is not used here, but we hold it
> > open so that gdb can write to it via /proc/PID/fd/N.
> 
> Done.

OK, but the comments still don't explain why you use PID!="self" in
/proc/PID/fd/N.  Please add the explanation about terminals.

Mark

[halyavin, 2012-04-05 10:48:44]

> OK, but the comments still don't explain why you use PID!="self" in
> /proc/PID/fd/N.  Please add the explanation about terminals.
> 
> Mark

Done

[Mark Seaborn, 2012-04-05 15:03:12]

LGTM, thanks

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:497: // Tell debugger to send a
byte to write end of the pipe.
'the debugger'; 'to write to the end'?

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:498: // We use file descriptor in
our process because debugger will be typically
'a file descriptor'; 'the debugger'

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:506: // If debugger crashes before
attaching to the NaCl process, user can release
'the debugger'; 'the user'

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:507: // resources by terminating
NaCl loader in Chrome Task Manager.
'the NaCl loader'

[halyavin, 2012-04-05 15:07:32]

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:497: // Tell debugger to send a
byte to write end of the pipe.
On 2012/04/05 15:03:12, Mark Seaborn wrote:
> 'the debugger'; 'to write to the end'?

Done.

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:498: // We use file descriptor in
our process because debugger will be typically
On 2012/04/05 15:03:12, Mark Seaborn wrote:
> 'a file descriptor'; 'the debugger'

Done.

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:506: // If debugger crashes before
attaching to the NaCl process, user can release
On 2012/04/05 15:03:12, Mark Seaborn wrote:
> 'the debugger'; 'the user'

Done.

https://chromiumcodereview.appspot.com/9950055/diff/17001/chrome/browser/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:507: // resources by terminating
NaCl loader in Chrome Task Manager.
On 2012/04/05 15:03:12, Mark Seaborn wrote:
> 'the NaCl loader'

Done.

[commit-bot: I haz the power, 2012-04-05 15:07:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/halyavin@google.com/9950055/20003

[commit-bot: I haz the power, 2012-04-05 18:18:26]

Change committed as 130972