Bug 190057 – Verify the contents of AssemblerBuffer on arm64e

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

....

[Saam Barati]

Created attachment 351006 [details]
WIP

[Saam Barati]

<rdar://problem/38916630>

[Saam Barati]

Created attachment 351025 [details]
patch

[Saam Barati]

Created attachment 351029 [details]
patch

[EWS Watchlist]

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.

[Mark Lam]

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.

[Saam Barati]

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 351035 [details]
patch

[EWS Watchlist]

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.

[Mark Lam]

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.

[Saam Barati]

Created attachment 351048 [details]
patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 351048 [details]
patch for landing

Clearing flags on attachment: 351048

Committed r236589: <https://trac.webkit.org/changeset/236589>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Guillaume Emont]

This breaks compilation on Armv7. I created Bug 190080 to address this.