MemoryValue always expects a signed offset. I ran into this for another patch, and it seems like an unlikely but really bad bug to run into. I'll audit our code and fix MemoryValue as well as other places which use signed integer. They should either fail to compile if given unsigned, or check and trap at runtime if we'd hit implementation-defined behavior.
Created attachment 307101 [details] patch
Attachment 307101 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:174: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:174: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:175: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:175: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:176: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:176: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:176: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 7 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 307101 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=307101&action=review > Source/JavaScriptCore/b3/B3MemoryValue.cpp:83 > + MemoryValue::MemoryValue(MemoryValue::MemoryValueLoad, Kind kind, Type type, Origin origin, Value* pointer, MemoryValue::OffsetType offset, HeapRange range, HeapRange fenceRange) undo indentation please.
Comment on attachment 307101 [details] patch Actually, cq- because I also want you to undo that indentation change in MemoryValue that Keith pointed out.
As ugly as this is, I think it's good to force people to be precise about how their offset becomes an int32. I have a question: what does this do to AtomicValue?
Created attachment 307169 [details] patch Here's an update that does AtomicValue and other offsets! I also added C++17's std::conjunction to WTF because this was becoming copy-pasta. The heart of this change is now in B3Value.h.
Attachment 307169 [details] did not pass style-queue: ERROR: Source/WTF/wtf/StdLibExtras.h:516: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:517: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:518: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:519: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WTF/wtf/StdLibExtras.h:520: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:88: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:88: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:89: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3AtomicValue.h:89: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/air/AirArg.h:549: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3Value.h:297: std::enable_if::type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/b3/B3Value.h:298: std::enable_if::type is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/JavaScriptCore/b3/B3Value.h:300: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:146: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:146: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:147: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:147: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:148: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:148: The parameter name "offset" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:148: The parameter name "range" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 20 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 307258 [details] patch Fix style and MSVC build.
Comment on attachment 307258 [details] patch Rejecting attachment 307258 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 307258, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/3549717
Created attachment 307259 [details] patch Forgot to update "oops", as always.
The commit-queue encountered the following flaky tests while processing attachment 307259 [details]: webrtc/captureCanvas-webrtc.html bug 170870 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
Comment on attachment 307259 [details] patch Clearing flags on attachment: 307259 Committed r215407: <http://trac.webkit.org/changeset/215407>
All reviewed patches have been landed. Closing bug.