Bug 196072 - [JSC] Fix build after r243232 on unsupported 64bit architectures
Summary: [JSC] Fix build after r243232 on unsupported 64bit architectures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tomas Popela
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-21 03:33 PDT by Tomas Popela
Modified: 2019-03-21 22:58 PDT (History)
6 users (show)

See Also:

Attachments
Patch (7.93 KB, patch)
2019-03-21 03:37 PDT, Tomas Popela 		no flags Details | Formatted Diff | Diff
Patch (1.87 KB, patch)
2019-03-21 12:45 PDT, Tomas Popela 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 2019-03-21 03:33:34 PDT 
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.
Comment 1 Tomas Popela 2019-03-21 03:37:20 PDT 
Created attachment 365529 [details]
Patch
Comment 2 Saam Barati 2019-03-21 09:25:23 PDT 
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
Comment 3 Mark Lam 2019-03-21 09:30:16 PDT 
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.
Comment 4 Keith Miller 2019-03-21 11:11:46 PDT 
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
Comment 5 Tomas Popela 2019-03-21 12:06:40 PDT 
Thank you Saam, Mark and Keith for the feedback! I will go with the Keith's suggestion (when the patch passes our CI).
Comment 6 Tomas Popela 2019-03-21 12:45:15 PDT 
Created attachment 365594 [details]
Patch
Comment 7 Keith Miller 2019-03-21 17:42:17 PDT 
Comment on attachment 365594 [details]
Patch

r=me
Comment 8 Tomas Popela 2019-03-21 22:56:08 PDT 
Comment on attachment 365594 [details]
Patch

Clearing flags on attachment: 365594

Committed r243363: <https://trac.webkit.org/changeset/243363>
Comment 9 Tomas Popela 2019-03-21 22:56:13 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-03-21 22:58:50 PDT 
<rdar://problem/49141052>