February 2009 - Wine-Devel - list.winehq.org

Re: faultrep:display useful information in ReportFault

by Jérôme Gardou

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--

Feb. 23, 2009

Re: faultrep:display useful information in ReportFault

by Alexandre Julliard

"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

Feb. 23, 2009

Re: [try2] gdiplus/tests: Dynamically load gdiplus.dll.

by Nicolas Le Cam

2009/2/23 Paul Vriens <paul.vriens.wine(a)gmail.com>: > Nicolas Le Cam wrote: >> >> 2009/2/23 Paul Vriens <paul.vriens.wine(a)gmail.com>: >>> >>> Nicolas Le Cam wrote: >>>> >>>> 2009/2/23 Francois Gouget <fgouget(a)free.fr>: >>>>> >>>>> On Sun, 22 Feb 2009, Nicolas Le Cam wrote: >>>>> >>>>>> This avoid a messagebox in regression tests on systems that don't have >>>>>> GdiPlus. >>>>> >>>>> This seems wrong because winetest.exe is already supposed to check for >>>>> missing dlls. If I remember correctly this was done explicitely so that >>>>> individual tests would not have to dynamically link everywhere. >>>>> >>>>> Also I tested on a platform that does not have gdiplus, and I did not >>>>> get a message box. In fact, on that platform, winetest did not extract >>>>> gdiplus_test.exe, which also seems very wrong to me. >>>>> >>>>> So I think that if there's any issue, it's on the winetest.exe side. >>>>> >>>>> -- >>>>> Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ >>>>> "Lotto: A tax on people who are bad at math." -- unknown >>>>> "Windows: Microsoft's tax on computer illiterates." -- WE7U >>>>> >>>>> >>>>> >>>> Thanks, >>>> >>>> On my w2k pro when I launch the winetest obtained from >>>> test.winehq.org, I get a MessageBox saying gdiplus is missing, and >>>> there's already some tests that load dlls dynamically (advpack, >>>> cryptui, fusion) so thought it was the right way. >>>> >>>> I will have a look on the winetest side. >>>> >>>> Nicolas Le Cam >>>> >>>> >>>> >>> The reason you see this is because next to a normal LoadLibrary we also >>> use the >>> .NET variant. On some boxes gdiplus.dll cannot be found through >>> LoadLibrary but >>> will trough LoadLibraryShim. As winetest now thinks the library is there >>> it will >>> try to launch "gdiplus_test.exe --list" and that of course fails. >>> >>> Not sure how to approach this. >>> >>> -- >>> Cheers, >>> >>> Paul. >>> >>> >>> >> >> This shouldn't be the case on my box as gdiplus is not present by >> default on W2K, and should be manually installed (not available >> through windows update), or installed by a third party app. >> My test box is a clean install without any third apps, except >> PDFCreator to simulate a printer to test this part of the suite. I >> will check if it has gdiplus bundled in. >> >> Nicolas Le Cam >> > Is .NET installed (any version)? > > -- > Cheers, > > Paul. > As you've suspected .Net Framework 1.1 and 2.0 SP1 are installed. If LoadLibraryShim is able to find gdiplus bundled with .NET Framework but LoadLibrary can't as well as linking the library to the test, shouldn't we avoid the use of it in the winetest check ? Tests that depends of a specific version of a .NET Framework dll could then use it inline ?

Feb. 23, 2009

Re: faultrep:display useful information in ReportFault

by Jérôme Gardou

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.

Feb. 23, 2009

Re: faultrep:display useful information in ReportFault

by Mikołaj Zalewski

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

Feb. 23, 2009

Re: faultrep:display useful information in ReportFault

by Chris Robinson

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.

Feb. 23, 2009

Re: faultrep:display useful information in ReportFault

by Erich Hoover

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

Feb. 23, 2009

Re: [1/2]tools/winemaker: add project-file support(try3)

by Francois Gouget

