391848 - Windows stack walking code broken on builds compiled with VC8

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The Windows stack walking code works fine for me on builds compiled with VC 7.1, but it doesn't give a bunch of the desired information on builds compiled with VC8.

If I change the third parameter to SymInitialize to FALSE and remove nsTraceRefcnt::LoadLibrarySymbols, I can see that the calls to SymLoadModule64 are succeeding (and returning a good base address) the first time they're made for a given library.  (They're supposed to fail when called multiple times.)

But SymGetModuleInfo64 always fails; I'm not yet sure why, although GetLastError gives the vague "The parameter is incorrect." (which is also what I get for SymLoadModule64 when SymLoadModule64 has previously loaded the module).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think the problem is that the compiler is somehow coputing sizeof(IMAGEHLP_MODULE) incorrectly -- it's 584 with VC7.1 but 1664 with VC8.  So SymGetModuleInfo64 is failing because it's being given the wrong SizeOfStruct.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yep, hardcoding 584 for SizeOfStruct makes it work with VC8.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Series of patches to be attached; for clarity, I wrote comment 0 after writing some patches in my tree that I wasn't sure were helping; it turns out the patch was necessary to get to the less-broken-but-still-broken state I described in comment 0.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: some cleanups while debugging

This is just some cleanup: more error reporting, removal of an unneeded cast, and use of the correct constant.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: process handle consistency

Some of the debugging functions require a real process handle rather than the pseudo-handle returned by GetCurrentProcess().  Some of the require the same process handle to be passed every time.  This puts a real process handle in a global so we don't need to worry about the difference.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: dbghelp version compatibility

This patch makes things work on systems (e.g., my Windows XP SP2 install) that have a dbghelp.dll older than the one for which the compiler (e.g., VC8) ships headers.

r+a appreciated on all patches, if possible...

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 276297 [details] [diff] [review]
patch 2: process handle consistency

Never mind this one; I was either testing with/without backwards or the symptoms I thought it was fixing (errors from WalkStack64) are just random.

Comment 8

[Benjamin Smedberg]

Comment on attachment 276298 [details] [diff] [review]
patch 3: dbghelp version compatibility

I'm not so sure about this one: shouldn't we be special-casing this on the version of the Windows SDK we're including, not the version of MSVC?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't see any nice version defines to ifdef based off of -- I suppose I could ifdef based on the presence of a constant that's defined in the newer header but not the older one, like SSRVACTION_QUERYCANCEL.  Or do you know a way to get the version of the Platform SDK?

imagehlp.h defines an API_VERSION_NUMBER constant, but it's 9 in both the new and old versions.  (Also a MINIDUMP_VERSION constant that's the same in both.)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I could also pass a smaller SizeOfStruct number based on an offsetof calculation that works against both versions.  That seemed to work when I tested it, although I'm not sure there's any guarantee that it would keep working since it wasn't a size of the struct that they ever shipped.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: dbghelp version compatibility

This is another version of the patch you already reviewed that should work even when different platform SDK versions are used than the ones that came with the compiler; I ifdefed based on a constant that was added in the same version.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 276296 [details] [diff] [review]
patch 1: some cleanups while debugging

a=bzbarsky

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Patches checked in, 2007-08-19 14:37 -0700 and 14:38.

Comment 14

[Ronny Perinke]

(In reply to comment #4)
> Created an attachment (id=276296) [details]
> patch 1: some cleanups while debugging
> 
> This is just some cleanup: more error reporting, removal of an unneeded cast,
> and use of the correct constant.
> 

removing the explicit conversion of callbackEspecial to PENUMLOADED_MODULES_CALLBACK leads to build errors with VC++ 2005 EE SP1 (and Vista SDK).

################
/cygdrive/i/building_Mozilla/source/TRUNK/mozilla/xpcom/base/nsStackWalk.cpp
nsStackWalk.cpp
i:/building_Mozilla/source/TRUNK/mozilla/xpcom/base/nsStackWalk.cpp(775) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK,PVOID)': Konvertierung des Parameters 2 von 'overloaded-function' in 'PENUMLOADED_MODULES_CALLBACK' nicht möglich
        Keine Funktion mit diesem Namen im Gültigkeitsbereich stimmt mit dem Zieltyp überein
