Summary: | If memset gets inlined at WebKit/Source/JavaScriptCore/wtf/Vector.h, it may produce a warning | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | asharif.tools | ||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, ap, eric, thakis, vapier, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
asharif.tools
2012-02-13 11:51:50 PST
Created attachment 126836 [details]
Fixes the problem.
You usually upload patches with "Reviewed by NOBODY (OOPS)", and the scripts put in a name after someone reviewed this. I'm not a webkit reviewer, so I can't review this. I also don't think this should be done, the error sounds like a toolchain bug. Comment on attachment 126836 [details]
Fixes the problem.
This makes uninitializedFill slower for the benefit of a platform specific tool. There must be a way to shut down the warning that doesn't have runtime costs.
Created attachment 126899 [details]
Should not have any runtime overhead with this patch at -O2.
This patch is now checking if dstEnd - dst is a constant before doing the 0 check. If dstEnd - dst is not a constant, the first part of the if condition is true and the whole if condition should be folded away.
Attachment 126899 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/Vector.h:185: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4]
Source/JavaScriptCore/wtf/Vector.h:185: Extra space for operator ! [whitespace/operators] [4]
Source/JavaScriptCore/wtf/Vector.h:186: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 3 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 126901 [details]
Fixed style warnings.
Comment on attachment 126901 [details] Fixed style warnings. View in context: https://bugs.webkit.org/attachment.cgi?id=126901&action=review This might be OK, but the whole warning seems like something that should be just globally disabled somehow. > Source/JavaScriptCore/wtf/Vector.h:186 > + if (!__builtin_constant_p(dstEnd - dst) > + || (!(dstEnd - dst))) Please don't wrap this line. Created attachment 126995 [details]
Fixed line-wrap issue.
Attachment 126995 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
Updating OpenSource
From git://git.webkit.org/WebKit
+ a9730c4...fb50dbd master -> origin/master (forced update)
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
<stdin>:1647: trailing whitespace.
<stdin>:1657: trailing whitespace.
<stdin>:1672: trailing whitespace.
return 0;
<stdin>:1674: trailing whitespace.
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168753 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/wk2/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/css/CSSCalculationValue.cpp
Auto-merging Source/WebCore/css/CSSCalculationValue.h
Auto-merging Source/WebCore/css/CSSParser.cpp
Auto-merging Source/WebKit/mac/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".
rebase refs/remotes/origin/master: command returned error: 1
Died at Tools/Scripts/update-webkit line 164.
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 127304 [details]
Proposed fix.
I rebased this patch.
Ping? Any updates on this? If this solution is not acceptable, let me know so we can close this bug. One question is whether there is a less intrusive way to disable this warning. A quick web search reveals that "With -D_FORTIFY_SOURCE=2 ... some conforming programs might fail." <http://gcc.gnu.org/ml/gcc-patches/2004-09/msg02055.html>. So, I'm not sure if we want to subscribe to supporting this mode. A discussion on webkit-dev mailing list may clarify things. while it's true that fortify breaks code that is correct from the language/runtime pov, we're talking about snippets that arguably go against style tastes. such as: struct { char a[2]; char b; char d[3]; } foo; strcpy(&foo.a[0], "aabdd"); many distros now build with fortify enabled by default for pretty much all packages (Debian/Ubuntu/Fedora/Redhat/Gentoo). it has certainly been a much bigger benefit than the handful of downsides. the proposed patch probably should have a comment in there explaining why this magic check is active. or perhaps replace/augment the GCC define with _FORTIFY_SOURCE so that the code is self-documenting. > replace/augment the GCC define with _FORTIFY_SOURCE so that the code is self-documenting
I like this idea (augmenting would likely be more self-documenting).
Created attachment 127643 [details]
Proposed fix.
Added ifdef _FORTIFY_SOURCE around the patch.
I kept the COMPILER(GCC) because __builtin_constant_p() may not be available on other compilers.
Comment on attachment 127643 [details]
Proposed fix.
I think that this is OK to land, but please merge the #ifs in one line:
#if COMPILER(GCC) && defined(_FORTIFY_SOURCE)
Oh, and please mention Fortify in ChangeLog. Created attachment 127659 [details]
Proposed fix.
Addressed reviewers' comments.
Comment on attachment 127659 [details]
Proposed fix.
Thank you!
Comment on attachment 127659 [details] Proposed fix. Rejecting attachment 127659 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/JavaScriptCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/11543290 I don't understand why the commit queue freaked out. Adam, Eric, do you see what's wrong? Comment on attachment 127659 [details] Proposed fix. View in context: https://bugs.webkit.org/attachment.cgi?id=127659&action=review > Source/JavaScriptCore/ChangeLog:10 > + Reviewed by NOBODY (OOPS). I believe it normally has a ! after the OOPS. http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm#L1399 Our OOPS! convention in WebKit is not very discoverable. I think we should consider changing it to DO_NOT_COMMIT or similar. Created attachment 127682 [details]
Proposed fix.
Rebased patch. Added exclamation point after OOPS.
Comment on attachment 127682 [details] Proposed fix. Clearing flags on attachment: 127682 Committed r108153: <http://trac.webkit.org/changeset/108153> All reviewed patches have been landed. Closing bug. |