Bug 209322

Summary: Update check for aarch64
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: Web Template FrameworkAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ddkilzer, ews-watchlist, mark.lam, mcatanzaro, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch none

Description Michael Catanzaro 2020-03-19 17:12:49 PDT 
/* CPU(ARM64) - Apple */
#if (defined(__arm64__) && defined(__APPLE__)) || defined(__aarch64__)
#define WTF_CPU_ARM64 1
#define WTF_CPU_KNOWN 1

#if defined(__arm64e__)
#define WTF_CPU_ARM64E 1
#endif
#endif


/* CPU(ARM64) - Apple */ is clearly a stale comment, since we're using CPU(ARM64) for Linux aarch64. Looks like the __APPLE__ check is obsolete. Let's see if removing it breaks anything?
Comment 1 Michael Catanzaro 2020-03-19 17:15:00 PDT 
Created attachment 394048 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2020-03-31 18:09:00 PDT 
Comment on attachment 394048 [details]
Patch

I seem to recall there were competing 64-bit ARM instruction sets when it originally came out--I'm not sure whether those got resolved, or if they're close enough that both of these can be defined as CPU(ARM64).

I'll CC Mark Lam as he might remember.
Comment 3 Mark Lam 2020-03-31 18:42:15 PDT 
(In reply to David Kilzer (:ddkilzer) from comment #2)
> Comment on attachment 394048 [details]
> Patch
> 
> I seem to recall there were competing 64-bit ARM instruction sets when it
> originally came out--I'm not sure whether those got resolved, or if they're
> close enough that both of these can be defined as CPU(ARM64).
> 
> I'll CC Mark Lam as he might remember.

I don't remember such a difference.  On Apple's side, I believe this change is fine.  I tested on all relevant build targets.  If this also works for linux, I'm fine with the change.
Comment 4 Mark Lam 2020-03-31 18:42:36 PDT 
Comment on attachment 394048 [details]
Patch

r=me
Comment 5 EWS 2020-03-31 19:06:55 PDT 
Committed r259332: <https://trac.webkit.org/changeset/259332>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394048 [details].
Comment 6 Radar WebKit Bug Importer 2020-03-31 19:07:11 PDT 
<rdar://problem/61135818>
Comment 7 Saam Barati 2020-03-31 21:08:00 PDT 
Comment on attachment 394048 [details]
Patch

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

> Source/WTF/wtf/PlatformCPU.h:116
> +#if defined(__arm64__) || defined(__aarch64__)

Doesn’t this also need to be done in bmalloc?
Comment 8 Michael Catanzaro 2020-04-01 07:25:17 PDT 
Yeah... but without a warning comment that the files need to be synced, how are devs to know? :(
Comment 9 Michael Catanzaro 2020-04-01 07:27:32 PDT 
(In reply to Mark Lam from comment #3)
> I don't remember such a difference.  On Apple's side, I believe this change
> is fine.  I tested on all relevant build targets.  If this also works for
> linux, I'm fine with the change.

This won't change anything on Linux. It's just cleaning up a condition that doesn't seem to be doing anything.
Comment 10 Michael Catanzaro 2020-04-01 07:27:41 PDT 
Created attachment 395165 [details]
Patch
Comment 11 Mark Lam 2020-04-01 07:37:46 PDT 
Comment on attachment 394048 [details]
Patch

Let’s not obsolete this first patch since it was landed, and not replaced.
Comment 12 EWS 2020-04-01 08:28:42 PDT 
Committed r259344: <https://trac.webkit.org/changeset/259344>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395165 [details].
Comment 13 Saam Barati 2020-04-01 11:25:09 PDT 
(In reply to Michael Catanzaro from comment #8)
> Yeah... but without a warning comment that the files need to be synced, how
> are devs to know? :(

Yeah it’s not great, but a lot of things in bmalloc and WTF need to be synced.