Bug 52585 – REGRESSION(r75709): Return value of fscanf() shouldn't be ignored.

• Component: Platform
• Status: RESOLVED FIXED

[Jarred Nicholls]

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.

[Jarred Nicholls]

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.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Comment on attachment 79183 [details]
Proposed patch

r- due to my last comment.

And rs=me to remove the redundant -Wreturn-type.

[Jarred Nicholls]

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

[Csaba Osztrogonác]

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)) {}

[Csaba Osztrogonác]

Once we should find a general solution for https://bugs.webkit.org/show_bug.cgi?id=45384 ... Any volunteer? :)

[Jarred Nicholls]

Created attachment 79209 [details]
Proposed patch

Check the return value instead of disabling a gcc warning flag, to benefit all ports.

[Csaba Osztrogonác]

Comment on attachment 79209 [details]
Proposed patch

LGTM, r=me.

[Darin Adler]

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.

[Jarred Nicholls]

Spot on Darin, will revise.

[Jarred Nicholls]

Created attachment 79210 [details]
Proposed patch

explicitly check for 1, as checking for "anything but zero" is incorrect as EOF could be returned.

[Jarred Nicholls]

Created attachment 79211 [details]
Proposed patch

broken patch

[Csaba Osztrogonác]

Comment on attachment 79211 [details]
Proposed patch

r=me, based on Darin's comment. Thanks for the advice, this patch is better.

[Csaba Osztrogonác]

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

[Jarred Nicholls]

(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 :)

[WebKit Commit Bot]

Comment on attachment 79211 [details]
Proposed patch

Clearing flags on attachment: 79211

Committed r75992: <http://trac.webkit.org/changeset/75992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.