Bug 196072 – [JSC] Fix build after r243232 on unsupported 64bit architectures

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

[Tomas Popela]

The build was broken due to:

#if CPU(X86_64) && CPU(ADDRESS64)
    static constexpr unsigned s_freeBitsAtTop = 16;
    static constexpr uintptr_t s_maskCompositeValueForPointer = 0x0000fffffffffff8;
#elif CPU(ARM64) && CPU(ADDRESS64)
    static constexpr unsigned s_freeBitsAtTop = 28;
    static constexpr uintptr_t s_maskCompositeValueForPointer = 0x0000000ffffffff8;
#endif

missing the values for other 64bit arches (such as ppc64, ppc64le, s390x). Mark suggested that we should put the code introduced in r243232 behing the USE() flag.

[Tomas Popela]

Created attachment 365529 [details]
Patch

[Saam Barati]

Comment on attachment 365529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365529&action=review

Also please undef your define at the bottom of the header

> Source/JavaScriptCore/bytecode/CodeOrigin.h:34
> +#define USE_COMPRESSED_CODE_ORIGIN CPU(ARM64) || CPU(X86_64)

We can only opt in (ARM64 && ADDRESS64)
We don’t want compression scheme for ARM64_32

[Mark Lam]

Comment on attachment 365529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365529&action=review

>> Source/JavaScriptCore/bytecode/CodeOrigin.h:34
>> +#define USE_COMPRESSED_CODE_ORIGIN CPU(ARM64) || CPU(X86_64)
> 
> We can only opt in (ARM64 && ADDRESS64)
> We don’t want compression scheme for ARM64_32

Hence, this line should say:
#define USE_COMPRESSED_CODE_ORIGIN (CPU(ADDRESS64) && (CPU(ARM64) || CPU(X86_64)))

That should exclude the CPU(ADDRESS64) architectures that cannot support this yet.

[Keith Miller]

I think we already expect 16 free bits at the top of any pointer for JSValue. Thus, I think you could change the code to:

#if CPU(ARM64) && CPU(ADDRESS64)
    static constexpr unsigned s_freeBitsAtTop = 28;
    static constexpr uintptr_t s_maskCompositeValueForPointer = 0x0000000ffffffff8;
#elif CPU(ADDRESS64)
    static constexpr unsigned s_freeBitsAtTop = 16;
    static constexpr uintptr_t s_maskCompositeValueForPointer = 0x0000fffffffffff8;
#endif

[Tomas Popela]

Thank you Saam, Mark and Keith for the feedback! I will go with the Keith's suggestion (when the patch passes our CI).

[Tomas Popela]

Created attachment 365594 [details]
Patch

[Keith Miller]

Comment on attachment 365594 [details]
Patch

r=me

[Tomas Popela]

Comment on attachment 365594 [details]
Patch

Clearing flags on attachment: 365594

Committed r243363: <https://trac.webkit.org/changeset/243363>

[Tomas Popela]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49141052>