Bug 209322 - Update check for aarch64
Summary: Update check for aarch64
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-19 17:12 PDT by Michael Catanzaro
Modified: 2020-04-01 11:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.26 KB, patch)
2020-03-19 17:15 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.20 KB, patch)
2020-04-01 07:27 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.