Bug 206703

Summary: Fix OpenSource iphoneos arm64e build
Product: WebKit Reporter: Jonathan Bedard <jbedard>
Component: JavaScriptCoreAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, ews-watchlist, keith_miller, mark.lam, msaboff, ryanhaddad, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=206620
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

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.