Windows improvements #225

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

@denis-sh
Contributor
  • core.sys.windows.windows: add nothrow, pure, and some functions (already merged: 2886846)
    • mark all WinAPI functions as nothrow
    • mark all extern(D) macros-functions as pure
    • add WinAPI functions from std.process
  • core.sys.windows.windows: add loadDbgHelpDll function
    This function will try to load dbghelp.dll from VS common folder instead of outdated Windows XP's dbghelp.dll or if there is no dbghelp.dll found by LoadLibrary.

Why second commit? Because it's a common situation and it took a lot of my time to understand that stack trace printing is really working but require more recent dbghelp.dll (my path was Issue 1001 -> comment 33 -> D 2.0 Stacktrace -> comment at the bottom of the page).

@MartinNowak MartinNowak commented on an outdated diff May 22, 2012
src/core/sys/windows/windows.d
@@ -25,6 +25,7 @@ extern (Windows)
alias UCHAR *PUCHAR;
alias char *PSZ;
alias wchar WCHAR;
+ alias WCHAR* LPWCH;
@MartinNowak
MartinNowak May 22, 2012 Member

Could you move that alias down to the other ones.
Eventually we should have something like this.

alias WCHAR* LPWCH, LPWSTR;
alias const(WCHAR)* LPCWCH, LPCWSTR;

alias WCHAR* PWCH, PWSTR;
alias const(WCHAR)* PCWCH, PCWSTR;
@MartinNowak MartinNowak and 1 other commented on an outdated diff May 22, 2012
src/core/sys/windows/windows.d
@@ -2450,6 +2452,10 @@ export
HWND GetFocus();
}
+export const(WCHAR)* GetEnvironmentStringsW();
+export BOOL FreeEnvironmentStringsW(const(WCHAR)* lpszEnvironmentBlock);
@MartinNowak
MartinNowak May 22, 2012 Member

Please use LPWCH instead of const(WCHAR)*.

@denis-sh
denis-sh May 22, 2012 Contributor

Done. It's now like in WinAPI header file. But according to [MSDN](http://msdn.microsoft.com/en-us/library/windows/desktop/ms683187(v=vs.85\).aspx):

Treat this memory as read-only; do not modify it directly.

Why not to mark this const?

@MartinNowak
MartinNowak May 22, 2012 Member

OK, that sounds reasonable but please add a short comment then.

@MartinNowak MartinNowak and 1 other commented on an outdated diff May 22, 2012
src/core/sys/windows/windows.d
@@ -2450,6 +2452,10 @@ export
HWND GetFocus();
}
+export const(WCHAR)* GetEnvironmentStringsW();
+export BOOL FreeEnvironmentStringsW(const(WCHAR)* lpszEnvironmentBlock);
+export DWORD GetEnvironmentVariableW(LPCWSTR lpName, LPWSTR lpBuffer, DWORD nSize);
+export BOOL SetEnvironmentVariableW(LPCWSTR lpName, LPCWSTR lpValue);
@MartinNowak
MartinNowak May 22, 2012 Member

It would be nice to have declarations for the ASCII functions too.

@denis-sh
denis-sh May 22, 2012 Contributor

Why?

@MartinNowak
MartinNowak May 22, 2012 Member

Even better.

@MartinNowak MartinNowak commented on an outdated diff May 22, 2012
src/core/sys/windows/windows.d
- alias const(WCHAR)* LPCWSTR, PCWSTR;
+ alias CHAR* LPTCH, LPTSTR, PTCH, PTSTR;
+ alias const(CHAR)* LPCTCH, LPCTSTR, PCTCH, PCTSTR;
@MartinNowak
MartinNowak May 22, 2012 Member

Let's leave it like that so we don't break client code but I just added a bug report for this.

@DmitryOlshansky
Member

I'm not sure about marking WinAPI functions nothrow. Since D uses SEH on Windows and Windows itself uses it in case of e.g. segfault and such. You surely see where this is going :)

@klickverbot
Member

@blackwhale: OTOH, SEH exceptions are translated into Errors anyway, so this shouldn't be a problem w.r.t. nothrow

@DmitryOlshansky
Member

@klickverbot Right. I always forget that Errors could be thrown even in nothrow code.

@klickverbot
Member

@blackwhale: I must admit that I'm not quite sure where we are heading with this, though – if we deem all SEH exceptions to be non-catchable (because the compiler is allowed to omit EH codegen for nothrow try blocks, AFAIK), the question is why we are translating them to D exceptions in the first place…

@DmitryOlshansky
Member

