WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
237347
Refactor OpcodeTraits to support the possibility of having 2-byte WASM opcode ids in bytecode streams
https://bugs.webkit.org/show_bug.cgi?id=237347
Summary
Refactor OpcodeTraits to support the possibility of having 2-byte WASM opcode...
Justin Michaud
Reported
2022-03-01 16:09:39 PST
Refactor OpcodeTraits to support the possibility of having 2-byte WASM opcode ids in bytecode streams
Attachments
Patch
(365.97 KB, patch)
2022-03-01 16:23 PST
,
Justin Michaud
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(366.29 KB, patch)
2022-03-01 16:45 PST
,
Justin Michaud
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(366.10 KB, patch)
2022-03-01 16:51 PST
,
Justin Michaud
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(366.17 KB, patch)
2022-03-01 17:03 PST
,
Justin Michaud
no flags
Details
Formatted Diff
Diff
Patch
(366.25 KB, patch)
2022-03-01 21:57 PST
,
Justin Michaud
no flags
Details
Formatted Diff
Diff
Patch
(365.48 KB, patch)
2022-03-02 09:17 PST
,
Justin Michaud
no flags
Details
Formatted Diff
Diff
Patch
(364.78 KB, patch)
2022-03-02 10:23 PST
,
Justin Michaud
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Justin Michaud
Comment 1
2022-03-01 16:23:36 PST
Created
attachment 453547
[details]
Patch
Justin Michaud
Comment 2
2022-03-01 16:26:44 PST
Comment on
attachment 453547
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=453547&action=review
> Source/JavaScriptCore/jit/JITExceptions.cpp:66 > + std::visit([&](auto* pc) {
I am calling this out as an area that needs special review
Justin Michaud
Comment 3
2022-03-01 16:45:25 PST
Created
attachment 453549
[details]
Patch
Justin Michaud
Comment 4
2022-03-01 16:51:44 PST
Created
attachment 453550
[details]
Patch
Justin Michaud
Comment 5
2022-03-01 17:03:15 PST
Created
attachment 453554
[details]
Patch
Keith Miller
Comment 6
2022-03-01 17:20:03 PST
Oooh, I think this will be really nice for cleaning up my bytecode metadata pointer patch!
Justin Michaud
Comment 7
2022-03-01 21:57:37 PST
Created
attachment 453570
[details]
Patch
Justin Michaud
Comment 8
2022-03-02 09:17:42 PST
Created
attachment 453625
[details]
Patch
Keith Miller
Comment 9
2022-03-02 09:51:27 PST
Comment on
attachment 453625
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=453625&action=review
r=me with comments.
> Source/JavaScriptCore/bytecode/BytecodeDumper.h:-135 > -class BytecodeDumper final : public JSC::BytecodeDumper<FunctionCodeBlockGenerator> {
How come we don't need this one anymore?
> Source/JavaScriptCore/bytecode/BytecodeDumper.h:151 > + int outOfLineJumpOffset(JSInstructionStream::Offset) const override;
Why is this JSInstructionStream? I'm assuming it's just a copy paste error and the two `Offset` types are the same in practice?
> Source/JavaScriptCore/bytecode/BytecodeGeneratorification.cpp:45 > + typename JSInstructionStream::Offset point { 0 };
Do you need `typename` here? I thought that wasn't necessary as of C++20. Ditto a bunch of other places.
Justin Michaud
Comment 10
2022-03-02 10:10:38 PST
Comment on
attachment 453625
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=453625&action=review
>> Source/JavaScriptCore/bytecode/BytecodeDumper.h:-135 >> -class BytecodeDumper final : public JSC::BytecodeDumper<FunctionCodeBlockGenerator> { > > How come we don't need this one anymore?
I'm not sure what you mean?
>> Source/JavaScriptCore/bytecode/BytecodeDumper.h:151 >> + int outOfLineJumpOffset(JSInstructionStream::Offset) const override; > > Why is this JSInstructionStream? I'm assuming it's just a copy paste error and the two `Offset` types are the same in practice?
Good catch!
>> Source/JavaScriptCore/bytecode/BytecodeGeneratorification.cpp:45 >> + typename JSInstructionStream::Offset point { 0 }; > > Do you need `typename` here? I thought that wasn't necessary as of C++20. Ditto a bunch of other places.
Yeah probably. I'll review the spec.
Justin Michaud
Comment 11
2022-03-02 10:23:12 PST
Created
attachment 453633
[details]
Patch
Keith Miller
Comment 12
2022-03-02 11:06:54 PST
Comment on
attachment 453625
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=453625&action=review
>>> Source/JavaScriptCore/bytecode/BytecodeDumper.h:-135 >>> -class BytecodeDumper final : public JSC::BytecodeDumper<FunctionCodeBlockGenerator> { >> >> How come we don't need this one anymore? > > I'm not sure what you mean?
I meant this subclass override but I now realize that question doesn't make any sense since it's just a different superclass now.
Keith Miller
Comment 13
2022-03-02 11:06:55 PST
Comment on
attachment 453625
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=453625&action=review
>>> Source/JavaScriptCore/bytecode/BytecodeDumper.h:-135 >>> -class BytecodeDumper final : public JSC::BytecodeDumper<FunctionCodeBlockGenerator> { >> >> How come we don't need this one anymore? > > I'm not sure what you mean?
I meant this subclass override but I now realize that question doesn't make any sense since it's just a different superclass now.
EWS
Comment 14
2022-03-02 22:50:56 PST
Committed
r290768
(
248012@main
): <
https://commits.webkit.org/248012@main
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 453633
[details]
.
Radar WebKit Bug Importer
Comment 15
2022-03-02 22:51:18 PST
<
rdar://problem/89734679
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug