Summary: | REGRESSION(r75709): Return value of fscanf() shouldn't be ignored. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jarred Nicholls <jarred> | ||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Blocker | CC: | barraclough, commit-queue, ossy, xan.lopez | ||||||||||
Priority: | P1 | Keywords: | Qt | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 45384 | ||||||||||||
Attachments: |
|
Description
Jarred Nicholls
2011-01-17 09:44:22 PST
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. |