Refactor OpcodeTraits to support the possibility of having 2-byte WASM opcode ids in bytecode streams

• Component: New Bugs
• Status: RESOLVED FIXED

[Justin Michaud]

Refactor OpcodeTraits to support the possibility of having 2-byte WASM opcode ids in bytecode streams

[Justin Michaud]

Created attachment 453547 [details]
Patch

[Justin Michaud]

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]

Created attachment 453549 [details]
Patch

[Justin Michaud]

Created attachment 453550 [details]
Patch

[Justin Michaud]

Created attachment 453554 [details]
Patch

[Keith Miller]

Oooh, I think this will be really nice for cleaning up my bytecode metadata pointer patch!

[Justin Michaud]

Created attachment 453570 [details]
Patch

[Justin Michaud]

Created attachment 453625 [details]
Patch

[Keith Miller]

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 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]

Created attachment 453633 [details]
Patch

[Keith Miller]

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 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]

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]

<rdar://problem/89734679>