Summary: | [JSC] Range of cache flush is not guaranteed by Linux kernel | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hojong Han <hojong.han> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | barraclough, fpizlo, ggaren, hausmann, loki, msaboff, webkit.review.bot, zherczeg | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Hojong Han
2012-02-02 21:28:30 PST
Created attachment 130982 [details]
Patch
Attachment 130982 [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/jit/ExecutableAllocator.h:202: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 130982 [details]
Patch
Is size_t the right type to use for startPage and endPage? It seems safer to use intptr_t or something.
Also it seems to me that you would be better off doing
uintptr_t start = reinterpret_cast<uintptr_t>(code) & ~(pageSize() - 1);
and just adding pageSize() each round instead of doing the divide-and-multiply dance. Dunno.
I'm not a reviewer, so please take these comments with a grain of salt.
(In reply to comment #3) > (From update of attachment 130982 [details]) > Is size_t the right type to use for startPage and endPage? It seems safer to use intptr_t or something. > > Also it seems to me that you would be better off doing > > uintptr_t start = reinterpret_cast<uintptr_t>(code) & ~(pageSize() - 1); > > and just adding pageSize() each round instead of doing the divide-and-multiply dance. Dunno. > > I'm not a reviewer, so please take these comments with a grain of salt. Thank you. It's really necessary salt for me. I'm going to put it into new patch. Phil, does this mean that munmap will eventually fail as well? (In reply to comment #5) > Phil, does this mean that munmap will eventually fail as well? Nope. We munmap one page at a time. Created attachment 131264 [details]
Patch
Attachment 131264 [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/jit/ExecutableAllocator.h:202: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 131268 [details]
Patch
Attachment 131268 [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/jit/ExecutableAllocator.h:202: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 131268 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131268&action=review > Source/JavaScriptCore/jit/ExecutableAllocator.h:209 > + current = next; You can eliminate the "next" variable if you change this line to "current += pageSize()". > Source/JavaScriptCore/jit/ExecutableAllocator.h:211 > + } while (end >= current); In C++, "end" traditionally means one-past-the-end. That's not the case here, so I don't think you should use the word "end". I'd suggest "currentPage" and "lastPage" for variable names. Created attachment 131496 [details]
Patch
Attachment 131496 [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/jit/ExecutableAllocator.h:201: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > (From update of attachment 131268 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131268&action=review > > > Source/JavaScriptCore/jit/ExecutableAllocator.h:209 > > + current = next; > > You can eliminate the "next" variable if you change this line to "current += pageSize()". > > > Source/JavaScriptCore/jit/ExecutableAllocator.h:211 > > + } while (end >= current); > > In C++, "end" traditionally means one-past-the-end. That's not the case here, so I don't think you should use the word "end". > > I'd suggest "currentPage" and "lastPage" for variable names. Thanks for your kind comments. I reflect your advice. Comment on attachment 131496 [details]
Patch
r=me
Comment on attachment 131496 [details] Patch Clearing flags on attachment: 131496 Committed r110792: <http://trac.webkit.org/changeset/110792> All reviewed patches have been landed. Closing bug. |