i:/building_Mozilla/source/TRUNK/mozilla/xpcom/base/nsStackWalk.cpp(842) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID)': Konvertierung des Parameters 2 von 'overloaded-function' in 'PENUMLOADED_MODULES_CALLBACK64' nicht möglich
        Keine Funktion mit diesem Namen im Gültigkeitsbereich stimmt mit dem Zieltyp überein
i:/building_Mozilla/source/TRUNK/mozilla/xpcom/base/nsStackWalk.cpp(958) : warning C4244: '=': Konvertierung von 'DWORD64' in 'unsigned long', mէlicher Datenverlust
i:/building_Mozilla/source/TRUNK/mozilla/xpcom/base/nsStackWalk.cpp(1010) : warning C4244: '=': Konvertierung von 'DWORD64' in 'unsigned long', mէlicher Datenverlust
make[5]: *** [nsStackWalk.obj] Error 2
make[5]: Leaving directory `/cygdrive/i/building_Mozilla/source/TRUNK/firefox_vc8/xpcom/base'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/i/building_Mozilla/source/TRUNK/firefox_vc8/xpcom'
make[3]: *** [libs_tier_xpcom] Error 2
make[3]: Leaving directory `/cygdrive/i/building_Mozilla/source/TRUNK/firefox_vc8'
make[2]: *** [tier_xpcom] Error 2
make[2]: Leaving directory `/cygdrive/i/building_Mozilla/source/TRUNK/firefox_vc8'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/cygdrive/i/building_Mozilla/source/TRUNK/firefox_vc8'
make: *** [build] Error 2
#####################
in english: cannot convert parameter 2 from 'overloaded-function' to 'PENUMLOADED_MODULES_CALLBACK'
no function in scope with that name and target type

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What type is in the headers you have?  And how does that differ from the actual type of the function?

Comment 16

[atzaus]

I am also getting the same error #14 with Windows SDK 6.0A (W2K3, W2K3R2, Vista) and 6.1 (W2K8 SDK)

nsStackWalk.cpp
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(775) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK,PVOID)' : cannot convert parameter 2 from 'overloaded-function' to 'PE NUMLOADED_MODULES_CALLBACK'
        None of the functions with this name in scope match the target type
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(842) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID)' : cannot convert parameter 2 from 'overloaded-function' to 'PENUMLOADED_MODULES_CALLBACK64'
        None of the functions with this name in scope match the target type
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(958) : warning C4244: '=' : conversion from
 'DWORD64' to 'unsigned long', possible loss of data
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(1010) : warning C4244: '=' : conversion from 'DWORD64' to 'unsigned long', possible loss of data
make[5]: *** [nsStackWalk.obj] Error 2
make[5]: Leaving directory `/e/BuildDir/mozilla/obj-firefox/xpcom/base'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/e/BuildDir/mozilla/obj-firefox/xpcom'
make[3]: *** [libs_tier_xpcom] Error 2
make[3]: Leaving directory `/e/BuildDir/mozilla/obj-firefox'
make[2]: *** [tier_xpcom] Error 2
make[2]: Leaving directory `/e/BuildDir/mozilla/obj-firefox'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/e/BuildDir/mozilla/obj-firefox'
make: *** [build] Error 2

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible fix for compilation error

Could you test this and see if it fixes the error?

Comment 18

[atzaus]