Hi, I had a look at this patch and here are some comments. The case of the filenames in the dsp and dsw files may not match the case of the filenames on the filesystem. So you need to ajsut the names before you put them in the makefile. I believe search_from() can be used for that. I see that you're translating the Visual C++ compiler flags to gcc compiler flags. That's good. But I'm not sure about the following conversion from /GX to -fexceptions. Exception handling is pretty different between windows and Unix. Do you know if /GX is about structured exception handing? I really couldn't tell, but maybe some exception expert will know. I see you're converting the /Zp1 option which is very nice (packing being one of the disagreements between Visual C++ and gcc). But you don't do it for Zp(2|4|8|16). I believe you could do it by simply appending the packing size to the '-fpack-struct=' option. (But maybe I missed something) A bigger issue is that you're putting all the compiler options in CXXEXTRA which only applies to C++ code. But in the dsp file it looks like this applies to both C and C++ code. So it probably should be put in both (except -fexceptions which seems to be on by default in C++, but that's minor). About the commented out call to fix_file_and_directory_names(), I don't think winemaker should go and modify files outside of '.'. So I think it's safe to remove this call entirely. Also I have some questions about a couple of heuristics: if ($has_headers) { push @{@$project_settings[$T_INCLUDE_PATH]},"-I."; } Is this still needed? In source_scan_directory() it was needed because we had no idea what the include path was supposed to look like. But the project file should define it clearly enough for us. However I have a dsp file in a directory with a header file and I don't see a '/I.'. So maybe it is implied? Also this part seems wrong: # Finally if we are building both libraries and programs in # this project, then the programs should be linked with all # the libraries The project file should be explicit enough on this part for us not to have to do any guessing. Also do we really need to worry much about matching source file with a given target? Won't we always have a single target per dsp file? In which case the matching can probably be simplified. The end of source_scan_project_file() looks pretty similar to the end of source_scan_directory(). Maybe some code can be shared. To make the patch smaller you can first submit the dsp parser, then the dsw one, the vcproj one, and finally the sln one. And once that's done, it would be nice to integrate this all with the regular source_scan() function, so that if it detects one of these files in the subdirectories, it invokes the appropriate project parser for that directory and stops recursing. That can be done as a further separate patch. Style issues: The style issues are at least partly my fault as I used two-space indentation and braces on the same line when I first wrote winemaker. Nowadays I'd prefer 4-space indentation with braces on their own line but that's a bit tricky when maintaining a consistent indentation. In any case, in places your code is too incoherent. For instance: } elsif(/^# TARGTYPE/) { if (/[[:space:]]0x0101$/) { # Win32 (x86) Application $prj_target_type=1; This snippet has both 2 and 4 space indentation and it's all new code. There's also an indentation problem there: } } foreach my $source (@sources_misc) { if ($source =~ /^$basename/i) { Personnally I'd recommend to maintain a consistent indentation scheme at least per function (though the policy is more to be consistent per file normally :-/ ). Also, I'd prefer to have a space between 'if' and the prentheseses and braces. You can also simplify a some string concatenations. Replace: $prj_target_cflags=$prj_target_cflags."-D".$1." "; with $prj_target_cflags.="-D$1 "; In any case, thanks for taking this on. I think you're pretty close to the end (even if you don't feel that way after reading the above<g>). -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ If you think the whole world revolves around you, quit staring at the GPS display while driving.

Feb. 23, 2009

Re: [try2] gdiplus/tests: Dynamically load gdiplus.dll.

by Austin English

On Mon, Feb 23, 2009 at 6:38 AM, Paul Vriens <paul.vriens.wine(a)gmail.com> wrote: > Nicolas Le Cam wrote: >> 2009/2/23 Paul Vriens <paul.vriens.wine(a)gmail.com>: >>> Nicolas Le Cam wrote: >>>> 2009/2/23 Francois Gouget <fgouget(a)free.fr>: >>>>> On Sun, 22 Feb 2009, Nicolas Le Cam wrote: >>>>> >>>>>> This avoid a messagebox in regression tests on systems that don't have >>>>>> GdiPlus. >>>>> This seems wrong because winetest.exe is already supposed to check for >>>>> missing dlls. If I remember correctly this was done explicitely so that >>>>> individual tests would not have to dynamically link everywhere. >>>>> >>>>> Also I tested on a platform that does not have gdiplus, and I did not >>>>> get a message box. In fact, on that platform, winetest did not extract >>>>> gdiplus_test.exe, which also seems very wrong to me. >>>>> >>>>> So I think that if there's any issue, it's on the winetest.exe side. >>>>> >>>>> -- >>>>> Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ >>>>> "Lotto: A tax on people who are bad at math." -- unknown >>>>> "Windows: Microsoft's tax on computer illiterates." -- WE7U >>>>> >>>>> >>>>> >>>> Thanks, >>>> >>>> On my w2k pro when I launch the winetest obtained from >>>> test.winehq.org, I get a MessageBox saying gdiplus is missing, and >>>> there's already some tests that load dlls dynamically (advpack, >>>> cryptui, fusion) so thought it was the right way. >>>> >>>> I will have a look on the winetest side. >>>> >>>> Nicolas Le Cam >>>> >>>> >>>> >>> The reason you see this is because next to a normal LoadLibrary we also use the >>> .NET variant. On some boxes gdiplus.dll cannot be found through LoadLibrary but >>> will trough LoadLibraryShim. As winetest now thinks the library is there it will >>> try to launch "gdiplus_test.exe --list" and that of course fails. >>> >>> Not sure how to approach this. >>> >>> -- >>> Cheers, >>> >>> Paul. >>> >>> >>> >> >> This shouldn't be the case on my box as gdiplus is not present by >> default on W2K, and should be manually installed (not available >> through windows update), or installed by a third party app. >> My test box is a clean install without any third apps, except >> PDFCreator to simulate a printer to test this part of the suite. I >> will check if it has gdiplus bundled in. >> >> Nicolas Le Cam >> > Is .NET installed (any version)? > > -- > Cheers, > > Paul. > > > FWIW, the W2K box I have at work has this same problem. There are several applications installed, including .Net 1.1 and .Net 2.0 Service Pack 1. -- -Austin

Feb. 23, 2009

Re: msctf: add internal function for handling TfClientIds

by Aric Stewart

Yup, reworked and resent. -aric James Mckenzie wrote: > Alexandre Julliard <julliard(a)winehq.org> wrote on Feb 23: >> Aric Stewart <aric(a)codeweavers.com> writes: >> >>> + /* If i understand this correctly at most a process should generate >>> + * only a handful of these... But in case I am wrong */ >>> + if (id_last == 65535) >>> + { >>> + ERR("TfClientIds generated exceeds USHORT limit\n"); >>> + return 0x0; >>> + } >>> + id_last++; >>> + id->id = MAKELONG(id_last, magic); >>> + id->data = data; >>> + list_add_head(&clientids,&id->entry); >> Using an array index would be cleaner than having to search through the >> list everytime you use an id. >> > Also, aren't linked lists inefficient as well. Searching through them can be very intense, unless they are small (< 5 elements). Anything larger than this and an array would be more efficient. > > James McKenzie > > >

Feb. 23, 2009