Re: faultrep:display useful information in ReportFault
Jérôme Gardou a écrit :
This should help debugging applications which performs their own exception handling. After a bit of thinking, I think that this should be displayed each time the function is called. Is there any macro in wine/debug.h which would display a message, even when WINEDEBUG is set to "-all" ?
On Sun, Feb 22, 2009 at 8:58 AM, Jérôme Gardou <jerome.gardou(a)gmail.com> wrote:
Jérôme Gardou a écrit :
This should help debugging applications which performs their own exception handling. After a bit of thinking, I think that this should be displayed each time the function is called. Is there any macro in wine/debug.h which would display a message, even when WINEDEBUG is set to "-all" ?
MESSAGE() -- -Austin
On Sunday 22 February 2009 12:09:01 pm Austin English wrote:
On Sun, Feb 22, 2009 at 8:58 AM, Jérôme Gardou <jerome.gardou(a)gmail.com> wrote:
Jérôme Gardou a écrit :
This should help debugging applications which performs their own exception handling.
After a bit of thinking, I think that this should be displayed each time the function is called. Is there any macro in wine/debug.h which would display a message, even when WINEDEBUG is set to "-all" ?
MESSAGE()
Perhaps a MessageBox or some fancier dialog would be better. MSDN says it can behave differently depending on how the user has their system configured.. what kind of options are available?
Chris Robinson a écrit :
On Sunday 22 February 2009 12:09:01 pm Austin English wrote:
On Sun, Feb 22, 2009 at 8:58 AM, Jérôme Gardou <jerome.gardou(a)gmail.com>
wrote:
Jérôme Gardou a écrit :
This should help debugging applications which performs their own exception handling.
After a bit of thinking, I think that this should be displayed each time the function is called. Is there any macro in wine/debug.h which would display a message, even when WINEDEBUG is set to "-all" ?
MESSAGE()
Thanks !
Perhaps a MessageBox or some fancier dialog would be better. MSDN says it can behave differently depending on how the user has their system configured.. what kind of options are available?
The aim of this patch is to provide some information to wine developers when an application crashes because of an unexpected behaviour of the winapi implementation. In this case, the implementation is WINE, and fixmes/traces etc... are currently displayed in console. I'm not sure that we want to reproduce windows XP behaviour here (understand "Your application encountered an unexpected error. What do you want to do : send report/do not send) which annoys most people, and is not that useful. Most wine users, when their app crashes, open a bug report and attach the console log. If the log contains information got from ReportFault, developers will gain some time, and have the preceding context of the call. Just my 2 cents :-)
On Sun, Feb 22, 2009 at 1:34 PM, Jérôme Gardou <jerome.gardou(a)gmail.com>wrote:
... I'm not sure that we want to reproduce windows XP behaviour here (understand "Your application encountered an unexpected error. What do you want to do : send report/do not send) which annoys most people, and is not that useful. Most wine users, when their app crashes, open a bug report and attach the console log. If the log contains information got from ReportFault, developers will gain some time, and have the preceding context of the call.
Just my 2 cents :-)
I know quite a few people (my converts) that run applications (mostly games) where they launch the application from an icon and there is no console. They've complained to me every once in a while that they would much prefer a dialog informed them when their application crashes. Personally, I think that fatal errors (including things like "a DLL could not be found") should launch a dialog if a terminal is not available (if "TERM" is not set). Erich Hoover ehoover(a)mines.edu
Erich Hoover a écrit :
I know quite a few people (my converts) that run applications (mostly games) where they launch the application from an icon and there is no console. They've complained to me every once in a while that they would much prefer a dialog informed them when their application crashes. Personally, I think that fatal errors (including things like "a DLL could not be found") should launch a dialog if a terminal is not available (if "TERM" is not set).
Erich Hoover ehoover(a)mines.edu <mailto:ehoover(a)mines.edu>
Why not writing directly debug output in a file instead of redirecting it to stderr. After all, Xorg has its own log, why not /tmp/wine.0.log ? And ReportFault would be "Your application crashed because of unexpected behaviour. Please report a bug to http://bugs.winehq.org. Don't forget to attach /tmp/wine.0.log. Please be kind and search for existing bug before reporting."
On Sun, Feb 22, 2009 at 2:08 PM, Jérôme Gardou <jerome.gardou(a)gmail.com>wrote:
Erich Hoover a écrit :
I know quite a few people (my converts) that run applications (mostly games) where they launch the application from an icon and there is no console. They've complained to me every once in a while that they would much prefer a dialog informed them when their application crashes. Personally, I think that fatal errors (including things like "a DLL could not be found") should launch a dialog if a terminal is not available (if "TERM" is not set).
Erich Hoover ehoover(a)mines.edu <mailto:ehoover(a)mines.edu>
Why not writing directly debug output in a file instead of redirecting it to stderr. After all, Xorg has its own log, why not /tmp/wine.0.log ?
And ReportFault would be "Your application crashed because of unexpected behaviour. Please report a bug to http://bugs.winehq.org. Don't forget to attach /tmp/wine.0.log. Please be kind and search for existing bug before reporting."
For the people I know that sounds like it would work. Provided that that message appeared as a dialog (if run without a terminal) and they wouldn't need to do anything special to activate the feature then I think that would help a lot for the non-terminal-savvy folks. Erich Hoover ehoover(a)mines.edu
On Sunday 22 February 2009 1:15:42 pm Erich Hoover wrote:
On Sun, Feb 22, 2009 at 2:08 PM, Jérôme Gardou <jerome.gardou(a)gmail.com>wrote:
And ReportFault would be "Your application crashed because of unexpected behaviour. Please report a bug to http://bugs.winehq.org. Don't forget to attach /tmp/wine.0.log. Please be kind and search for existing bug before reporting."
For the people I know that sounds like it would work. Provided that that message appeared as a dialog (if run without a terminal) and they wouldn't need to do anything special to activate the feature then I think that would help a lot for the non-terminal-savvy folks.
Having the information there of why it crashed would be nice too, especially if it's hard to reproduce. Knowing what the fault was, and where (with a backtrace if possible) can give the more development-inclined people information of where to look for the issue, instead of needlessly trying to make it crash again or go digging around in log files.
Chris Robinson a écrit :
Having the information there of why it crashed would be nice too, especially if it's hard to reproduce. Knowing what the fault was, and where (with a backtrace if possible) can give the more development-inclined people information of where to look for the issue, instead of needlessly trying to make it crash again or go digging around in log files.
Well, that was the intent of my patch : ReportFault is made to give windows developers information about why the application catched an exception. There is no reason why wine people could not use it. By the way, is there an easy way to know if wine was launched from console or not?
On Sun, Feb 22, 2009 at 2:34 PM, Jérôme Gardou <jerome.gardou(a)gmail.com>wrote:
Chris Robinson a écrit :
Having the information there of why it crashed would be nice too, especially if it's hard to reproduce. Knowing what the fault was, and where (with a backtrace if possible) can give the more development-inclined people information of where to look for the issue, instead of needlessly trying to make it crash again or go digging around in log files.
Well, that was the intent of my patch : ReportFault is made to give windows developers information about why the application catched an exception. There is no reason why wine people could not use it. By the way, is there an easy way to know if wine was launched from console or not?
Wine may have a macro or function to check for you, but I am not aware of it. In any application you can check to see if getenv("TERM") is empty, if it has a value then the application was launched from a console. Erich Hoover ehoover(a)mines.edu
2009/2/23 Erich Hoover <ehoover(a)mines.edu>:
On Sun, Feb 22, 2009 at 2:34 PM, Jérôme Gardou <jerome.gardou(a)gmail.com> wrote:
Chris Robinson a écrit :
Having the information there of why it crashed would be nice too, especially if it's hard to reproduce. Knowing what the fault was, and where (with a backtrace if possible) can give the more development-inclined people information of where to look for the issue, instead of needlessly trying to make it crash again or go digging around in log files.
Well, that was the intent of my patch : ReportFault is made to give windows developers information about why the application catched an exception. There is no reason why wine people could not use it. By the way, is there an easy way to know if wine was launched from console or not?
Wine may have a macro or function to check for you, but I am not aware of it. In any application you can check to see if getenv("TERM") is empty, if it has a value then the application was launched from a console.
#include <unistd.h> and use isatty() would be the correct way to do it. But is a dialog box that only appears when not being run from a terminal really such a good idea?
On Sun, Feb 22, 2009 at 7:44 PM, Ben Klein <shacklein(a)gmail.com> wrote:
... #include <unistd.h> and use isatty() would be the correct way to do it. But is a dialog box that only appears when not being run from a terminal really such a good idea?
Does wine have a global handle to a file descriptor that can be used in a call to isatty()? My suggestion (maybe this wasn't clear) was to normally output the diagnostic information to the console, but if the console is not available (if wine was launched from a desktop icon, for example) then to pop up with a dialog. Erich Hoover ehoover(a)mines.edu
On Monday 23 February 2009 8:39:22 am Erich Hoover wrote:
My suggestion (maybe this wasn't clear) was to normally output the diagnostic information to the console, but if the console is not available (if wine was launched from a desktop icon, for example) then to pop up with a dialog.
That's what the isatty() function call is for, to tell if stderr is writing to a console or not. Though I'm not sure it'd be a big problem to output to the console and popup a message box, with an option to suppress the message box via a registry option, or something.
I've written a dialog about a crash some time ago to add to winedbg (http://www.winehq.org/pipermail/wine-patches/2009-January/068073.html). It's probably time to ask what is the status of this patch? If this patch gets accepted (Alexandre seemed to like the previous version, but pointed that it broke kernel32 tests because there had been no way to disable it and he didn't have much time to review it), maybe calling `winedbg --auto` from ReportFault would be the best way - you would then have a dialog (the same as during a regular crash, can be disabled from the registry), the full debugger output and an option to run an interactive debugger. AFAIK this is the other way round than Windows does (AFAIK drwatson loads faultrep.dll into the process that crashed and calls ReportFault), but loading a DLL into a process where everything could be corrupted seems a strange idea. Jérôme Gardou pisze:
Most wine users, when their app crashes, open a bug report and attach the console log. If the log contains information got from ReportFault, developers will gain some time, and have the preceding context of the call.
I'm affraid this is what is done by the 1% best users ;). I'd expect that a typical user to complain that Linux is broken because on Linux, from time to time, apps just disappear from the screen while on Windows they don't. Already knowing that there is such a thing like Wine requires some knowledge :). Mikołaj Zalewski
Mikołaj Zalewski a écrit :
I've written a dialog about a crash some time ago to add to winedbg (http://www.winehq.org/pipermail/wine-patches/2009-January/068073.html). It's probably time to ask what is the status of this patch? If this patch gets accepted (Alexandre seemed to like the previous version, but pointed that it broke kernel32 tests because there had been no way to disable it and he didn't have much time to review it), maybe calling `winedbg --auto` from ReportFault would be the best way - you would then have a dialog (the same as during a regular crash, can be disabled from the registry), the full debugger output and an option to run an interactive debugger. AFAIK this is the other way round than Windows does (AFAIK drwatson loads faultrep.dll into the process that crashed and calls ReportFault), but loading a DLL into a process where everything could be corrupted seems a strange idea.
Launching winedbg sounds good, but I wonder if calling winedbg AFTER the exception has been catched by the application would be useful. It's time for me to document myself :-)
Jérôme Gardou pisze:
Most wine users, when their app crashes, open a bug report and attach the console log. If the log contains information got from ReportFault, developers will gain some time, and have the preceding context of the call.
I'm affraid this is what is done by the 1% best users ;). I'd expect that a typical user to complain that Linux is broken because on Linux, from time to time, apps just disappear from the screen while on Windows they don't. Already knowing that there is such a thing like Wine requires some knowledge :).
Mikołaj Zalewski
Let's rephrase it : most bug reporters expect the console log to be useful for developers willing to fix their problem :-)
Jérôme Gardou pisze:
Let's rephrase it : most bug reporters expect the console log to be useful for developers willing to fix their problem :-) Maybe I should have made it clear - I think that your patch, even in the first version, was a step forward :). As for the second patch, Alexandre doesn't like when we export some wine-specific functions from DLLs. On the other hand, kernel32 is always builtin, so the main argument against it (that it will break if someone uses a native DLL) is not present, so maybe it could be accepted. But it's Alexandre who can answer it.
Mikołaj
Mikołaj Zalewski a écrit :
Jérôme Gardou pisze:
Let's rephrase it : most bug reporters expect the console log to be useful for developers willing to fix their problem :-) Maybe I should have made it clear - I think that your patch, even in the first version, was a step forward :). As for the second patch, Alexandre doesn't like when we export some wine-specific functions from DLLs. On the other hand, kernel32 is always builtin, so the main argument against it (that it will break if someone uses a native DLL) is not present, so maybe it could be accepted. But it's Alexandre who can answer it.
Mikołaj
Well, launching winedbg instead of printing incomplete information is, IMHO, the best way to make something useful, and, as you said it, kernel32 is always builtin, as ntdll, and faultrep is too close of the system internals to be a totally winapiesque dll. After all, kernel32 already exports a few wine_* functions. To be honest, I'd really like to have an opinion about this patch before sending it and being silently rejected.
"Jérôme Gardou" <jerome.gardou(a)gmail.com> writes:
Well, launching winedbg instead of printing incomplete information is, IMHO, the best way to make something useful, and, as you said it, kernel32 is always builtin, as ntdll, and faultrep is too close of the system internals to be a totally winapiesque dll. After all, kernel32 already exports a few wine_* functions.
To be honest, I'd really like to have an opinion about this patch before sending it and being silently rejected.
You certainly don't want to add exports to kernel32 for that. Launching the debugger isn't hard to do in faultrep, plus you most likely want to do it differently from what kernel32 does. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard a écrit :
"Jérôme Gardou" <jerome.gardou(a)gmail.com> writes:
Well, launching winedbg instead of printing incomplete information is, IMHO, the best way to make something useful, and, as you said it, kernel32 is always builtin, as ntdll, and faultrep is too close of the system internals to be a totally winapiesque dll. After all, kernel32 already exports a few wine_* functions.
To be honest, I'd really like to have an opinion about this patch before sending it and being silently rejected.
You certainly don't want to add exports to kernel32 for that. Launching the debugger isn't hard to do in faultrep, plus you most likely want to do it differently from what kernel32 does.
Is this one too hackish for you ?
From d54e4c5bf909e9bde775a22cb1eabb6f8cae1bcb Mon Sep 17 00:00:00 2001 From: =?utf-8?q?J=C3=A9r=C3=B4me=20Gardou?= <jerome.gardou(a)laposte.net> Date: Mon, 23 Feb 2009 20:47:34 +0100 Subject: [PATCH] faultrep:Make ReportFault display useful information by calling winedbg. MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------1.6.0.6"
This is a multi-part message in MIME format. --------------1.6.0.6 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit --- dlls/faultrep/faultrep.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) --------------1.6.0.6 Content-Type: text/x-patch; name="d54e4c5bf909e9bde775a22cb1eabb6f8cae1bcb.diff" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="d54e4c5bf909e9bde775a22cb1eabb6f8cae1bcb.diff" diff --git a/dlls/faultrep/faultrep.c b/dlls/faultrep/faultrep.c index d4bd713..df02f5b 100644 --- a/dlls/faultrep/faultrep.c +++ b/dlls/faultrep/faultrep.c @@ -107,7 +107,10 @@ BOOL WINAPI AddERExcludedApplicationA(LPCSTR lpAppFileName) */ EFaultRepRetVal WINAPI ReportFault(LPEXCEPTION_POINTERS pep, DWORD dwOpt) { - FIXME("%p 0x%x stub\n", pep, dwOpt); + LPTOP_LEVEL_EXCEPTION_FILTER filter = SetUnhandledExceptionFilter(NULL) ; + FIXME("%p 0x%x stub. Getting back to winedbg\n", pep, dwOpt); + UnhandledExceptionFilter(pep) ; + SetUnhandledExceptionFilter(filter) ; return frrvOk; } --------------1.6.0.6--
"Jérôme Gardou" <jerome.gardou(a)gmail.com> writes:
Alexandre Julliard a écrit :
You certainly don't want to add exports to kernel32 for that. Launching the debugger isn't hard to do in faultrep, plus you most likely want to do it differently from what kernel32 does.
Is this one too hackish for you ?
Yes. -- Alexandre Julliard julliard(a)winehq.org
Mikołaj Zalewski a écrit :
I've written a dialog about a crash some time ago to add to winedbg (http://www.winehq.org/pipermail/wine-patches/2009-January/068073.html). It's probably time to ask what is the status of this patch? If this patch gets accepted (Alexandre seemed to like the previous version, but pointed that it broke kernel32 tests because there had been no way to disable it and he didn't have much time to review it), maybe calling `winedbg --auto` from ReportFault would be the best way - you would then have a dialog (the same as during a regular crash, can be disabled from the registry), the full debugger output and an option to run an interactive debugger. AFAIK this is the other way round than Windows does (AFAIK drwatson loads faultrep.dll into the process that crashed and calls ReportFault), but loading a DLL into a process where everything could be corrupted seems a strange idea.
Jérôme Gardou pisze:
Most wine users, when their app crashes, open a bug report and attach the console log. If the log contains information got from ReportFault, developers will gain some time, and have the preceding context of the call.
I'm affraid this is what is done by the 1% best users ;). I'd expect that a typical user to complain that Linux is broken because on Linux, from time to time, apps just disappear from the screen while on Windows they don't. Already knowing that there is such a thing like Wine requires some knowledge :).
Mikołaj Zalewski
Instead of reinventing the wheel, why not call kernel32 which does it for us.... What do you think about this patch?
From 8d94c4a333f90cc2be6c9019f84ccc7712b5a0b2 Mon Sep 17 00:00:00 2001 From: =?utf-8?q?J=C3=A9r=C3=B4me=20Gardou?= <jerome.gardou(a)laposte.net> Date: Mon, 23 Feb 2009 09:35:13 +0100 Subject: [PATCH] faultrep: Make ReportFault do something useful MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------1.6.0.6"
This is a multi-part message in MIME format. --------------1.6.0.6 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit --- dlls/faultrep/faultrep.c | 7 ++++++- dlls/kernel32/except.c | 2 +- dlls/kernel32/kernel32.spec | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) --------------1.6.0.6 Content-Type: text/x-patch; name="8d94c4a333f90cc2be6c9019f84ccc7712b5a0b2.diff" Content-Transfer-Encoding: 8bit Content-Disposition: attachment; filename="8d94c4a333f90cc2be6c9019f84ccc7712b5a0b2.diff" diff --git a/dlls/faultrep/faultrep.c b/dlls/faultrep/faultrep.c index d4bd713..bb896f9 100644 --- a/dlls/faultrep/faultrep.c +++ b/dlls/faultrep/faultrep.c @@ -37,6 +37,7 @@ static const WCHAR SZ_EXCLUSIONLIST_KEY[] = { 'E','r','r','o','r','R','e','p','o','r','t','i','n','g','\\', 'E','x','c','l','u','s','i','o','n','L','i','s','t', 0}; + /************************************************************************* * AddERExcludedApplicationW [FAULTREP.@] * @@ -107,7 +108,11 @@ BOOL WINAPI AddERExcludedApplicationA(LPCSTR lpAppFileName) */ EFaultRepRetVal WINAPI ReportFault(LPEXCEPTION_POINTERS pep, DWORD dwOpt) { - FIXME("%p 0x%x stub\n", pep, dwOpt); + LONG ret ; + TRACE("%p 0x%x stub\n", pep, dwOpt); + + ret = wine_start_debugger_atomic(pep) ; + return frrvOk; } diff --git a/dlls/kernel32/except.c b/dlls/kernel32/except.c index 21d3584..f471e05 100644 --- a/dlls/kernel32/except.c +++ b/dlls/kernel32/except.c @@ -347,7 +347,7 @@ EXIT: * * returns TRUE for the two first conditions, FALSE for the last */ -static int start_debugger_atomic(PEXCEPTION_POINTERS epointers) +int CDECL start_debugger_atomic(PEXCEPTION_POINTERS epointers) { static HANDLE hRunOnce /* = 0 */; diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec index b0f4ee7..e5e79c3 100644 --- a/dlls/kernel32/kernel32.spec +++ b/dlls/kernel32/kernel32.spec @@ -1264,3 +1264,6 @@ # Init code @ cdecl __wine_kernel_init() + +#debugging +@ cdecl wine_start_debugger_atomic(ptr) start_debugger_atomic --------------1.6.0.6--
participants (7)
-
Alexandre Julliard -
Austin English -
Ben Klein -
Chris Robinson -
Erich Hoover -
Jérôme Gardou -
Mikołaj Zalewski