Bug 162884

Created attachment 290540 [details] patch

Comment on attachment 290540 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=290540&action=review > Source/JavaScriptCore/wasm/WASMParser.h:95 > + if (m_sourceLength < 4 || m_offset >= m_sourceLength - 4) Why this change?

Comment on attachment 290540 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=290540&action=review > Source/JavaScriptCore/wasm/WASMOps.h:174 > +template<typename Int> inline bool isValidOpType(Int i) { return i <= 188; } Aren't there gaps in the WASM opcodes?

Comment on attachment 290540 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=290540&action=review >> Source/JavaScriptCore/wasm/WASMOps.h:174 >> +template<typename Int> inline bool isValidOpType(Int i) { return i <= 188; } > > Aren't there gaps in the WASM opcodes? Yup. I'm planning on generating tables (for this and other things) as the next patch. Baby steps :) >> Source/JavaScriptCore/wasm/WASMParser.h:95 >> + if (m_sourceLength < 4 || m_offset >= m_sourceLength - 4) > > Why this change? It avoids overflowing m_sourceLength. That's not likely to happen with the code as-is, but I've seen random refactorings in the past lead to wraparound in later code.

Created attachment 290617 [details] patch Add opcode validity table.

Attachment 290617 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WASMOps.h:177: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 290619 [details] patch Fix style, update changelog.

Comment on attachment 290619 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=290619&action=review r=me. > Source/JavaScriptCore/wasm/WASMOps.h:180 > + static const uint8_t valid[] = { 0xff, 0x8f, 0xff, 0x2, 0xff, 0xff, 0x7f, 0xa, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x1f }; > + return 0 <= i && i <= 188 && (valid[i / 8] & (1 << (i % 8))); I think this would be better as a constexpr WTF::BitMap but I'm not going to block this patch on it. If you don't make it a WTF::BitMap, you should put a comment saying that the valid array is a bit map.