the compiler is allowed to omit EH codegen for nothrow try blocks, AFAIK.
This all smells quite bad. It's only because of Errors being rare and generally fatal. But damn it they are still catchable!

@MartinNowak
Member

Let's not have this discussion again.
It's not efficiently possible to use exception handling for asynchronous exceptions. For sure some things need to improve w.r.t. throwing Errors.

the question is why we are translating them to D exceptions in the first place

I don't really know and it really has wrong semantics (runtime teardown is executed) but apparantly it allows to reuse some stack tracing mechanism.

@denis-sh
Contributor

So is http://dlang.org/errors.html completely incorrect?!

@MartinNowak
Member

I've filed two bugs (8135, 8136).

@denis-sh denis-sh core.sys.windows.windows: add loadDbgHelpDll function
This function will try to load dbghelp.dll from VS common folder instead of outdated Windows XP's dbghelp.dll or if there is no dbghelp.dll found by LoadLibrary.
127361c
@complexmath
Member

SEH exceptions are translated into thrown exceptions in C++/Windows as well, and I think this is why it was done for D. But I do think the inconsistent behavior between Windows and Posix isn't ideal. Either way, it's pretty well established that Errors should not be recoverable, so throwing on an SEH exception is really just a means of generating a stack trace and allowing the user to log the error or whatever. An alternative would be to add some method to core.exception where the SEH adaptor could be plugged in by the user at run-time, and we could provide an equivalent adaptor for Posix as well (while it's technically illegal to throw from a signal handler, it does work on some platforms).

@MartinNowak MartinNowak commented on an outdated diff Jun 2, 2012
src/core/sys/windows/dbghelp.d
- return &sm_inst;
+ wchar[MAX_PATH] windir = void, path = void;
+ size_t len = GetEnvironmentVariableW("windir", windir.ptr, windir.length);
+ GetModuleFileNameW(sm_hndl, path.ptr, path.length);
+ if(path[0 .. len] != windir[0 .. len] || path[len + 1 .. len + 9] != "system32")
+ return; // dbghelp.dll isn't loaded from %windir%\system32 folder
@MartinNowak
MartinNowak Jun 2, 2012 Member

You missed to check the return value of GetModuleFileNameW and risk a buffer overflow.

@MartinNowak MartinNowak commented on the diff Jun 2, 2012
src/core/sys/windows/dbghelp.d
+ if(len >= 13 && p[0 .. 2] == "VS")
+ {
+ p += 2, len -= 2;
+ while(len && cast(uint)(*p - '0') <= 9)
+ ++p, --len;
+ if(len > 10 && p[0 .. 10] == "COMNTOOLS=")
+ {
+ wchar[MAX_PATH + 50] buff = void;
+ p += 10, len -= 10;
+ buff[0 .. len] = p[0 .. len];
+ buff[len .. len + 15] = `..\IDE\dbghelp` "\0";
+
+ if(auto newHndl = LoadLibraryW(buff.ptr))
+ {
+ if(sm_hndl)
+ FreeLibrary(sm_hndl);
@MartinNowak
MartinNowak Jun 2, 2012 Member

Do you think there might arise issues when both versions are loaded into the process.
They could probably hold on the same resources and deadlock.

@denis-sh
denis-sh Jun 2, 2012 Contributor

I don't think so. DllMain isn't a place for such complicated tasks. That's why there is SymInitialize.
And it works fine on my PC.

@MartinNowak
Member

Do you have a clue what's causing issues with the pre-installed dbghelp.dll?
Is it the missing symbol server?
http://msdn.microsoft.com/en-us/library/windows/desktop/ms679294(v=vs.85).aspx

I have some reservations towards adding a pure development tool to the runtime.
It will make a program behave different on a dev PC vs. an end-user PC without any notice.
Microsoft recommends to distribute dbghelp along with the executable.
http://msdn.microsoft.com/en-us/library/windows/desktop/ff805116(v=vs.85).aspx

@denis-sh
Contributor
denis-sh commented Jun 2, 2012

Do you have a clue what's causing issues with the pre-installed dbghelp.dll?

No. But I think if a person who developed this module stated that it doesn't work with old dbghelp.dll it's really complicated task to make it working with it.

I have some reservations towards adding a pure development tool to the runtime.

Me too. But we have the only runtime (e.g. there are MSVCRT.DLL and MSVCRTD.DLL for debugging) and lack of stacktrace is really annoying.

It will make a program behave different on a dev PC vs. an end-user PC without any notice.

Yes, so a notice must be added! But... too late, it already works such way, so this doesn't change the situation.

Microsoft recommends to distribute dbghelp along with the executable.

And this should be in our notice about debugging D application once somebody will write it.

@MartinNowak
Member

What am I expected to get?
I've compiled the binary with dmd -g -debug and the ran cv2pdb (0.24) on the executable.

dbghelp (5.1.2600.5512) from XP - without patch

std.conv.ConvException@d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(1585): Unexpected 'a' when converting from type string to type uint
----------------
d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(67121): std
d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(273): std
d:\Code\D\bt.d(5): D main
----------------

dbghelp (6.12.2.633) from VC10 - with patch

std.conv.ConvException@d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(1585): Unexpected 'a' when converting from type string to type uint
----------------
d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(67121): stdconvtoImpl!(uint, immutable(char)[])toImpl
d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(273): stdconvto!(uint)to!(immutable(char)[])to
d:\Code\D\bt.d(5): D main
----------------

It seems that the old version is only choking on the module separator.
Why are the symbols already demangled?

@denis-sh
Contributor
denis-sh commented Jun 2, 2012

Why are the symbols already demangled?

Because of cv2pdb. Everything works fine without it (just use Mago debugger).

@ghost
ghost commented Jun 2, 2012

Why not try to load it from C:\Program Files\Debugging Tools for Windows (x86) ? It's simpler to install the tools than an entire IDE.

@denis-sh
Contributor
denis-sh commented Jun 2, 2012

Why not try to load it from C:\Program Files\Debugging Tools for Windows (x86) ? It's simpler to install the tools than an entire IDE.

Of course! If it will be merged, everyone can improve it by adding another search path for tools he is using with a pull.

@MartinNowak
Member

I've rebuild cv2pdb and disabled the dotReplacement in symutil.cpp.
That gives me a clean stacktrace with my default XP dbghelp.

std.conv.ConvException@d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(1585): Unexpected 'a' when converting from type string to type uint
----------------
d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(67121): std.conv.toImpl!(uint,string).toImpl
d:\Code\D\DPL\dmd\bin\..\..\phobos\std\conv.d(273): std.conv.to!(uint).to!(string).to
d:\Code\D\bt.d(5): D main
----------------
@ghost
ghost commented Jun 2, 2012

I've never managed to get a stacktrace on XP. It always prints out addresses, even on a fresh XP installation (-g -debug). Not even replacing dbghelp.dll helps (after removing it from dllcache to avoid WFP). I've yet to try this pull, maybe it works now..

@MartinNowak
Member

@AndrejMitrovic

I've never managed to get a stacktrace on XP.

You need to convert the debug information format using cv2pdb.

Not even replacing dbghelp.dll helps

This is explicitly not recommended, see [caution](http://msdn.microsoft.com/en-us/library/windows/desktop/ms679294(v=vs.85\).aspx).

@denis-sh

Do you have test case that needs an updated dbghelp.dll.
If it's only an issue with the symbol names we should fix cv2pdb instead.

@denis-sh
Contributor
denis-sh commented Jun 4, 2012

Do you have test case that needs an updated dbghelp.dll.
If it's only an issue with the symbol names we should fix cv2pdb instead.

I don't use cv2pdb, I use Mago debugger.

With cv2pdb from Visual D 0.3.32:

void g(int a) { assert(a == 1); }
void f() { g(3); }
void main() { f(); }
  • no cv2pdb
    old dbghelp.dll: Addresses only;
    new dbghelp.dll:
main.d(1): void main.g(int)
main.d(2): void main.f()
main.d(3): _Dmain
  • with cv2pdb
    old dbghelp.dll:
main.d(1): main
main.d(2): main
main.d(3): D main

new dbghelp.dll:

main.d(1): maing
main.d(2): mainf
main.d(3): D main

Patch just loads new dbghelp.dll.

@ghost
ghost commented Jun 4, 2012

@dawgfoto

You need to convert the debug information format using cv2pdb.

Yes that's a workaround. But I want to see a stacktrace without having to call cv2pdb.

@MartinNowak
Member

I digged a little deeper into this and found that most issues with the stack tracing are self-caused.
The only thing I still need to sort out is getting SymGetLineFromAddr64 to work with XP's dbghelp.dll.

For now this will print symbolic stack traces using CodeView and an old dbghelp.
https://github.com/dawgfoto/druntime/commits/StackTrace

Probably we can close this pull request then. If all we'd get from loading a VC dbghelp is line number it might not be worth the hassle.

@denis-sh
Contributor

Thanks a lot, @dawgfoto! I hope your changes will be merged soon.

@denis-sh denis-sh closed this Jun 14, 2012
@MartinNowak MartinNowak referenced this pull request Jun 19, 2012
Merged

Windows stacktrace #243

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment