We have a broken build w/ gcc 4.4.5 and libc 2.12.1 on Ubuntu 10.10 due to a warn_unused_result attribute applied on fscanf, and a misuse in change set 75709 (http://trac.webkit.org/changeset/75709/trunk/Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp) where the return value is not checked/used. Patch to come shortly.
Created attachment 79183 [details] Proposed patch Added another compiler flag to ignore unused return values, and to not treat those warnings as errors. Also removed duplicate -Wreturn-type entry.
It isn't Qt specific bug, but the return value of fscanf() should be always checked to avoid strange bugs. I prefer check the return value of fscanf() to paper over the problem with -Wreturn-type compiler option.
Comment on attachment 79183 [details] Proposed patch r- due to my last comment. And rs=me to remove the redundant -Wreturn-type.
(In reply to comment #2) > It isn't Qt specific bug, but the return value of fscanf() should be always checked to avoid strange bugs. I prefer check the return value of fscanf() to paper over the problem with -Wreturn-type compiler option. That was my original patch :) I know it's not Qt specific regression, but it is (at least) effecting the Qt Linux build. It got past the build bots somehow ;) I'll revise the patch.
452 static void maybeModifyVMPoolSize() 453 { 454 FILE* fp = fopen("/proc/sys/vm/overcommit_memory", "r"); 455 if (!fp) 456 return; 457 458 unsigned overcommit = 0; 459 fscanf(fp, "%u", &overcommit); Here the return value of fscanf must be true, so we can ignore it here, but we shouldn't do it in general. I think the similar "check" to http://trac.webkit.org/changeset/66984 would be good: + // We have to use the return value of fwrite to avoid "ignoring return value" gcc warning + // See https://bugs.webkit.org/show_bug.cgi?id=45384 for details. - fscanf(fp, "%u", &overcommit); + if (fscanf(fp, "%u", &overcommit)) {}
Once we should find a general solution for https://bugs.webkit.org/show_bug.cgi?id=45384 ... Any volunteer? :)
Created attachment 79209 [details] Proposed patch Check the return value instead of disabling a gcc warning flag, to benefit all ports.
Comment on attachment 79209 [details] Proposed patch LGTM, r=me.
Comment on attachment 79209 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=79209&action=review > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:466 > + // We have to use the return value of fscanf to avoid "ignoring return value" gcc warning > + // See https://bugs.webkit.org/show_bug.cgi?id=45384 for details. This comment isn’t needed. We need to check the return value of fscanf because that’s how you use fscanf! I don’t think we need a reference to this bug. > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:467 > + if (fscanf(fp, "%u", &overcommit)) { This is not right. We need to check if the value is 1 rather than checking for 0. This function can return EOF, for example. Since we’re going to the trouble to check here, I suggest doing the check correctly.
Spot on Darin, will revise.
Created attachment 79210 [details] Proposed patch explicitly check for 1, as checking for "anything but zero" is incorrect as EOF could be returned.
Created attachment 79211 [details] Proposed patch broken patch
Comment on attachment 79211 [details] Proposed patch r=me, based on Darin's comment. Thanks for the advice, this patch is better.
Jarred, if you realized that you upload a bad patch next time, just remove r? flag and set obsolete flag instead of giving a "self-r-" .
(In reply to comment #14) > Jarred, if you realized that you upload a bad patch next time, just > remove r? flag and set obsolete flag instead of giving a "self-r-" . Will do, I just like criticizing myself :)
Comment on attachment 79211 [details] Proposed patch Clearing flags on attachment: 79211 Committed r75992: <http://trac.webkit.org/changeset/75992>
All reviewed patches have been landed. Closing bug.