(In reply to comment #8) > Comment on attachment 290619 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290619&action=review > > r=me. > > > Source/JavaScriptCore/wasm/WASMOps.h:180 > > + static const uint8_t valid[] = { 0xff, 0x8f, 0xff, 0x2, 0xff, 0xff, 0x7f, 0xa, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x1f }; > > + return 0 <= i && i <= 188 && (valid[i / 8] & (1 << (i % 8))); > > I think this would be better as a constexpr WTF::BitMap but I'm not going to > block this patch on it. If you don't make it a WTF::BitMap, you should put a > comment saying that the valid array is a bit map. I sort of don't mind this. It has no magic, which is refreshing.

(In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 290619 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=290619&action=review > > > > r=me. > > > > > Source/JavaScriptCore/wasm/WASMOps.h:180 > > > + static const uint8_t valid[] = { 0xff, 0x8f, 0xff, 0x2, 0xff, 0xff, 0x7f, 0xa, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x1f }; > > > + return 0 <= i && i <= 188 && (valid[i / 8] & (1 << (i % 8))); > > > > I think this would be better as a constexpr WTF::BitMap but I'm not going to > > block this patch on it. If you don't make it a WTF::BitMap, you should put a > > comment saying that the valid array is a bit map. > > I sort of don't mind this. It has no magic, which is refreshing. I don't really know how much this matters but a switch statement would have been fine. Clang will optimize it to a bitvector lookup. So, it comes down to a question of what is clearer. I think that the switch statement is clearer, but I don't care that much.

The following code: ``` bool isValidOpType(std::size_t i) { switch (i) { case 0: return true; case 1: return true; case 2: return true; case 3: return true; case 4: return true; case 5: return true; case 6: return true; case 7: return true; case 8: return true; case 9: return true; case 10: return true; case 11: return true; case 12: return false; case 13: return false; case 14: return false; case 15: return true; case 16: return true; case 17: return true; case 18: return true; case 19: return true; case 20: return true; case 21: return true; case 22: return true; case 23: return true; case 24: return false; case 25: return true; case 26: return false; case 27: return false; case 28: return false; case 29: return false; case 30: return false; case 31: return false; case 32: return true; case 33: return true; case 34: return true; case 35: return true; case 36: return true; case 37: return true; case 38: return true; case 39: return true; case 40: return true; case 41: return true; case 42: return true; case 43: return true; case 44: return true; case 45: return true; case 46: return true; case 47: return true; case 48: return true; case 49: return true; case 50: return true; case 51: return true; case 52: return true; case 53: return true; case 54: return true; case 55: return false; case 56: return false; case 57: return true; case 58: return false; case 59: return true; case 60: return false; case 61: return false; case 62: return false; case 63: return false; case 64: return true; case 65: return true; case 66: return true; case 67: return true; case 68: return true; case 69: return true; case 70: return true; case 71: return true; case 72: return true; case 73: return true; case 74: return true; case 75: return true; case 76: return true; case 77: return true; case 78: return true; case 79: return true; case 80: return true; case 81: return true; case 82: return true; case 83: return true; case 84: return true; case 85: return true; case 86: return true; case 87: return true; case 88: return true; case 89: return true; case 90: return true; case 91: return true; case 92: return true; case 93: return true; case 94: return true; case 95: return true; case 96: return true; case 97: return true; case 98: return true; case 99: return true; case 100: return true; case 101: return true; case 102: return true; case 103: return true; case 104: return true; case 105: return true; case 106: return true; case 107: return true; case 108: return true; case 109: return true; case 110: return true; case 111: return true; case 112: return true; case 113: return true; case 114: return true; case 115: return true; case 116: return true; case 117: return true; case 118: return true; case 119: return true; case 120: return true; case 121: return true; case 122: return true; case 123: return true; case 124: return true; case 125: return true; case 126: return true; case 127: return true; case 128: return true; case 129: return true; case 130: return true; case 131: return true; case 132: return true; case 133: return true; case 134: return true; case 135: return true; case 136: return true; case 137: return true; case 138: return true; case 139: return true; case 140: return true; case 141: return true; case 142: return true; case 143: return true; case 144: return true; case 145: return true; case 146: return true; case 147: return true; case 148: return true; case 149: return true; case 150: return true; case 151: return true; case 152: return true; case 153: return true; case 154: return true; case 155: return true; case 156: return true; case 157: return true; case 158: return true; case 159: return true; case 160: return true; case 161: return true; case 162: return true; case 163: return true; case 164: return true; case 165: return true; case 166: return true; case 167: return true; case 168: return true; case 169: return true; case 170: return true; case 171: return true; case 172: return true; case 173: return true; case 174: return true; case 175: return true; case 176: return true; case 177: return true; case 178: return true; case 179: return true; case 180: return true; case 181: return true; case 182: return true; case 183: return true; case 184: return true; case 185: return true; case 186: return true; case 187: return true; case 188: return true; default: return false; } } ``` Codegens as shitty code on clang 3.9: ``` isValidOpType(unsigned long): # @isValidOpType(unsigned long) cmp rdi, 188 ja .LBB0_2 mov al, 1 jmp qword ptr [8*rdi + .LJTI0_0] .LBB0_3: ret .LBB0_2: xor eax, eax jmp .LBB0_3 .LJTI0_0: .quad .LBB0_3 .quad .LBB0_3 # and so on... ``` Same for ARMv7: ``` isValidOpType(int): @ @isValidOpType(int) mov r1, r0 cmp r1, #188 bhi .LBB0_4 adr r2, .LJTI0_0 lsl r1, r1, #2 mov r0, #1 ldr pc, [r1, r2] .LJTI0_0: .long .LBB0_3 .long .LBB0_3 # and so on... ``` In general I like to trust my compiler... but not for switches. LLVM's changed its approach over time, it generates good code in some cases, but not always and isn't consistent from one version to another IMO. I usually would just go fix LLVM... but meh, switches are fraught with peril. I'll add a comment though :-)

Created attachment 290640 [details] patch Add bitset comment.

Comment on attachment 290640 [details] patch Clearing flags on attachment: 290640 Committed r206794: <http://trac.webkit.org/changeset/206794>

All reviewed patches have been landed. Closing bug.