Bug 206703 - Fix OpenSource iphoneos arm64e build
Summary: Fix OpenSource iphoneos arm64e build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-23 14:59 PST by Jonathan Bedard
Modified: 2020-01-27 17:30 PST (History)
14 users (show)

See Also:


Attachments
Patch (3.11 KB, patch)
2020-01-23 15:22 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.18 KB, patch)
2020-01-24 11:36 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (8.61 KB, patch)
2020-01-24 11:41 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.43 KB, patch)
2020-01-24 15:54 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.53 KB, patch)
2020-01-27 14:54 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2020-01-23 14:59:27 PST
On arm64e, _STRUCT_ARM_THREAD_STATE64 is opaque. We need to handle this case so WebKit builds with arm64e.
Comment 1 Jonathan Bedard 2020-01-23 15:22:25 PST
Created attachment 388599 [details]
Patch
Comment 2 Yusuke Suzuki 2020-01-23 15:32:38 PST
Comment on attachment 388599 [details]
Patch

I think we should not use __opaque_xxx since it is opaque. When introducing PLATFORM_REGISTERS_WITH_PROFILE feature, we defined getter / setter for stack-pointer and instruction-pointer. And instructionPointerImpl and stackPointerImpl are the implementation for these getters and setters. Let's use the macro in getters / setters directly and avoid using stackPointerImpl / instructionPointerImpl for ARM64 / ARM64E Darwin.
Comment 3 Mark Lam 2020-01-23 15:34:17 PST
Comment on attachment 388599 [details]
Patch

r=me
Comment 4 Mark Lam 2020-01-23 15:37:03 PST
Comment on attachment 388599 [details]
Patch

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

> Source/JavaScriptCore/runtime/MachineContext.h:98
> +    return reinterpret_cast<void*&>(regs.__opaque_sp);

Actually, Yusuke is right.  Can you use the appropriate macros / setter / getters for this instead of accessing __opaque_sp directly?

> Source/JavaScriptCore/runtime/MachineContext.h:404
> +    return reinterpret_cast<void*&>(regs.__opaque_pc);

Ditto.
Comment 5 Jonathan Bedard 2020-01-24 11:36:10 PST
Created attachment 388711 [details]
Patch
Comment 6 Jonathan Bedard 2020-01-24 11:37:19 PST
(In reply to Mark Lam from comment #4)
> Comment on attachment 388599 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388599&action=review
> 
> > Source/JavaScriptCore/runtime/MachineContext.h:98
> > +    return reinterpret_cast<void*&>(regs.__opaque_sp);
> 
> Actually, Yusuke is right.  Can you use the appropriate macros / setter /
> getters for this instead of accessing __opaque_sp directly?
> 
> > Source/JavaScriptCore/runtime/MachineContext.h:404
> > +    return reinterpret_cast<void*&>(regs.__opaque_pc);
> 
> Ditto.

Drafted a change which uses the macros, it's substantially more complicated, though. Still working on testing it everywhere, but wanted it up for discussion.
Comment 7 Jonathan Bedard 2020-01-24 11:41:20 PST
Created attachment 388713 [details]
Patch
Comment 8 Yusuke Suzuki 2020-01-24 15:29:46 PST
Comment on attachment 388713 [details]
Patch

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

r=me with nit.

> Source/JavaScriptCore/runtime/MachineContext.h:60
> +#if OS(DARWIN) && !USE(PLATFORM_REGISTERS_WITH_PROFILE) && (CPU(ARM_THUMB2) || CPU(ARM) || CPU(ARM64))
> +#define USE_DARWIN_REGISTER_MACROS 1
> +#endif

Let's put this in wtf/PlatformUse.h
Comment 9 Jonathan Bedard 2020-01-24 15:54:53 PST
Created attachment 388735 [details]
Patch
Comment 10 Jonathan Bedard 2020-01-24 15:58:50 PST
(In reply to Jonathan Bedard from comment #9)
> Created attachment 388735 [details]
> Patch

I intend to land this on Monday, because while it's tested, it has not been exhaustively tested on every effected configuration.
Comment 11 WebKit Commit Bot 2020-01-27 12:06:50 PST
The commit-queue encountered the following flaky tests while processing attachment 388735 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2020-01-27 12:07:28 PST
Comment on attachment 388735 [details]
Patch

Clearing flags on attachment: 388735

Committed r255159: <https://trac.webkit.org/changeset/255159>
Comment 13 WebKit Commit Bot 2020-01-27 12:07:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2020-01-27 12:08:12 PST
<rdar://problem/58928799>
Comment 15 Ryan Haddad 2020-01-27 13:37:28 PST
Reverted r255159 for reason:

Broke the watchOS build.

Committed r255169: <https://trac.webkit.org/changeset/255169>
Comment 16 Jonathan Bedard 2020-01-27 14:54:36 PST
Created attachment 388917 [details]
Patch
Comment 17 WebKit Commit Bot 2020-01-27 17:30:13 PST
The commit-queue encountered the following flaky tests while processing attachment 388917 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org)
The commit-queue is continuing to process your patch.
Comment 18 WebKit Commit Bot 2020-01-27 17:30:46 PST
Comment on attachment 388917 [details]
Patch

Clearing flags on attachment: 388917

Committed r255216: <https://trac.webkit.org/changeset/255216>
Comment 19 WebKit Commit Bot 2020-01-27 17:30:48 PST
All reviewed patches have been landed.  Closing bug.