Re: kernel32/tests: Fix a failing test in vista
Nicolas Le Cam [EMAIL PROTECTED] wrote: Vista introduced new flags for CompareString, as stated in [1]. The value defined for LINGUISTIC_IGNORECASE is 0x10. So the invalid flag test don't fail as expected starting from this platform. [1] http://msdn.microsoft.com/en-us/library/ms647476.aspx LINGUISTIC_IGNORECASE should be added first instead of using a hex value, and CompareString implementation should handle that flag as well. -- Dmitry.
Re: one liner patch to stop crash, everquest2.exe
2008/7/21 Stefan Dösinger [EMAIL PROTECTED]: Is the shader backend recreated properly after the reset? Just to clarify, in dlls/wined3d/device.c, IWineD3DDeviceImpl_Reset(), line 7355 there's a call to shader_alloc_private(). This call is supposed to recreate This-shader_priv.
Re: one liner patch to stop crash, everquest2.exe
I changed: hr = This-shader_backend-shader_alloc_private(iface); To the following: FIXME(BEFORE, hr: 0x%08x\n, hr); hr = This-shader_backend-shader_alloc_private(iface); FIXME(AFTER, hr: 0x%08x\n, hr); ...and then I ran everquest2.exe again, alt+enter to full screen... fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d:IWineD3DDeviceImpl_Reset BEFORE, hr: 0x fixme:d3d:IWineD3DDeviceImpl_Reset AFTER, hr: 0x fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d:IWineD3DDeviceImpl_Reset BEFORE, hr: 0x fixme:d3d:IWineD3DDeviceImpl_Reset AFTER, hr: 0x fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER wine: Unhandled page fault on read access to 0x0008 at address 0x7e44abcc (thread 0009), starting debugger... The last two seem interesting where they don't reach shader_alloc_private() portion of the code, could this maybe be part of the problem? On Mon, Jul 21, 2008 at 1:39 PM, H. Verbeet [EMAIL PROTECTED] wrote: 2008/7/21 Stefan Dösinger [EMAIL PROTECTED]: Is the shader backend recreated properly after the reset? Just to clarify, in dlls/wined3d/device.c, IWineD3DDeviceImpl_Reset(), line 7355 there's a call to shader_alloc_private(). This call is supposed to recreate This-shader_priv.
Re: one liner patch to stop crash, everquest2.exe
2008/7/21 Andrew Fenn [EMAIL PROTECTED]: fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d:IWineD3DDeviceImpl_Reset BEFORE, hr: 0x fixme:d3d:IWineD3DDeviceImpl_Reset AFTER, hr: 0x fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d:IWineD3DDeviceImpl_Reset BEFORE, hr: 0x fixme:d3d:IWineD3DDeviceImpl_Reset AFTER, hr: 0x fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER wine: Unhandled page fault on read access to 0x0008 at address 0x7e44abcc (thread 0009), starting debugger... The last two seem interesting where they don't reach shader_alloc_private() portion of the code, could this maybe be part of the problem? That's probably the reason, yes. I don't see how Reset can return before calling shader_alloc_private() though.
Re: one liner patch to stop crash, everquest2.exe
Here's my uneducated idea about what's happening, it's calling the USER_SetWindowPos() in /user32/winpos.c to an x and y of 0 which goes off to SendMessageW. SendMessageW calls send_message which in turn calls call_window_proc. At this point something resets because SendMessageW should log both a 1 and 2\n which I added to be printed out but as you can see below it only prints out a 1 and then goes on to IWineD3DDeviceImpl_Reset after hitting call_window_proc.. fixme:msg:SendMessageW 1sendmsg1 fixme:msg:send_message sendmsg2 fixme:msg:send_message sendmsg3 fixme:d3d:IWineD3DDeviceImpl_Reset HELLO, hr: 0x fixme:d3d_shader:shader_glsl_free FREE SHADER wine: Unhandled page fault on read access to 0x0008 at address 0x7e452e5c (thread 0009), starting debugger... Unhandled exception: page fault on read access to 0x0008 in 32-bit code (0x7e452e5c). Does any of that make any sense? On Mon, Jul 21, 2008 at 2:58 PM, H. Verbeet [EMAIL PROTECTED] wrote: 2008/7/21 Andrew Fenn [EMAIL PROTECTED]: fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d:IWineD3DDeviceImpl_Reset BEFORE, hr: 0x fixme:d3d:IWineD3DDeviceImpl_Reset AFTER, hr: 0x fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d:IWineD3DDeviceImpl_Reset BEFORE, hr: 0x fixme:d3d:IWineD3DDeviceImpl_Reset AFTER, hr: 0x fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE fixme:d3d_shader:shader_glsl_free FREE SHADER wine: Unhandled page fault on read access to 0x0008 at address 0x7e44abcc (thread 0009), starting debugger... The last two seem interesting where they don't reach shader_alloc_private() portion of the code, could this maybe be part of the problem? That's probably the reason, yes. I don't see how Reset can return before calling shader_alloc_private() though.
Re: gphoto2.ds: On Solaris Blastwave's libgphoto2.so needs libintl.so.
Francois Gouget [EMAIL PROTECTED] writes: Blastwave's 'gphoto2-config --libs' does not say that we need to link with libintl.so when linking with libgphoto2.so. In a way it's normal as this dependency does not come from libgphoto2 itself, but from one of the libraries it links with. I don't think libintl is available everywhere so I made this patch that first tries to link without it, and then again with. I think that should be fixed in Blastwave. The whole point of having a config --libs script is to return the necessary paths and dependencies. -- Alexandre Julliard [EMAIL PROTECTED]
Re: one liner patch to stop crash, everquest2.exe
2008/7/21 Andrew Fenn [EMAIL PROTECTED]: Here's my uneducated idea about what's happening, it's calling the USER_SetWindowPos() in /user32/winpos.c to an x and y of 0 which goes off to SendMessageW. SendMessageW calls send_message which in turn calls call_window_proc. At this point something resets because SendMessageW should log both a 1 and 2\n which I added to be printed out but as you can see below it only prints out a 1 and then goes on to IWineD3DDeviceImpl_Reset after hitting call_window_proc.. fixme:msg:SendMessageW 1sendmsg1 fixme:msg:send_message sendmsg2 fixme:msg:send_message sendmsg3 fixme:d3d:IWineD3DDeviceImpl_Reset HELLO, hr: 0x fixme:d3d_shader:shader_glsl_free FREE SHADER wine: Unhandled page fault on read access to 0x0008 at address 0x7e452e5c (thread 0009), starting debugger... Unhandled exception: page fault on read access to 0x0008 in 32-bit code (0x7e452e5c). Does any of that make any sense? Ah, so it's essentially a recursive Reset call. My guess would be that we don't want to allow that, but it requires some tests on native win32 to verify. You could try returning WINED3DERR_INVALIDCALL when Reset gets called from inside another Reset call, to see how the application reacts to that.
Re: [PATCH 2/2] gdiplus: Detect integer overflow in GdipCreateBitmapFromScan0.
Lei Zhang [EMAIL PROTECTED] writes:
-datalen = abs(stride * height);
+datalen = stride * height;
size = sizeof(BITMAPFILEHEADER) + sizeof(BITMAPINFOHEADER) + datalen;
+if (datalen = 0 || size = 0){
+GdipFree(*bitmap);
+*bitmap = NULL;
+return InvalidParameter;
+}
Testing for overflow is a good idea, but checking for a negative result
is not the right way. You can get overflow with a positive result too.
--
Alexandre Julliard
[EMAIL PROTECTED]
Re: mlang: A very basic implementation of the IMLangLineBreakConsole interface
+interface IMLangLineBreakConsole : IUnknown
+{
+HRESULT BreakLineML(
+[in] IUnknown* pSrcMLStr, /* FIXME: IMLangString */
+[in] long lSrcPos,
+[in] long lSrcLen,
+[in] long cMinColumns,
+[in] long cMaxColumns,
+[out] long* plLineLen,
+[out] long* plSkipLen);
Please add the necessary IMLangString definitions needed for the
function to be declared correctly.
--
Alexandre Julliard
[EMAIL PROTECTED]
Re: [PATCH] ntdll: Use our own implementation of atoi.
Lei Zhang [EMAIL PROTECTED] writes: diff --git a/dlls/ntdll/string.c b/dlls/ntdll/string.c index ebfa6fb..6b36364 100644 --- a/dlls/ntdll/string.c +++ b/dlls/ntdll/string.c @@ -466,10 +466,31 @@ ULONG __cdecl NTDLL_strtoul( const char *nptr, char **endptr, int base ) /* * atoi (NTDLL.@) + * + * Same implementation as _atoi64. If it's the same then there's no need to duplicate it, just call atoi64. Also what about atol? -- Alexandre Julliard [EMAIL PROTECTED]
Re: ntoskrnl tests - where to write them?
I have been looking at writing a couple of tests for ntoskrl functions but I have had difficulty locating a suitable place. It sort of looks like ntdll/tests is the place but it is not clear. Is there a guide for locating this one? Jeff Now I am working on ntoskrnl tests. I am going to send theirs to wine-patches soon.
RE: one liner patch to stop crash, everquest2.exe
Ah, so it's essentially a recursive Reset call. My guess would be that we don't want to allow that, but it requires some tests on native win32 to verify. You could try returning WINED3DERR_INVALIDCALL when Reset gets called from inside another Reset call, to see how the application reacts to that. This also raises another question regarding the window handling in wined3d: Are we allowed / supposed to modify the window size and position of the destination window the app supplies like this? A lot of apps seem to need that, otherwise the rendering is placed incorrectly or not visible because the window is hidden. However, it could be that Windows somehow renders correctly without changing the Window's properties. So the SetWindowPos() call in Reset() might be the wrong thing here. It is not entirely unexpected that the app calls Reset when it receives a Window position / size change. If the Window size changes, the app is supposed to reset the D3D device, otherwise the rendering output is stretched in Present(which can look kinda ugly)
Re: kernel32/tests: Fix a failing test in vista
(resending as I forgot to CC to wine-devel) This test is here to test handling of incorrect flags. So a hex value is necessary as even any combination of know values are correct according to msdn. Sorry if I wasn't clear enough. I'm new to wine and English isn't my first language. I can add LINGUISTIC_IGNORECASE and other new definitions and try to implement handling of those new flags in CompareString but IMHO it's a different task. 2008/7/21 Dmitry Timoshkov [EMAIL PROTECTED]: Nicolas Le Cam [EMAIL PROTECTED] wrote: Vista introduced new flags for CompareString, as stated in [1]. The value defined for LINGUISTIC_IGNORECASE is 0x10. So the invalid flag test don't fail as expected starting from this platform. [1] http://msdn.microsoft.com/en-us/library/ms647476.aspx LINGUISTIC_IGNORECASE should be added first instead of using a hex value, and CompareString implementation should handle that flag as well. -- Dmitry.
Re: wineesd: Make use of `esd-config --libs` for the configure check. Filter out /usr/lib and /usr/lib64. Also follow the naming convention for the 'xxx-config' variables.
Francois Gouget [EMAIL PROTECTED] writes: I added code to filter out -L/usr/lib and -L/usr/lib64 because we're doing it for gphoto2 already, presumably to avoid trouble on 64bit Linux systems. I'm not sure if that was a gphoto2-specific issue or if it should be done more systematically. If the latter, then there are other places that don't do that filtering: libxml-2.0, libxslt, hal and freetype (but then maybe they don't report the -L settings). If the filtering is not desired, then I can resubmit. We have only had problems with it with gphoto2 as far as I know. If some other libraries have the same problem then it can be added for them, but I don't see a need to do it systematically. -- Alexandre Julliard [EMAIL PROTECTED]
Re: [5/6] WineD3D: Remove some #ifdefs
Right now it's simply broken of course. The extension being defined in the header is no guarantee the driver actually supports it.
Re: gdi32: StretchDIBits seems to use the wrong picture offset
On Sat, Jul 19, 2008 at 11:06 AM, Mathias Kosch [EMAIL PROTECTED] wrote: I provided a screenshot as attachment for you to see. I wasn't able to send it to this mailing list. I think this is by intention. So please ask me if you want to see. You can attach the screenshots to the bug's entry in bugzilla. Please send patches to wine-patches, not wine-devel.
Failing make crosstest
For me make crosstest fails on comctl32, is anyone else seeing this? Or have I missed a secret step? make[2]: Entering directory `/usr/local/src/cvs/wine/dlls/comctl32/tests' i586-mingw32msvc-windres -i rsrc.res -o rsrc.res.cross.o i586-mingw32msvc-windres: rsrc.res: Not a valid WIN32 resource file make[2]: *** [rsrc.res.cross.o] Error 1 make[2]: Leaving directory `/usr/local/src/cvs/wine/dlls/comctl32/tests' make[1]: *** [comctl32/tests/__crosstest__] Error 2 make[1]: Leaving directory `/usr/local/src/cvs/wine/dlls' make: *** [dlls/__crosstest__] Error 2 signature.asc Description: This is a digitally signed message part
RE: CUDA wrapper
You could use oprofile to find out where the CPU time is spent - this behavior can be caused by a lot of issues. Does the Cuda client work now? If so, it would be cool if we could include the wrapper in Wine, or get it into a shape to make it easilly redistributable and installable next to Wine. From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Seth Shelnutt Sent: Saturday, July 19, 2008 7:12 AM To: wine-devel@winehq.org Subject: Re: CUDA wrapper It seems when using this wrapper and a cuda enabled program, it causes the program/wine to use 100% of a CPU core, while running in windows the FaH GPU client only takes around 10-15% at most of a CPU core. Any ideas why the sudden jump to 100% use? It makes the systems most unusable in the normal sense, as a desktop.
RE: [5/6] WineD3D: Remove some #ifdefs
Right now it's simply broken of course. The extension being defined in the header is no guarantee the driver actually supports it. That's the reason for the patch. Of course my patch doesn't fix the fact that we just assume this extension is supported. For perfection we'll need a codepath that is based on OpenGL 1.0 texture combiners. I guess I'll do that as a fun project once I can get my hands on this old Mach64 card again...
Re: [RFC] crypt32: fixed the base64 tests on Vista.
Hi Reece, thanks for looking into failures on Vista. To me, without understanding this in any more detail, it looks as if Vista is broken here (and thus the Vista return parts should be marked as broken()). However, Vista may be doing the right thing, and thus the behaviour in these cases has changed between XP and Vista. The latter seems more likely, but I do not have any experience in this area, nor understand these tests well enough to say one way or the other. I think you're probably right that Vista's changed. By definition, that makes it right. I suspect what it's doing is guessing that something is base64-encoded even if it doesn't begin with the -BEGIN CERTIFICATE- or whatever header. It'd be interesting to see what format Vista guesses the encoded data was in when the encoded data have no header. I don't have access to a Vista machine myself, so it'd be hard for me to fix it. Would you mind having a go? Thanks, --Juan
Re: [RFC] crypt32: fixed the base64 tests on Vista.
2008/7/21 Juan Lang [EMAIL PROTECTED]: Hi Reece, thanks for looking into failures on Vista. No problem. To me, without understanding this in any more detail, it looks as if Vista is broken here (and thus the Vista return parts should be marked as broken()). However, Vista may be doing the right thing, and thus the behaviour in these cases has changed between XP and Vista. The latter seems more likely, but I do not have any experience in this area, nor understand these tests well enough to say one way or the other. I think you're probably right that Vista's changed. By definition, that makes it right. I suspect what it's doing is guessing that something is base64-encoded even if it doesn't begin with the -BEGIN CERTIFICATE- or whatever header. It'd be interesting to see what format Vista guesses the encoded data was in when the encoded data have no header. I don't have access to a Vista machine myself, so it'd be hard for me to fix it. Thanks for the pointers as to what may be going on. Would you mind having a go? Thanks, I'll need to take a look at the tests in more detail and the API documentation (if I can get to the non-WinCE docs on MSDN!) to see what is really going on and which ones are failing and why. I will dig deeper into this. Thanks, - Reece
Re: [PATCH] shell32: Clear a pointer on failure in XDG_UserDirLookup.
On Wed, Jul 16, 2008 at 12:54 PM, Lei Zhang [EMAIL PROTECTED] wrote: Hi, We should clear the out pointer on failure, so the caller won't access bad data. Anything wrong with this patch? Should I check the return value from XDG_UserDirLookup() instead?
Re: [PATCH] shell32: Clear a pointer on failure in XDG_UserDirLookup.
On Mon, Jul 21, 2008 at 1:41 PM, Lei Zhang [EMAIL PROTECTED] wrote: On Wed, Jul 16, 2008 at 12:54 PM, Lei Zhang [EMAIL PROTECTED] wrote: Hi, We should clear the out pointer on failure, so the caller won't access bad data. Anything wrong with this patch? Should I check the return value from XDG_UserDirLookup() instead? Nevermind, just saw commit de3afabf.
Re: rasapi32: the tests need raserror.h to build using MS headers.
Am Monday 21 July 2008 21:44 schrieb Reece Dunn: The error constants (e.g. ERROR_BUFFER_TOO_SMALL) are defined in raserror.h, which is needed for the tests to build using the Microsoft headers (checked against the Vista SDK headers). - Reece my wine tree does not have the raserror.h header. Therefore the compilation under wine breaks for me. Did you forget to include the header file in the patch? Stefan
Re: [Gdiplus try2 02/16] Implement GdipCreateRegion
Adam Petaccia wrote: @@ -226,6 +277,11 @@ GpStatus WINGDIPAPI GdipSetEmpty(GpRegion*region) if(!(calls++)) FIXME(not implemented\n); +TRACE(%p\n, region); + +GdipDeleteRegion(region); +GdipCreateRegion(region); +region-node-type = RegionDataEmptyRect; return NotImplemented; } This is still wrong. You probably don't even want to implement GdipSetEmpty in this patch anyway. In general you may be better off submitting a smaller patch series rather than 16 patches in one go. Huw.
Re: [Gdiplus try2 02/16] Implement GdipCreateRegion
On Mon, 2008-07-21 at 22:08 +0100, Huw Davies wrote: Adam Petaccia wrote: @@ -226,6 +277,11 @@ GpStatus WINGDIPAPI GdipSetEmpty(GpRegion*region) if(!(calls++)) FIXME(not implemented\n); +TRACE(%p\n, region); + +GdipDeleteRegion(region); +GdipCreateRegion(region); +region-node-type = RegionDataEmptyRect; return NotImplemented; } This is still wrong. You probably don't even want to implement GdipSetEmpty in this patch anyway. Ug, that's a user problem in dealing with histories with git. The behavior was corrected in a later patch in the series. Is there anything else wrong with the patch series? If not I'll resend with this issue straightened out. In general you may be better off submitting a smaller patch series rather than 16 patches in one go. Huw.
Valgrind warnings in new msxml3 saxreader code?
Hi Piotr, I see a lot of new valgrind warnings today that seem to be triggered by your new msxml3 code. I don't know offhand if they're bugs in libxml2 or in your code, could you have a look? Thanks! See http://kegel.com/wine/valgrind/logs/2008-07-21-07.56/vg-msxml3_saxreader-diff.txt Here are the first two: Conditional jump or move depends on uninitialised value(s) at xmlStrlen (in /usr/lib/libxml2.so.2.6.31) by xmlSetupParserForBuffer (in /usr/lib/libxml2.so.2.6.31) by isaxxmlreader_parse (saxreader.c:1077) by test_saxreader (saxreader.c:502) by func_saxreader (saxreader.c:518) by run_test (test.h:488) by main (test.h:537) Uninitialised value was created by a client request at mark_block_uninitialized (heap.c:164) by RtlAllocateHeap (heap.c:1236) by SAFEARRAY_Malloc (safearray.c:94) by SafeArrayAllocData (safearray.c:536) by SAFEARRAY_Create (safearray.c:231) by SafeArrayCreate (safearray.c:576) by test_saxreader (saxreader.c:494) by func_saxreader (saxreader.c:518) by run_test (test.h:488) by main (test.h:537) Conditional jump or move depends on uninitialised value(s) at get_length_mbs_utf8 (utf8.c:286) by wine_utf8_mbstowcs (utf8.c:312) by MultiByteToWideChar (locale.c:1838) by bstr_from_xmlChar (node.c:284) by libxmlCharacters (saxreader.c:249) by xmlParseCharData (in /usr/lib/libxml2.so.2.6.31) by xmlParseContent (in /usr/lib/libxml2.so.2.6.31) by xmlParseElement (in /usr/lib/libxml2.so.2.6.31) by xmlParseDocument (in /usr/lib/libxml2.so.2.6.31) by isaxxmlreader_parse (saxreader.c:1083) by test_saxreader (saxreader.c:502) by func_saxreader (saxreader.c:518) by run_test (test.h:488) by main (test.h:537) Uninitialised value was created by a client request at mark_block_uninitialized (heap.c:164) by RtlAllocateHeap (heap.c:1236) by SAFEARRAY_Malloc (safearray.c:94) by SafeArrayAllocData (safearray.c:536) by SAFEARRAY_Create (safearray.c:231) by SafeArrayCreate (safearray.c:576) by test_saxreader (saxreader.c:494) by func_saxreader (saxreader.c:518) by run_test (test.h:488) by main (test.h:537) ...
Re: one liner patch to stop crash, everquest2.exe
yes I am looking at the shaders... and am noticing something as well in the traces... some reason when it goes to ask how much memory to use... on my machine it thinks it only has 16mb of texture memory.. when the laptop has 256mb of video ram...? if you look in device.c the pixel shader fails atleast on my firegl5200.? The other curiosity is that when EverQuest2.exe runs it seems to be going through the adapter initialization two times.. once when it loads then it starts loading up sound? and then it looks atleast in my traces? it initializes the adapter again getting an adapter which is the second adapter (I think) used for dual head Chris
Re: one liner patch to stop crash, everquest2.exe
Stefan Dösinger wrote:
Is the shader backend recreated properly after the reset?
*From:* [EMAIL PROTECTED]
[mailto:[EMAIL PROTECTED] *On Behalf Of *Andrew Fenn
*Sent:* Sunday, July 20, 2008 11:11 PM
*To:* H. Verbeet
*Cc:* wine-devel@winehq.org
*Subject:* Re: one liner patch to stop crash, everquest2.exe
I think that both calls are coming from d3d9:IDirect3DDevice9Impl_Reset
I put some FIXME's in the code and it passes the same function. In the
log below you can see where the crash is happening.
fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE
fixme:d3d_shader:shader_glsl_free FREE SHADER
fixme:d3d9:IDirect3DDevice9Impl_Reset RESET DEVICE
fixme:d3d_shader:shader_glsl_free FREE SHADER
wine: Unhandled page fault on read access to 0x0008 at address
0x7e44ab4c (thread 0009), starting debugger...
Does this mean that the problem resides somewhere in
IDirect3DDevice9Impl_Reset ?
On Thu, Jul 17, 2008 at 10:44 PM, H. Verbeet [EMAIL PROTECTED]
mailto:[EMAIL PROTECTED] wrote:
2008/7/17 Andrew Fenn [EMAIL PROTECTED] mailto:[EMAIL PROTECTED]:
Here's what I got from the debug log.
Backtrace:
=1 0x7e43e9b3 shader_glsl_free+0x23(iface=0x19c4b8)
[/home/andrew/wine/wine/dlls/wined3d/glsl_shader.c:3491] in wined3d
(0x0032e918)
2 0x7e41f318 IWineD3DDeviceImpl_Reset+0x198(iface=0x19c4b8,
pPresentationParameters=0x32e9c0)
[/home/andrew/wine/wine/dlls/wined3d/device.c:7237] in wined3d
(0x0032e998)
3 0x7e4f33ce IDirect3DDevice9Impl_Reset+0x1ae(iface=register EDI
not in
topmost frame, pPresentationParameters=0x1b9f848)
[/home/andrew/wine/wine/dlls/d3d9/device.c:381] in d3d9 (0x0032ea08)
4 0x008d31a1 in everquest2 (+0x4d31a1) (0x0032ea3c)
0x7e43e9b3 shader_glsl_free+0x23
[/home/andrew/wine/wine/dlls/wined3d/glsl_shader.c:3491] in wined3d: movl
0x8(%esi),%eax
3491if(priv-depth_blt_glsl_program_id) {
That's the second one, but where does the previous call come from?
No I dont think it s getting reset correctly
chris
Re: general question..
On Mon, Jul 21, 2008 at 8:41 PM, Chris Ahrendt [EMAIL PROTECTED] wrote: As I have been going through trying to debug the everquest2 issues on my machine I have run into a few places where I think the code should be changed a little ( this alot of times is where there is a GOTO in the code). My question is in the case where I do find these goto's and I rewrite the code should I submit the changes here for approval? I have been doing some code reviews and found a few places where there are goto's that don't need to be there. I guess I am old school and really dislike code with goto's (they make debugging and maintenance a nightmare (I used to maintain 20 year old cobol code along time ago =) ) You're not going to find many in this project that agree with your stance on the use of goto's. That being said, I'm curious to see where you think the use of goto's should be replaced by something else. Feel free to send a RFC to wine-devel. P.S. Please don't send discussion emails to wine-patches. wine-devel is for discussion and wine-patches is for patches only. -- James Hawkins
RE: general question..
Can you give some examples of such code? Nobody here is a goto-maniac, but in all cases I know the goto is used for a good reason. Most of the time it is used for error path to avoid ugly if() nesting and/or code duplication when freeing partially allocated objects -Original Message- From: [EMAIL PROTECTED] [mailto:wine-patches- [EMAIL PROTECTED] On Behalf Of Chris Ahrendt Sent: Monday, July 21, 2008 8:42 PM To: [EMAIL PROTECTED] Subject: general question.. As I have been going through trying to debug the everquest2 issues on my machine I have run into a few places where I think the code should be changed a little ( this alot of times is where there is a GOTO in the code). My question is in the case where I do find these goto's and I rewrite the code should I submit the changes here for approval? I have been doing some code reviews and found a few places where there are goto's that don't need to be there. I guess I am old school and really dislike code with goto's (they make debugging and maintenance a nightmare (I used to maintain 20 year old cobol code along time ago =) ) Chris
Re: general question..
On Mon, Jul 21, 2008 at 9:24 PM, Chris Ahrendt [EMAIL PROTECTED] wrote: Well here is my list so far : device.c - filled with goto's if you need the routines I can supply them... they can be moved to a routine and called... wine$ find . -name device.c ./dlls/ddraw/device.c ./dlls/dinput/tests/device.c ./dlls/dinput/device.c ./dlls/wined3d/device.c ./dlls/d3d8/tests/device.c ./dlls/d3d8/device.c ./dlls/mountmgr.sys/device.c ./dlls/d3d9/tests/device.c ./dlls/d3d9/device.c ./programs/winedevice/device.c ./server/device.c context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. directx.c - same... this can be moved to a routine and simplified.. this is in routine WineD3D_CreateFakeGLContext which I was tracing down some stuff... cleanup usually does not warrant a new function. This is exactly what we use goto for. pixelshader.c - this has a recompile goto.. again why not make this into a routine? provider.c - same This function is ugly, but not because it uses goto. The function is 144 lines long, which is too long. I'm not familiar with this code, but I'm pretty sure some factorization is in order. The level of indentation could be reduced by breaking on failure instead of proceeding on success. Assuming this previous change is made, the use of the goto would be that much more appealing. These are just the ones I have run accross so far.. Why do you say that... or is the idea.. hack and get it done... one of the problems with goto's is it makes finding and putting in patches a pain in the ass.. and thats putting it bluntly.. Complete sentences go a long way towards effective communication :-) The use of goto's cannot possibly be considered as a hack. I also don't see how you find goto's a problem when patching the code. Of course.. Do you have a suggestion on how I should word the RFC? No, just send in a patch to wine-devel stating why you think the change is correct. P.S. Make sure you reply-all to cc wine-devel. We're an open project and we communicate openly so everyone can be a part of the discussion. -- James Hawkins
Re: general question..
Stefan Dösinger wrote: Can you give some examples of such code? Nobody here is a goto-maniac, but in all cases I know the goto is used for a good reason. Most of the time it is used for error path to avoid ugly if() nesting and/or code duplication when freeing partially allocated objects -Original Message- From: [EMAIL PROTECTED] [mailto:wine-patches- [EMAIL PROTECTED] On Behalf Of Chris Ahrendt Sent: Monday, July 21, 2008 8:42 PM To: [EMAIL PROTECTED] Subject: general question.. As I have been going through trying to debug the everquest2 issues on my machine I have run into a few places where I think the code should be changed a little ( this alot of times is where there is a GOTO in the code). My question is in the case where I do find these goto's and I rewrite the code should I submit the changes here for approval? I have been doing some code reviews and found a few places where there are goto's that don't need to be there. I guess I am old school and really dislike code with goto's (they make debugging and maintenance a nightmare (I used to maintain 20 year old cobol code along time ago =) ) Chris Sure =) Well here is my list so far : device.c - filled with goto's if you need the routines I can supply them... they can be moved to a routine and called... context.c - This case its just a return with noting else.. why do a goto why not just do the return? directx.c - same... this can be moved to a routine and simplified.. this is in routine WineD3D_CreateFakeGLContext which I was tracing down some stuff... pixelshader.c - why not make this into a routine? provider.c - same These are just the ones I have run accross so far.. In the case of code dupes instead of using a goto like the createfakeGLContext why not make the fail: if(wined3d_fake_gl_context_hdc) ReleaseDC(wined3d_fake_gl_context_hwnd, wined3d_fake_gl_context_hdc); wined3d_fake_gl_context_hdc = NULL; if(wined3d_fake_gl_context_hwnd) DestroyWindow(wined3d_fake_gl_context_hwnd); wined3d_fake_gl_context_hwnd = NULL; if(glCtx) pwglDeleteContext(glCtx); LeaveCriticalSection(wined3d_fake_gl_context_cs); return FALSE; into a routine and then do a return fakeContextFail(glCtx); Then of course above createfakeGLContext define fakeContextFail(..) Not saying Goto's are completely bad.. in some cases they are good for avoiding some tricky situations but alot of times they are not... just my $.02 not trying to start a flame war guys just trying to help =) Chris
Re: general question..
James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan
Re: general question..
Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan probably not this is the sort of discussion I was wanting to start with the GOTO comment =) Chris
Re: general question..
Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan The GL initialization stuff at the end should also go into a subfunction imho. It really looks like the core of the function control flow does this, unless I'm misunderstanding: hdc = get_hdc_somehow() [ offscreen and onscreen choices ] ctx = create_new_context(hdc) switch_context(new_ctx) initialize_gl_stuff_in_new_context(new_ctx) switch_context(old_ctx) The rest is unimportant details like making it work properly :) Ivan
Re: general question..
Ivan Gyurdiev wrote: Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan The GL initialization stuff at the end should also go into a subfunction imho. It really looks like the core of the function control flow does this, unless I'm misunderstanding: hdc = get_hdc_somehow() [ offscreen and onscreen choices ] ctx = create_new_context(hdc) switch_context(new_ctx) initialize_gl_stuff_in_new_context(new_ctx) switch_context(old_ctx) The rest is unimportant details like making it work properly :) Ivan Ok here is the next question.. do I just rework it into seperate functions and leave the main API routine alone and just call the others... whats the process to do something like this? Chris
Re: general question..
Chris Ahrendt wrote: fail: if(wined3d_fake_gl_context_hdc) ReleaseDC(wined3d_fake_gl_context_hwnd, wined3d_fake_gl_context_hdc); wined3d_fake_gl_context_hdc = NULL; if(wined3d_fake_gl_context_hwnd) DestroyWindow(wined3d_fake_gl_context_hwnd); wined3d_fake_gl_context_hwnd = NULL; if(glCtx) pwglDeleteContext(glCtx); LeaveCriticalSection(wined3d_fake_gl_context_cs); return FALSE; into a routine and then do a return fakeContextFail(glCtx); That will turn into nightmare when tracking a locking problem down. Call to LeaveCriticalSection should always be in the same function as EnterCriticalSection. Same applies to any resource that you do not want to leak. That is much more important then calling a function a routine and replacing all goto's with that function call. Also I'd like to show how you can free local variables?
Re: general question..
Chris Ahrendt wrote: Ivan Gyurdiev wrote: Ivan Gyurdiev wrote: James Hawkins wrote: context.c - same except in this case its just a return with noting else.. why do a goto why not just do the return? This is probably ok to change. I can only imagine that the original author thought there might be cleanup needed at some later point. The bigger question is why there is a huge if-else statement, and why this function is so large. Huge if-else statement = 2 sub-functions Shader dirty constants - do shader internals really belong here ? Ivan The GL initialization stuff at the end should also go into a subfunction imho. It really looks like the core of the function control flow does this, unless I'm misunderstanding: hdc = get_hdc_somehow() [ offscreen and onscreen choices ] ctx = create_new_context(hdc) switch_context(new_ctx) initialize_gl_stuff_in_new_context(new_ctx) switch_context(old_ctx) The rest is unimportant details like making it work properly :) Ivan Ok here is the next question.. do I just rework it into seperate functions and leave the main API routine alone and just call the others... whats the process to do something like this? Chris Unless you are trying to fix something, make it more efficient there is no reason to touch the code just because it looks bad. Some one might have lots of work done on said function and just about to send their changes in. Then come to find out that all of the changes have to be scrapped and rewritten because some one reformatted the code. And it also helps if you actually understand what that piece of code is doing. Some code is kept ugly just so one day it can be properly rewritten not just reformatted. Vitaliy.
