Use irt from within chrome install

Use irt from within chrome install


BUG=http://code.google.com/p/chromium/issues/detail?id=140091

Committed: https://code.google.com/p/nativeclient-sdk/source/detail?r=1413

[Sam Clegg, 2012-09-17 17:41:09]

Query the host OS to determine which IRT to use.
On windows we always use 64-bit IRT on 64-bit OS.

[binji, 2012-09-17 17:52:06]

lgtm, just a few questions/comments.

http://codereview.chromium.org/10915292/diff/1/.gitignore
File .gitignore (right):

http://codereview.chromium.org/10915292/diff/1/.gitignore#newcode37
.gitignore:37: /visual_studio/NativeClientVSAddIn/NaCl.Build.CPPTasks/obj/
yuck... is there any way to change the VS output directory for the add-in to not
spray the source directory with output?

http://codereview.chromium.org/10915292/diff/1/visual_studio/NativeClientVSAd...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs
(right):

http://codereview.chromium.org/10915292/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:67:
string file_version = version_info.FileVersion;
It seems GetVersionInfo can return a FileVersionInfo with null FileVersion; you
should probably check for it.

see
http://msdn.microsoft.com/en-us/library/system.diagnostics.fileversioninfo.ge...

http://codereview.chromium.org/10915292/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:68:
irtPath_ = Path.Combine(Path.GetDirectoryName(chrome_path), file_version,
"nacl_irt_" + arch + ".nexe");
nit: wrap at 100 chars (for C#)

http://codereview.chromium.org/10915292/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:181:
contents.AppendFormat("nacl-irt \"{0}\"\n", irtPathEscaped);
what happens when this command isn't run? Is it useful to start the debugger at
all?

http://codereview.chromium.org/10915292/diff/3001/visual_studio/NativeClientV...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PropertyManager.cs
(right):

http://codereview.chromium.org/10915292/diff/3001/visual_studio/NativeClientV...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PropertyManager.cs:206:
public string TargetArchitecture
Where is this set to its default?

[Sam Clegg, 2012-09-17 19:58:16]

http://codereview.chromium.org/10915292/diff/1/.gitignore
File .gitignore (right):

http://codereview.chromium.org/10915292/diff/1/.gitignore#newcode37
.gitignore:37: /visual_studio/NativeClientVSAddIn/NaCl.Build.CPPTasks/obj/
On 2012/09/17 17:52:06, binji wrote:
> yuck... is there any way to change the VS output directory for the add-in to
not
> spray the source directory with output?

This is kinda the way visual studio users expect things
to work.  We could probably override it but I'm not sure
its a good idea.  Noel, what do you think?

http://codereview.chromium.org/10915292/diff/1/visual_studio/NativeClientVSAd...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs
(right):

http://codereview.chromium.org/10915292/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:67:
string file_version = version_info.FileVersion;
On 2012/09/17 17:52:06, binji wrote:
> It seems GetVersionInfo can return a FileVersionInfo with null FileVersion;
you
> should probably check for it.
> 
> see
>
http://msdn.microsoft.com/en-us/library/system.diagnostics.fileversioninfo.ge...

Done.

http://codereview.chromium.org/10915292/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:68:
irtPath_ = Path.Combine(Path.GetDirectoryName(chrome_path), file_version,
"nacl_irt_" + arch + ".nexe");
On 2012/09/17 17:52:06, binji wrote:
> nit: wrap at 100 chars (for C#)

Done.

http://codereview.chromium.org/10915292/diff/1/visual_studio/NativeClientVSAd...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:181:
contents.AppendFormat("nacl-irt \"{0}\"\n", irtPathEscaped);
On 2012/09/17 17:52:06, binji wrote:
> what happens when this command isn't run? Is it useful to start the debugger
at
> all?

In this case yes, you only need to IRT if you want to have
symbols for the IRT itself.  Even if we can't find the IRT
we can still debug the application code.

However, there are fatal errors in this function and I
have not figured out how to abort the debugging session
from here.

http://codereview.chromium.org/10915292/diff/3001/visual_studio/NativeClientV...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PropertyManager.cs
(right):

http://codereview.chromium.org/10915292/diff/3001/visual_studio/NativeClientV...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PropertyManager.cs:206:
public string TargetArchitecture
On 2012/09/17 17:52:06, binji wrote:
> Where is this set to its default?

IIUC, the visual studio property files define this
property along with its default value (like for all
the other properties in this file).

[binji, 2012-09-17 21:54:06]

slgtm

http://codereview.chromium.org/10915292/diff/11/visual_studio/NativeClientVSA...
File visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs
(right):

http://codereview.chromium.org/10915292/diff/11/visual_studio/NativeClientVSA...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:72:
MessageBox.Show(string.Format("Debugging of {0} NaCl modules is not possible on
this system ({1}).",
nit: wrap at 100

http://codereview.chromium.org/10915292/diff/11/visual_studio/NativeClientVSA...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:76:
// check chrome version
Not necessary for this CL, but these checks feel like they should be separate
functions.

http://codereview.chromium.org/10915292/diff/11/visual_studio/NativeClientVSA...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:99:
// We look for the IRT in several ways, mimicing what chrome itself
nit: mimicking

http://codereview.chromium.org/10915292/diff/11/visual_studio/NativeClientVSA...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:109:
if (file_version == null)
nit: the comment above makes it seem like this check should be reversed.

http://codereview.chromium.org/10915292/diff/11/visual_studio/NativeClientVSA...
visual_studio/NativeClientVSAddIn/NativeClientVSAddIn/PluginDebuggerGDB.cs:111:
if (!File.Exists(irtPath_))
pull this common code out

[noelallen1, 2012-09-18 17:47:56]

LGTM same as above.