Its still a no go...
nsStackWalk.cpp
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(785) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK,PVOID)' : cannot convert parameter 2 from 'overloaded-function' to 'PENUMLOADED_MODULES_CALLBACK'
        None of the functions with this name in scope match the target type
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(852) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID)' : cannot convert parameter 2 from 'overloaded-function' to 'PENUMLOADED_MODULES_CALLBACK64'
        None of the functions with this name in scope match the target type
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(968) : warning C4244: '=' : conversion from
 'DWORD64' to 'unsigned long', possible loss of data
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(1020) : warning C4244: '=' : conversion from 'DWORD64' to 'unsigned long', possible loss of data
make[5]: *** [nsStackWalk.obj] Error 2

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: another possible fix for compilation error

What about this?  (Or, if it changes the error message but still fails, what about both at the same time?)

Comment 20

[atzaus]

After only patch 2
nsStackWalk.cpp
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(775) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK,PVOID)' : cannot convert parameter 2 from 'BOOL (__stdcall *)(LPSTR,DWORD,ULONG,PVOID)' to
 'PENUMLOADED_MODULES_CALLBACK'
        This conversion requires a reinterpret_cast, a C-style cast or function-style cast
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(842) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID)' : cannot convert parameter 2 from 'BOOL (__stdcall *)(PTSTR,DWORD64,ULONG,PVOID)' to 'PENUMLOADED_MODULES_CALLBACK64'
        This conversion requires a reinterpret_cast, a C-style cast or function-style cast
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(958) : warning C4244: '=' : conversion from 'DWORD64'
 to 'unsigned long', possible loss of data
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(1010) : warning C4244: '=' : conversion from 'DWORD64
' to 'unsigned long', possible loss of data
make[5]: *** [nsStackWalk.obj] Error 2

Patch2 then patch 1
nsStackWalk.cpp
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(785) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK,PVOID)' : cannot convert parameter 2 from 'BOOL (__stdcall *)(LPSTR,DWORD,ULONG,PVOID)' to  'PENUMLOADED_MODULES_CALLBACK'
        This conversion requires a reinterpret_cast, a C-style cast or function-style cast
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(852) : error C2664: 'BOOL (HANDLE,PENUMLOADED_MODULES_CALLBACK64,PVOID)' : cannot convert parameter 2 from 'BOOL (__stdcall *)(PTSTR,DWORD64,ULONG,PVOID)' to 'PENUMLOADED_MODULES_CALLBACK64'
        This conversion requires a reinterpret_cast, a C-style cast or function-style cast
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(968) : warning C4244: '=' : conversion from 'DWORD64' to 'unsigned long', possible loss of data
e:/BuildDir/mozilla/xpcom/base/nsStackWalk.cpp(1020) : warning C4244: '=' : conversion from 'DWORD64
' to 'unsigned long', possible loss of data
make[5]: *** [nsStackWalk.obj] Error 2

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, I backed out the part of the patch that caused that error, although I'm a little skeptical of whether the code will actually work with that compiler.

Comment 22

[atzaus]

It (nsStackwalk.cpp) compiles now...2 errors down and 1 more to go to make firefox compile again with VC9

Comment 23

[Ronny Perinke]

(In reply to comment #21)
> OK, I backed out the part of the patch that caused that error, although I'm a
> little skeptical of whether the code will actually work with that compiler.
> 

I've tested the Windows 2003 R2 SDK which works well without the cast to PENUMLOADED_MODULES_CALLBACK. But the Vista SDK (http://www.microsoft.com/downloads/details.aspx?familyid=C2B1E300-F358-4523-B479-F53D234CDCCF&displaylang=en) does not. Both compiled with VC8 (VC++ 2005 Express Edition).
I think you can ifdef for _INC_SDKDDKVER to check if it's SDK6. It's defined in sdkddkver.h, but SDK5 (Win 2003) has no such file and it's not defined elsewhere too.

I'm downloading the Windows Server 2008 SDK preview right now, to check if it also includes sdkddkver.h.

Comment 24

[Ronny Perinke]

(In reply to comment #23)
> I'm downloading the Windows Server 2008 SDK preview right now, to check if it
> also includes sdkddkver.h.
> 

it does and ifdef'ing worked with SDK6 (I didn't expected anything else)