Juan wrote:
Since Rob was noncommittal about whether -Werror was a good or a bad idea, I'll take a stance: I think it's a bad one. ... some warnings are just plain wrong. In some cases the "fix" to quiet the warning is uglier than the warning.
In those cases, we can disable the warning.
Having a code base that has hundreds of warnings is also a problem, but we're fortunate enough not to be in that situation.
Are you sure about that? When I configure with -Wall -Werror, I can't even configure properly; one gets the errors
configure: libxrandr development files not found, XRandr won't be supported. configure: libxinerama development files not found, multi-monitor setups won't be supported. configure: WARNING: Old Mesa headers detected. Consider upgrading your Mesa libraries. OpenGL and Direct3D won't be supported.
And if one blows past those, "make depend" fails. In fact, everything fails pretty spectacularly.
We should probably have a look at -Wall errors in more detail and breadth before we assume that we're ok. - Dan
Are you sure about that? When I configure with -Wall -Werror, I can't even configure properly; one gets the errors
configure: libxrandr development files not found, XRandr won't be supported. configure: libxinerama development files not found, multi-monitor setups won't be supported. configure: WARNING: Old Mesa headers detected. Consider upgrading your Mesa libraries. OpenGL and Direct3D won't be supported.
This is precisely part of the problem with -Werror. You've now caused some configure checks to fail that, presumably, were passing before. Are they worth making warning-free? Some of the warnings may be in headers we don't control. What do we do then?
And if one blows past those, "make depend" fails. In fact, everything fails pretty spectacularly.
So a broken config leads to a broken compile? I guess I'm not seeing the severity of that. --Juan
On Thu, Sep 18, 2008 at 7:10 PM, Juan Lang juan.lang@gmail.com wrote:
Are you sure about that? When I configure with -Wall -Werror, I can't even configure properly; one gets the errors
configure: libxrandr development files not found, XRandr won't be supported. configure: libxinerama development files not found, multi-monitor setups won't be supported. configure: WARNING: Old Mesa headers detected. Consider upgrading your Mesa libraries. OpenGL and Direct3D won't be supported.
This is precisely part of the problem with -Werror. You've now caused some configure checks to fail that, presumably, were passing before. Are they worth making warning-free? Some of the warnings may be in headers we don't control. What do we do then?
I think the important thing for Dan's case is that he also used -Wall, which we should never turn on by default, especially not with -Werror. Correct me if I've made an incorrect assumption.
I think the important thing for Dan's case is that he also used -Wall, which we should never turn on by default, especially not with -Werror. Correct me if I've made an incorrect assumption.
-Wall is turned by default: EXTRACFLAGS="-Wall -pipe"
--Juan
On Thu, Sep 18, 2008 at 7:20 PM, Juan Lang juan.lang@gmail.com wrote:
I think the important thing for Dan's case is that he also used -Wall, which we should never turn on by default, especially not with -Werror. Correct me if I've made an incorrect assumption.
-Wall is turned by default: EXTRACFLAGS="-Wall -pipe"
K, wasn't sure. Thanks for the info.
On Thu, Sep 18, 2008 at 5:17 PM, James Hawkins truiken@gmail.com wrote:
I think the important thing for Dan's case is that he also used -Wall, which we should never turn on by default, especially not with -Werror. Correct me if I've made an incorrect assumption.
Many serious projects always use both -Wall and -Werror, including Chrome.
It would be nice if Wine also became serious in this regard, but it will take some work to get there. - Dan
On Thu, Sep 18, 2008 at 8:46 PM, Dan Kegel dank@kegel.com wrote:
On Thu, Sep 18, 2008 at 5:17 PM, James Hawkins truiken@gmail.com wrote:
I think the important thing for Dan's case is that he also used -Wall, which we should never turn on by default, especially not with -Werror. Correct me if I've made an incorrect assumption.
Many serious projects always use both -Wall and -Werror, including Chrome.
And not so serious ones. We actually try to use -Werror everywhere in ROS except the Wine code and a few other third party modules. I think it would be a good idea to enable it on a selective basis in Wine, start with programs,server,libs,etc and then slowly turn it on for dlls.
So Sitting on the airplane I was thinking of additional clarifications on debugging. Again these are just some ideas...
So lets look at an exception:
What I am talking about when more descriptive is this :
Since we know from other parts where the read is done.. why not dump the line of code?
Unhandled exception: page fault on read access to 0x00000008 in 32-bit code (0x602ec680).
so why not put as well the following:
Failure Point : /home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:1404] 1404 IDirectDrawSurface_Release(surface3);
Code Segment... is this in WINE SPACE or Windows Application Space:
Register dump: CS:0073 SS:007b DS:007b ES:007b FS:0033 GS:003b EIP:602ec680 ESP:0033f1d0 EBP:0033f2b8 EFLAGS:00010202( - 00 - -RI1) EAX:6030b1bc EBX:6030b1b8 ECX:6ab142cc EDX:00000000 ESI:8876000a EDI:60305e8e
Put out the ASCII value as well to the right. If we can see if this is a wine Structure in here.. why not dump that or be able to highlight that fact and say this is a FOO.. here is what it contains. Also this looks like a partial stack dump.. if These are pointers to addresses or structures then why not dump those as well. Not all the time will people be able to debug at the point of failure, timing issues and the like. At a minimum if we know a certain address is a certain wine structure then note it in the stack dump if we could.
Stack dump: 0x0033f1d0: 6030b1bc 603074e8 8876000a 00000000 0x0033f1e0: 00000064 00000064 000000a0 000000a0 0x0033f1f0: 00000000 00000000 00000000 00000000 0x0033f200: 60305e8e 0033f22c 60302736 0033f2a8 0x0033f210: 0033f2a4 0033f2a0 0033f29c 603076c0 0x0033f220: 6030734c 0033f298 00030028 0000006c
See Below for a thought on this :
Backtrace: =>1 0x602ec680 AttachmentTest+0xfa0() [/home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:1404] in ddraw_test (0x0033f2b8) 2 0x602f1735 func_dsurface+0x2175() [/home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:2764] in ddraw_test (0x0033fde8) 3 0x60301bd8 run_test+0x128(name="dsurface.c") [/home/cahrendt/wine-git/dlls/ddraw/tests/../../../include/wine/test.h:454] in ddraw_test (0x0033fe28) 4 0x60302442 main+0x122(argc=<register ECX not in topmost frame>, argv=<register ECX not in topmost frame>) [/home/cahrendt/wine-git/dlls/ddraw/tests/../../../include/wine/test.h:503] in ddraw_test (0x0033fed8) 5 0x603025bb __wine_spec_exe_entry+0x5b(peb=0x7ffdf000) [/home/cahrendt/wine-git/dlls/winecrt0/exe_entry.c:36] in ddraw_test (0x0033ff08) 6 0x7b874207 start_process+0xc7(arg=(nil)) [/home/cahrendt/wine-git/dlls/kernel32/process.c:904] in kernel32 (0x0033ffe8) 0x602ec680 AttachmentTest+0xfa0 [/home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:1404] in ddraw_test: call *0x8(%edx) 1404 IDirectDrawSurface_Release(surface3);
This would be a little clearer: Application Backtrace: => 1 0x602ec680 AttachmentTest+0xfa0() 2 0x602f1735 func_dsurface+0x2175() 3 0x60301bd8 run_test+0x128(name="dsurface.c") 4 0x60302442 main+0x122(argc=<register ECX not in topmost frame>, argv=<register ECX not in topmost frame>) 5 0x603025bb __wine_spec_exe_entry+0x5b(peb=0x7ffdf000) 6 0x7b874207 start_process+0xc7(arg=(nil))
Failure Point : /home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:1404] 1404 IDirectDrawSurface_Release(surface3);
[/home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:1404] in ddraw_test (0x0033f2b8) [/home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:2764] in ddraw_test (0x0033fde8) [/home/cahrendt/wine-git/dlls/ddraw/tests/../../../include/wine/test.h:454] in ddraw_test (0x0033fe28) [/home/cahrendt/wine-git/dlls/ddraw/tests/../../../include/wine/test.h:503] in ddraw_test (0x0033fed8) [/home/cahrendt/wine-git/dlls/winecrt0/exe_entry.c:36] in ddraw_test (0x0033ff08) [/home/cahrendt/wine-git/dlls/kernel32/process.c:904] in kernel32 (0x0033ffe8) [/home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:1404] in ddraw_test: call *0x8(%edx) 0x602ec680 AttachmentTest+0xfa0
The Thread that Loaded the Module would be nice here if we can get it:
Modules: Module Address Debug info Name (55 modules) ELF 626000- 62f000 Deferred libsm.so.6 ELF 640000- 65c000 Deferred ld-linux.so.2 ELF 65e000- 7a1000 Deferred libc.so.6 ELF 7a3000- 7ca000 Deferred libm.so.6 ELF 7cc000- 7d0000 Deferred libdl.so.2 ELF 7d2000- 7e9000 Deferred libpthread.so.0 ELF 800000- 809000 Deferred librt.so.1
Specifying this is an application thread or a WINE thread would be nice along with the resources that each thread had / has at the time would be as well. So if Thread c is a wine thread for kernel say WINE thread for kernel if that is possible.
Threads: process tid prio (all id:s are in hex) 0000000c 00000014 0 00000013 0 00000012 0 0000000e 0 0000000d 0 0000000f 00000016 0 00000015 0 00000011 0 00000010 0 0000002d 0000002e 0 00000033 (D) Z:\home\cahrendt\wine-git\dlls\ddraw\tests\ddraw_test.exe 00000034 0 <==
Why do we need another backtrace when we have it above?
Backtrace: =>1 0x602ec680 AttachmentTest+0xfa0() [/home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:1404] in ddraw_test (0x0033f2b8) 2 0x602f1735 func_dsurface+0x2175() [/home/cahrendt/wine-git/dlls/ddraw/tests/dsurface.c:2764] in ddraw_test (0x0033fde8) 3 0x60301bd8 run_test+0x128(name="dsurface.c") [/home/cahrendt/wine-git/dlls/ddraw/tests/../../../include/wine/test.h:454] in ddraw_test (0x0033fe28) 4 0x60302442 main+0x122(argc=<register ECX not in topmost frame>, argv=<register ECX not in topmost frame>) [/home/cahrendt/wine-git/dlls/ddraw/tests/../../../include/wine/test.h:503] in ddraw_test (0x0033fed8) 5 0x603025bb __wine_spec_exe_entry+0x5b(peb=0x7ffdf000) [/home/cahrendt/wine-git/dlls/winecrt0/exe_entry.c:36] in ddraw_test (0x0033ff08) 6 0x7b874207 start_process+0xc7(arg=(nil)) [/home/cahrendt/wine-git/dlls/kernel32/process.c:904] in kernel32 (0x0033ffe8)
On Thu, Sep 18, 2008 at 5:10 PM, Juan Lang juan.lang@gmail.com wrote:
Are you sure about that? When I configure with -Wall -Werror, I can't even configure properly; one gets the errors
configure: libxrandr development files not found, XRandr won't be supported.
This is precisely part of the problem with -Werror. You've now caused some configure checks to fail that, presumably, were passing before. Are they worth making warning-free? Some of the warnings may be in headers we don't control. What do we do then?
You're pretty negative on this idea, aren't you? Let me whittle away at it a bit before we reject it as impractical.
And if one blows past those, "make depend" fails. In fact, everything fails pretty spectacularly.
So a broken config leads to a broken compile?
No, there seem to be multiple severe breakages not caused by the slightly broken config. But let me gather some more data. - Dan
You're pretty negative on this idea, aren't you? Let me whittle away at it a bit before we reject it as impractical.
Sure, have fun.
I've used -Wall -Werror with success in the past with a medium-sized code base, and used it as a stick to keep people from checking in dodgy code. So it's not that it's always impractical. The issue I have is that a distributed development environment makes it hard, as does maintaining a portable code base linked against many possible versions of libraries. If a commit doesn't break on Alexandre's box, and it gets committed, but it does break on some other machine, what then? Who fixes it?
I think that individually, we can turn it on from time to time to see what pops up, just like running with valgrind. But forcing everyone to run with it seems draconian. --Juan
On Thu, Sep 18, 2008 at 5:54 PM, Juan Lang juan.lang@gmail.com wrote:
If a commit doesn't break on Alexandre's [and patchwatcher's] box, and it gets committed, but it does break on some other machine, what then? Who fixes it?
I suppose the answer there is "patchwatcher builds with multiple compilers" and/or "affected users with bum compilers turn off -Werror" and/or "we disable the errors that have high failure rates on some compilers".
I think that individually, we can turn it on from time to time to see what pops up, just like running with valgrind.
You obviously don't understand my plans with patchwatcher and valgrind :-)
But forcing everyone to run with it seems draconian.
What is "draconian" but a synonym for "dictatotorial"? Sounds like it would fit in just fine with Wine's maintainer's style :-) - Dan
I suppose the answer there is "patchwatcher builds with multiple compilers" and/or "affected users with bum compilers turn off -Werror" and/or "we disable the errors that have high failure rates on some compilers".
Sure, that makes as much sense as what I suggested. Consider me mollified, but I withhold the right for an "I told you so" if this leads to silly bugs and patches flying around ;-)
You obviously don't understand my plans with patchwatcher and valgrind :-)
Okay, now you're piquing my interest :)
What is "draconian" but a synonym for "dictatotorial"? Sounds like it would fit in just fine with Wine's maintainer's style :-)
You've got me there :) --Juan
"Dan Kegel" dank@kegel.com writes:
On Thu, Sep 18, 2008 at 5:54 PM, Juan Lang juan.lang@gmail.com wrote:
If a commit doesn't break on Alexandre's [and patchwatcher's] box, and it gets committed, but it does break on some other machine, what then? Who fixes it?
I suppose the answer there is "patchwatcher builds with multiple compilers" and/or "affected users with bum compilers turn off -Werror" and/or "we disable the errors that have high failure rates on some compilers".
No, the answer is "developers who really want their build to die on simple warnings can turn it on". We need Wine to build properly on a wide range of platforms and compiler versions, there's simply no way to ensure that the compile will always be warning free.
It is very important that everybody be able to build Wine and give it a try, even non-developers. That's why I'm so strict about portable code, proper configure checks, and conservative makefiles. There's nothing worse than downloading a project you want to play with and find out you can't even compile it. A compile failure is a serious bug, and we should do everything possible to avoid them. Making -Werror the default is the worse thing we could do in that respect.
On Fri, Sep 19, 2008 at 1:17 AM, Alexandre Julliard julliard@winehq.org wrote:
A compile failure is a serious bug, and we should do everything possible to avoid them.
Agreed, to an extent. A user who is trying to compile with a really whacky toolchain (say, a C compiler on an Amiga, a mainframe, or a wristwatch) should expect some errors, and we should not try to avoid those if they reflect real problems that need to be solved before wine can run properly.
And I also feel pretty strongly that compiler warnings have some value, and we should pay attention to them. Right now we're plugging our ears and going "la la la la la I can't hear you", and that seems a bit careless. As Wine aims for higher and higher quality levels, eventually we will have to change our ways here.
Making -Werror the default is the worse thing we could do in that respect.
If we do it in the right way, it could end up increasing our code quality quite a bit without inconveniencing any users.
The right way is to slowly fix all the warnings -- like Andrew Talbot is doing -- and slowly encourage all wine developers to crank up their warning levels to the max. Once we've voluntarily cleared out nearly all the warnings, we can then have a flag day to clean out all the rest, and switch -Werror on. That will keep errors from creeping back in.
"Dan Kegel" dank@kegel.com writes:
Agreed, to an extent. A user who is trying to compile with a really whacky toolchain (say, a C compiler on an Amiga, a mainframe, or a wristwatch) should expect some errors, and we should not try to avoid those if they reflect real problems that need to be solved before wine can run properly.
You don't need a wacky toolchain. All you need is a slightly old gcc, or a platform that not everybody has access to, like Mac OS. I periodically build on Mac OS and check for warnings, and there are always a few that creep in, like size_t printf formats. Do you really think Wine would be better off if it failed to build on Mac 90% of the time? Is printing a size_t with %d really so dangerous that it needs to break the build?
And I also feel pretty strongly that compiler warnings have some value, and we should pay attention to them. Right now we're plugging our ears and going "la la la la la I can't hear you", and that seems a bit careless. As Wine aims for higher and higher quality levels, eventually we will have to change our ways here.
That's absolutely not true. We are trying very hard to avoid warnings, I don't commit patches that add warnings, and many people are sending fixes when they find warnings on their platform. That doesn't mean they should break the build.
If we do it in the right way, it could end up increasing our code quality quite a bit without inconveniencing any users.
It wouldn't make any difference to code quality, since we are already warnings-free on most standard builds.
The right way is to slowly fix all the warnings -- like Andrew Talbot is doing -- and slowly encourage all wine developers to crank up their warning levels to the max. Once we've voluntarily cleared out nearly all the warnings, we can then have a flag day to clean out all the rest, and switch -Werror on. That will keep errors from creeping back in.
Everybody is free to build their tree with -Werror, but there's no way that it will become the default.
On Fri, Sep 19, 2008 at 04:35:45PM +0200, Alexandre Julliard wrote:
"Dan Kegel" dank@kegel.com writes:
Agreed, to an extent. A user who is trying to compile with a really whacky toolchain (say, a C compiler on an Amiga, a mainframe, or a wristwatch) should expect some errors, and we should not try to avoid those if they reflect real problems that need to be solved before wine can run properly.
You don't need a wacky toolchain. All you need is a slightly old gcc, or a platform that not everybody has access to, like Mac OS. I periodically
Or a newer gcc, or -O3, or -D_FORTIFY_SOURCE=2 (default in SUSE and Redhat) etc etc.
If someone wants -Werror, he can always set CFLAGS for himself ;)
Ciao, Marcus
OK, I just ran with -Wall -Werror, and got a grand total of four errors:
freetype.c:1051: warning: 'name.string_len' is used uninitialized in this function Looks bogus.
engine.c:2128: warning: 'str' may be used uninitialized in this function Bogus.
context.c:80: warning: 'update_minfilter' may be used uninitialized in this function context.c:80: warning: 'update_magfilter' may be used uninitialized in this function
Real! Looks like one slipped by yesterday: http://source.winehq.org/git/wine.git/?a=commitdiff;h=9533a5cbbf76cf4b9013ed...
That's not a bad haul.
Now, if everyone were building with -O2 -Wall -Werror, that would have been caught sooner. But hey, if Alexandre doesn't like the idea, I guess it won't fly.
So my alternate suggestion is for patchwatcher to reject patches that fail with -Werror. (And work around the couple of bogus errors I listed above.)
That gets us the desired benefits without running into the problems Alexandre and Marcus pointed out.
How's that sound? - Dan
On Fri, 19 Sep 2008, Dan Kegel wrote: [...]
So my alternate suggestion is for patchwatcher to reject patches that fail with -Werror. (And work around the couple of bogus errors I listed above.)
It does not even have to build with -Werror, just to grep the log for warnings, and warn if there are new ones.
The benefit is that because the compilation succeeded, you still get to test the patch for other errors, notably by running the conformance tests. Then the patchwatcher report can complain about both warnings, and any conformance regressions that it identified.
In fact, we could even have patchwatcher compile with multiple compilers, a bleeding one and an older one, gcc 2.95 say, so we catch these nameless unions earlier ;-) (but I don't mind continuing with my weekly gcc 2.95 build)
-
Alexandre Julliard -
Chris Ahrendt -
Dan Kegel -
Francois Gouget -
James Hawkins -
Juan Lang -
Marcus Meissner -
Steven Edwards