Summary: | Verify the contents of AssemblerBuffer on arm64e | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, fpizlo, ggaren, gskachkov, guijemont, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Saam Barati
2018-09-27 14:43:19 PDT
Created attachment 351006 [details]
WIP
Created attachment 351025 [details]
patch
Created attachment 351029 [details]
patch
Attachment 351029 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:964: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:992: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:196: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:197: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:198: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/ARM64Assembler.h:333: Wrong number of spaces before statement. (expected: 16) [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AssemblerBuffer.h:44: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AssemblerBuffer.h:178: Wrong number of spaces before statement. (expected: 20) [whitespace/indent] [4]
Total errors found: 8 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 351029 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=351029&action=review LGTM but I would like to read through it one more time tonight before r+ing. Some comments and suggestions for now. > Source/JavaScriptCore/assembler/ARM64Assembler.h:329 > + ARM64Assembler( > +#if CPU(ARM64E) > + unsigned randomNumber > +#endif > + ) nit: How about express this as: ARM64Assembler(unsigned randomNumber = 0) I think the compiler will optimize the argument out if not used. > Source/JavaScriptCore/assembler/ARM64Assembler.h:337 > { > } ... and add UNUSED_PARAM(randomNumber) here? > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:994 > + , m_assembler( > +#if CPU(ARM64E) > + random() > +#endif > + ) nit: this looks so wonky. IMHO, I think it's easier to read if you express it as: #if CPU(ARM64E) , m_assembler(random()) #else , m_assembler() #endif > Source/JavaScriptCore/assembler/AssemblerBuffer.h:174 > + AssemblerBuffer( > +#if CPU(ARM64E) > + unsigned randomNumber > +#endif > + ) nit: consider expressing as: AssemblerBuffer(unsigned randomNumber = 0) > Source/JavaScriptCore/assembler/AssemblerBuffer.h:181 > { > } ... and add UNUSED_PARAM(randomNumber) here? > Source/JavaScriptCore/assembler/AssemblerBuffer.h:291 > + ARM64EHash hash() const > + { > + return m_hash; > + } nit: just make this one line: ARM64EHash hash() const { return hash; } > Source/JavaScriptCore/assembler/AssemblerBuffer.h:294 > +#if !CPU(ARM64) // If we were to define this on arm64e, we'd need a way to update the hash as we wrote directly into the buffer. /wrote/write/. > Source/JavaScriptCore/assembler/AssemblerBuffer.h:298 > + void* data() const > + { > + return m_storage.buffer(); > + } Ditto: can be single line. > Source/JavaScriptCore/assembler/LinkBuffer.cpp:211 > +#if CPU(ARM64E) > + unsigned index = readPtr; > +#endif > + > + for (size_t i = 0; i < bytes; i += sizeof(InstructionType)) { > + InstructionType insn = *src++; > +#if CPU(ARM64E) > + verifyUncompactedHash.update(insn, index); > + index += sizeof(InstructionType); > +#endif > + *dst++ = insn; > + } How about put this whole thing in #if CPU(ARM64E), and have an #else memcpy(dst, src, bytes); > Source/JavaScriptCore/assembler/LinkBuffer.cpp:220 > +#if CPU(ARM64E) > + if (verifyUncompactedHash.hash() != assemblerBufferHash.hash()) { > + dataLogLn("Hashes don't match: ", RawPointer(bitwise_cast<void*>(verifyUncompactedHash.hash())), " ", RawPointer(bitwise_cast<void*>(assemblerBufferHash.hash()))); > + dataLogLn("Crashing!"); > + CRASH(); > + } > +#endif You can move this blob into the #if CPU(ARM64E) blob above right after the for loop. Comment on attachment 351029 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=351029&action=review >> Source/JavaScriptCore/assembler/ARM64Assembler.h:329 >> + ) > > nit: How about express this as: > ARM64Assembler(unsigned randomNumber = 0) > > I think the compiler will optimize the argument out if not used. I think that's worse than this. >> Source/JavaScriptCore/assembler/AssemblerBuffer.h:174 >> + ) > > nit: consider expressing as: > > AssemblerBuffer(unsigned randomNumber = 0) same Comment on attachment 351029 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=351029&action=review >> Source/JavaScriptCore/assembler/LinkBuffer.cpp:211 >> + } > > How about put this whole thing in #if CPU(ARM64E), and have an #else > memcpy(dst, src, bytes); I'd rather have this code be shared on all archs that do branch compaction. Created attachment 351035 [details]
patch
Attachment 351035 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:964: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:196: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:197: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:198: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/ARM64Assembler.h:333: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AssemblerBuffer.h:44: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AssemblerBuffer.h:178: Wrong number of spaces before statement. (expected: 20) [whitespace/indent] [4]
Total errors found: 7 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 351035 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=351035&action=review r=me with remaining issues resolved. I think you missed the ARMv7Assembler::fillNops(). Incidentally, the ARMv7 port does use the copy function in fillNops() and needs a fix. > Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:973 > AssemblerType::fillNops(static_cast<char*>(buffer.data()) + startCodeSize, memoryToFillWithNopsInBytes, isCopyingToExecutableMemory); I think you need to update this to use the new fillNops() with memcpy. Strange, how did this build for x86_64 or any other non ARM64 build? Oh, I get it. The copyFunction argument's type is templated, and on all those platforms, the argument happens to be unused. Hence, x86_64 doesn't care that we passed it a boolean instead. I think it's misleading to leave this code in this state. We should make it pass memcpy for the 3rd arg here. > Source/JavaScriptCore/assembler/AssemblerBuffer.h:174 > + ) Fix indentation. Created attachment 351048 [details]
patch for landing
Attachment 351048 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/assembler/AbstractMacroAssembler.h:964: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:196: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:197: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/LinkBuffer.cpp:198: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/assembler/ARM64Assembler.h:333: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/assembler/AssemblerBuffer.h:44: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 6 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 351048 [details] patch for landing Clearing flags on attachment: 351048 Committed r236589: <https://trac.webkit.org/changeset/236589> All reviewed patches have been landed. Closing bug. This breaks compilation on Armv7. I created Bug 190080 to address this. |