add windbg smarts to mozdebug

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 years ago
No description provided.
Assignee

Comment 1

4 years ago
Joel for general mozdebug stuff, Aaron for the Windows specific bits.  In
particular, I didn't know whether windbg could be installed as a part of
different SDKs, so I just had us look for multiple ones.  If there's a windbg64
(?), we could add support for that in a different bug...
Attachment #8704232 - Flags: review?(jmaher)
Attachment #8704232 - Flags: review?(aklotz)
Comment on attachment 8704232 [details] [diff] [review]
add windbg smarts to mozdebug

Review of attachment 8704232 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Yes, multiple versions of WinDbg can be installed with multiple SDK versions.

Back in the day, the standalone MSIs used to install to a different path, but that has probably changed. You could certainly grab http://people.mozilla.org/~aklotz/windbg32.msi and see if it installs to a different location, but you'll need to do it on a machine without the Windows SDK -- the Windows Installer is smart enough to know when a specific version of windbg components are already present.

The 64-bit windbg has the same executable name, it's just in a different location on disk (the x64 subdirectory instead of x86). I agree with filing a separate bug for that -- I think we'd need to add some smarts to know whether we're running a 64-bit build and then adjust the path accordingly.

::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
@@ +59,5 @@
>  }
>  
>  # Maps each OS platform to the preferred debugger programs found in _DEBUGGER_INFO.
>  _DEBUGGER_PRIORITIES = {
>        'win': ['devenv.exe', 'wdexpress.exe'],

It looks like you should add windbg to this priority list. IMHO windbg should be the highest priority.

@@ +69,5 @@
>  
> +def _windbg_installation_paths():
> +    programFilesSuffixes = ['', ' (x86)']
> +    programFiles = "C:/Program Files"
> +    windowsKitsVersions = ['8', '8.1', '10']

I think we should put the entries in this list in descending order. Windbg 10 has some significant improvements so we should always yield that path first to ensure that it has precedence.

@@ +77,5 @@
> +                                         'Windows Kits')
> +        for version in windowsKitsVersions:
> +            yield os.path.join(windowsKitsPrefix, version,
> +                               'Debuggers', 'x86', 'windbg.exe')
> +    

Nit: whitespace
Attachment #8704232 - Flags: review?(aklotz) → review+
You could import mozinfo and check `mozinfo.bits == 64`, although that doesn't do exactly what you want if someone hasn't already loaded mozinfo.json for you, like:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#531
Assignee

Comment 4

4 years ago
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #2)
> This looks good. Yes, multiple versions of WinDbg can be installed with
> multiple SDK versions.

Thanks for the comments!

> Back in the day, the standalone MSIs used to install to a different path,
> but that has probably changed. You could certainly grab
> http://people.mozilla.org/~aklotz/windbg32.msi and see if it installs to a
> different location, but you'll need to do it on a machine without the
> Windows SDK -- the Windows Installer is smart enough to know when a specific
> version of windbg components are already present.

I think I used that msi to install it on my machine, so that's good.  Alternatively, we can announce and then see if people complain that windbg isn't being found.  (The "works on my machine" approach.)

> The 64-bit windbg has the same executable name, it's just in a different
> location on disk (the x64 subdirectory instead of x86). I agree with filing
> a separate bug for that -- I think we'd need to add some smarts to know
> whether we're running a 64-bit build and then adjust the path accordingly.

Great.

> ::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
> @@ +59,5 @@
> >  }
> >  
> >  # Maps each OS platform to the preferred debugger programs found in _DEBUGGER_INFO.
> >  _DEBUGGER_PRIORITIES = {
> >        'win': ['devenv.exe', 'wdexpress.exe'],
> 
> It looks like you should add windbg to this priority list. IMHO windbg
> should be the highest priority.

I thought about that, but wasn't completely sure that was the right decision (i.e. if somebody blindly installed the entire SDK, and then ran with just --debugger, I'd bet that windbg is not what they would expect).  So I left it out.

> @@ +69,5 @@
> >  
> > +def _windbg_installation_paths():
> > +    programFilesSuffixes = ['', ' (x86)']
> > +    programFiles = "C:/Program Files"
> > +    windowsKitsVersions = ['8', '8.1', '10']
> 
> I think we should put the entries in this list in descending order. Windbg
> 10 has some significant improvements so we should always yield that path
> first to ensure that it has precedence.

Good point, fixed.
Comment on attachment 8704232 [details] [diff] [review]
add windbg smarts to mozdebug

Review of attachment 8704232 [details] [diff] [review]:
-----------------------------------------------------------------

Please do let me know if there is no more integrated way to find the path and I can r+.

::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
@@ +113,5 @@
> +    if not debuggerPath and debugger == 'windbg.exe':
> +        for candidate in _windbg_installation_paths():
> +            if os.path.exists(candidate):
> +                debuggerPath = candidate
> +                break

this whole block seems like a special case to me.  If the user specifies --debugger=windbg, why wouldn't we catch the executable in find_executable()?  I would think we could add the call to _windbg_installation_paths() in find_executable() or not make it a full block outside of the normal setup.

I will give props for a nice comment.
Attachment #8704232 - Flags: review?(jmaher) → review-
Assignee

Comment 6

4 years ago
(In reply to Joel Maher (:jmaher) from comment #5)
> Comment on attachment 8704232 [details] [diff] [review]
> add windbg smarts to mozdebug
> 
> Review of attachment 8704232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please do let me know if there is no more integrated way to find the path
> and I can r+.
> 
> ::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
> @@ +113,5 @@
> > +    if not debuggerPath and debugger == 'windbg.exe':
> > +        for candidate in _windbg_installation_paths():
> > +            if os.path.exists(candidate):
> > +                debuggerPath = candidate
> > +                break
> 
> this whole block seems like a special case to me.  If the user specifies
> --debugger=windbg, why wouldn't we catch the executable in
> find_executable()?  I would think we could add the call to
> _windbg_installation_paths() in find_executable() or not make it a full
> block outside of the normal setup.

windbg doesn't get added to $PATH by default, and I doubt that many people put windbg on the $PATH (I'm not even sure I know how).  find_executable only looks at $PATH by default:

https://hg.python.org/cpython/file/tip/Lib/distutils/spawn.py#l169

And we can't modify find_executable, as it's distributed as part of Python.

I guess we could pass in the potential install locations as a second call to find_executable, or we could just keep the current code.  Which do you prefer?
Flags: needinfo?(jmaher)
ok, lets keep the current code, for some reason I thought find_executable was our own stuff.
Flags: needinfo?(jmaher)
Comment on attachment 8704232 [details] [diff] [review]
add windbg smarts to mozdebug

Review of attachment 8704232 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
@@ +68,5 @@
>  }
>  
> +def _windbg_installation_paths():
> +    programFilesSuffixes = ['', ' (x86)']
> +    programFiles = "C:/Program Files"

Ideally one of the magic environment variables (https://technet.microsoft.com/en-us/library/cc749104%28v=ws.10%29.aspx) would be used for locating Program Files.

Comment 10

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36c1e4eb0c91
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.