Re: Re: Proposed Windows-specific change: Enable crash dumps (like core files)

On Fri, Dec 17, 2010 at 17:24, Craig Ringer <craig@postnewspapers.com.au> wrote:
> On 17/12/2010 7:17 PM, Magnus Hagander wrote:
>
>> What version of dbghelp do you have?
>
> 6.1.7600.16385 by default, as shipped in Windows 7 32-bit, and what I was
> testing with.
>
> 6.12.0002.633 is what came with my copy of Debugging Tools for windows, from
> the windows SDK. The same version comes with Visual Studio 10 Express.
>
> 6.9.0003.113 is what came with Visual Studio 9 Express.
>
> I've now re-tested with the latest, 6.12.0002.633, and have the same result.
>
>> And what does the API report for it?
>
> This:
>
> version = (*pApiVersion)();
> write_stderr("version: %hu.%hu.%hu\n",
>        version->MajorVersion,
>        version->MinorVersion,
>        version->Revision);
>
> outputs "version: 4.0.5" when loading dbghelp.dll 6.12.0002.633 from
> c:\postgres91\bin\dbghelp.dll as verified by a write_stderr() just before
> LoadLibrary and a test verifying the LoadLibrary worked.

Now, that's annoying. So clearly we can't use that function to
determine which version we're on. Seems it only works for "image help
api", and not the general thing.

According to http://msdn.microsoft.com/en-us/library/ms679294(v=vs.85).aspx,
we could look for:

SysEnumLines - if present, we have at least 6.1.

However, I don't see any function that appeared in 6.0 only..

We're probably better off looking at the VERSIONINFO structure. Which
has a not-so-nice API, but at least it's documented :)


> Shouldn't dbghelp.dll just ignore any flags it doesn't understand? I'm
> surprised we can't just pass flags a particular version doesn't know about
> and have it deal with them. Still, I guess it's not wise to assume such
> things.

You'd think so, but if I include all the flags and run it with the
default one on Windows 2003, it comes back with the infamous
"Parameter Is Incorrect" error.



>> The code is supposed to add the private memory if it finds version
>> 6 or higher, perhaps that check is incorrect?
>
> It begins to look like the version check might be lying. I'll dig further in
> a bit; it's 12:20am now and I need to sleep.
>
> I'm going on holiday in about 48 hours, and will be away from Windows
> (phew!) unless I get a VM set up during that time. I'll see what I can do.

I'll try to put together a version that uses the VERSIONINFO to
determine it, for you to test.


>>> I'm happy with the patch as it stands, with the one exception that
>>> consideration of mingw is required before it can be committed.
>>
>> What do you mean? That it has to be tested on mingw specifically, or
>> something else?
>
> I'm not even sure it'll compile or link with MinGW, and if it does, I doubt
> it'll be useful. It should only be compiled and called for native VC++
> builds.

I'm pretty sure it could be useful with mingw as well. not *as*
useful, but still useful. You'd get proper stack traces and such for
anything in msvcrt, and the actual reason for the exception etc.


> I'm not sure if backend\port\win32 is built for all win32, or only for VC++.

It's for all win32.

>  Testing for defined(_MSC_VER) would do the trick. I'm not sure testing
> defined(WIN32_ONLY_COMPILER) is quite right, since I doubt dbghelp.dll would
> be much use for a Pg compiled with Borland or the like either if that were
> ever supported.

We don't support building the backend with borland - only libpq and psql.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/